Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow ordering using column aliases #3931

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/PostgREST/ApiRequest/QueryParams.hs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ import PostgREST.ApiRequest.Types (AggregateFunction (..),
ListVal, LogicOperator (..),
LogicTree (..), OpExpr (..),
OpQuantifier (..), Operation (..),
OrderDirection (..),
OrderDirection (..), OrderElement (..),
OrderNulls (..), OrderTerm (..),
QuantOperator (..),
SelectItem (..),
Expand Down Expand Up @@ -762,11 +762,11 @@ pOrder :: Parser [OrderTerm]
pOrder = lexeme (try pOrderRelationTerm <|> pOrderTerm) `sepBy1` char ','
where
pOrderTerm = do
fld <- pField
elm <- pOrdElem
dir <- optionMaybe pOrdDir
nls <- optionMaybe pNulls <* pEnd <|>
pEnd $> Nothing
return $ OrderTerm fld dir nls
return $ OrderTerm elm dir nls

pOrderRelationTerm = do
nam <- pFieldName
Expand All @@ -775,6 +775,10 @@ pOrder = lexeme (try pOrderRelationTerm <|> pOrderTerm) `sepBy1` char ','
nls <- optionMaybe pNulls <* pEnd <|> pEnd $> Nothing
return $ OrderRelationTerm nam fld dir nls

pOrdElem :: Parser OrderElement
pOrdElem = OrderAlias <$> try (aliasSeparator *> pFieldName) <|>
OrderField <$> try pField

pNulls :: Parser OrderNulls
pNulls = try (pDelimiter *> string "nullsfirst" $> OrderNullsFirst) <|>
try (pDelimiter *> string "nullslast" $> OrderNullsLast)
Expand Down
8 changes: 7 additions & 1 deletion src/PostgREST/ApiRequest/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
, Operation (..)
, OpQuantifier(..)
, OrderDirection(..)
, OrderElement(..)
, OrderNulls(..)
, OrderTerm(..)
, SingleVal
Expand Down Expand Up @@ -65,7 +66,7 @@

data OrderTerm
= OrderTerm
{ otTerm :: Field
{ otTerm :: OrderElement

Check warning on line 69 in src/PostgREST/ApiRequest/Types.hs

View check run for this annotation

Codecov / codecov/patch

src/PostgREST/ApiRequest/Types.hs#L69

Added line #L69 was not covered by tests
, otDirection :: Maybe OrderDirection
, otNullOrder :: Maybe OrderNulls
}
Expand All @@ -77,6 +78,11 @@
}
deriving (Eq, Show)

data OrderElement
= OrderField Field
| OrderAlias FieldName
deriving (Eq, Show)

Check warning on line 84 in src/PostgREST/ApiRequest/Types.hs

View check run for this annotation

Codecov / codecov/patch

src/PostgREST/ApiRequest/Types.hs#L84

Added line #L84 was not covered by tests

data OrderDirection
= OrderAsc
| OrderDesc
Expand Down
6 changes: 4 additions & 2 deletions src/PostgREST/Plan.hs
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,8 @@ addOrders ctx ApiRequest{..} rReq =

resolveOrder :: ResolverContext -> OrderTerm -> CoercibleOrderTerm
resolveOrder _ (OrderRelationTerm a b c d) = CoercibleOrderRelationTerm a b c d
resolveOrder ctx (OrderTerm fld dir nulls) = CoercibleOrderTerm (resolveTypeOrUnknown ctx fld Nothing) dir nulls
resolveOrder _ (OrderTerm (OrderAlias ali) dir nulls) = CoercibleOrderAliasTerm ali dir nulls
resolveOrder ctx (OrderTerm (OrderField fld) dir nulls) = CoercibleOrderFieldTerm (resolveTypeOrUnknown ctx fld Nothing) dir nulls

-- Validates that the related resource on the order is an embedded resource,
-- e.g. if `clients` is inside the `select` in /projects?order=clients(id)&select=*,clients(*),
Expand All @@ -792,7 +793,6 @@ addRelatedOrders (Node rp@ReadPlan{order,from} forest) = do
newOrder <- newRelOrder `traverse` order
Node rp{order=newOrder} <$> addRelatedOrders `traverse` forest
where
newRelOrder cot@CoercibleOrderTerm{} = Right cot
newRelOrder cot@CoercibleOrderRelationTerm{coRelation} =
let foundRP = rootLabel <$> find (\(Node ReadPlan{relName, relAlias} _) -> coRelation == fromMaybe relName relAlias) forest in
case foundRP of
Expand All @@ -804,6 +804,8 @@ addRelatedOrders (Node rp@ReadPlan{order,from} forest) = do
else Left $ RelatedOrderNotToOne (qiName from) name
Nothing ->
Left $ NotEmbedded coRelation
newRelOrder cot = Right cot


-- | Searches for null filters on embeds, e.g. `projects=not.is.null` on `GET /clients?select=*,projects(*)&projects=not.is.null`
--
Expand Down
7 changes: 6 additions & 1 deletion src/PostgREST/Plan/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,16 @@
deriving (Eq, Show)

data CoercibleOrderTerm
= CoercibleOrderTerm
= CoercibleOrderFieldTerm
{ coField :: CoercibleField
, coDirection :: Maybe OrderDirection
, coNullOrder :: Maybe OrderNulls
}
| CoercibleOrderAliasTerm
{ coAlias :: Alias

Check warning on line 74 in src/PostgREST/Plan/Types.hs

View check run for this annotation

Codecov / codecov/patch

src/PostgREST/Plan/Types.hs#L74

Added line #L74 was not covered by tests
, coDirection :: Maybe OrderDirection
, coNullOrder :: Maybe OrderNulls
}
| CoercibleOrderRelationTerm
{ coRelation :: FieldName
, coRelTerm :: Field
Expand Down
3 changes: 2 additions & 1 deletion src/PostgREST/Query/SqlFragment.hs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,8 @@ pgFmtOrderTerm qi ot =
maybe mempty nullOrder $ coNullOrder ot])
where
fmtOTerm = \case
CoercibleOrderTerm{coField=cof} -> pgFmtField qi cof
CoercibleOrderFieldTerm{coField=cof} -> pgFmtField qi cof
CoercibleOrderAliasTerm{coAlias=coa} -> pgFmtIdent coa
CoercibleOrderRelationTerm{coRelation, coRelTerm=(fn, jp)} -> pgFmtField (QualifiedIdentifier mempty coRelation) (unknownField fn jp)

direction OrderAsc = "ASC"
Expand Down
3 changes: 3 additions & 0 deletions test/spec/Feature/Query/AggregateFunctionsSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ allowed =
it "returns the count grouped by all provided fields when other fields are selected" $
get "/projects?select=c:count(),client_id&order=client_id.desc" `shouldRespondWith`
[json|[{ "c": 1, "client_id": null }, { "c": 2, "client_id": 2 }, { "c": 2, "client_id": 1}]|] { matchHeaders = [matchContentTypeJson] }
it "works with a json field accessor and aggregates" $
get "/json_table?select=data->foo->>bar,count()&order=:bar.desc.nullslast" `shouldRespondWith`
[json|[{ "bar":"baz", "count":1}, {"bar":null, "count":2}]|] {matchHeaders = [matchContentTypeJson] }

context "performing a count by using it as a column (backwards compat)" $ do
it "returns the count of all rows when no other fields are selected" $
Expand Down
Loading