Skip to content

Commit

Permalink
(WIP) fix: spread embeds failing with count() aggregate
Browse files Browse the repository at this point in the history
- Fixed "column reference <col> is ambiguous" when selecting "?select=...table(col,count())"
- Fixed "column <json_aggregate>.<alias> does not exist" when selecting "?select=...table(aias:count())"
  • Loading branch information
laurenceisla committed Aug 20, 2024
1 parent 817f1e3 commit 16311f4
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 16 deletions.
29 changes: 20 additions & 9 deletions src/PostgREST/Plan.hs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import PostgREST.SchemaCache.Identifiers (FieldName,
QualifiedIdentifier (..),
RelIdentifier (..),
Schema)
--TableName)
import PostgREST.SchemaCache.Relationship (Cardinality (..),
Junction (..),
Relationship (..),
Expand Down Expand Up @@ -270,7 +271,7 @@ data ResolverContext = ResolverContext
}

resolveColumnField :: Column -> CoercibleField
resolveColumnField col = CoercibleField (colName col) mempty False (colNominalType col) Nothing (colDefault col)
resolveColumnField col = CoercibleField (colName col) mempty False (colNominalType col) Nothing (colDefault col) False

resolveTableFieldName :: Table -> FieldName -> CoercibleField
resolveTableFieldName table fieldName =
Expand Down Expand Up @@ -376,15 +377,24 @@ initReadRequest ctx@ResolverContext{qi=QualifiedIdentifier{..}} =
addAliases :: ReadPlanTree -> Either ApiRequestError ReadPlanTree
addAliases = Right . fmap addAliasToPlan
where
addAliasToPlan rp@ReadPlan{select=sel} = rp{select=map aliasSelectField sel}
addAliasToPlan rp@ReadPlan{select=sel, relIsSpread=spr} = rp{select=map (aliasSelectField spr) sel}

aliasSelectField :: CoercibleSelectField -> CoercibleSelectField
aliasSelectField field@CoercibleSelectField{csField=fieldDetails, csAggFunction=aggFun, csAlias=alias}
| isJust alias || isJust aggFun = field
aliasSelectField :: Bool -> CoercibleSelectField -> CoercibleSelectField
aliasSelectField isSpread field@CoercibleSelectField{csField=fieldDetails, csAggFunction=aggFun, csAlias=alias}
| isJust alias = field
| isJust aggFun = fieldAliasForSpreadAgg isSpread field
| isJsonKeyPath fieldDetails, Just key <- lastJsonKey fieldDetails = field { csAlias = Just key }
| isTransformPath fieldDetails = field { csAlias = Just (cfName fieldDetails) }
| otherwise = field

fieldAliasForSpreadAgg isSpread field@CoercibleSelectField{csField=fieldDetails, csAggFunction=aggFun} =
-- A request like: `/top_table?select=...middle_table(...nested_table(count()))` will `SELECT` the full row instead of `*`,
-- because doing a `COUNT(*)` in `top_table` would not return the desired results.
-- So we use the "count" alias if none is present since the field name won't be selected.
if isSpread && cfName fieldDetails == "*" && aggFun == Just Count
then field { csAlias = Just "count" }
else field

isJsonKeyPath CoercibleField{cfJsonPath=(_: _)} = True
isJsonKeyPath _ = False

Expand Down Expand Up @@ -703,9 +713,10 @@ hoistFromSelectFields relAggAlias fields =
let (modifiedField, maybeAgg) = modifyField field
in (modifiedField : newFields, maybeAgg : aggList)

modifyField field@CoercibleSelectField{csAggFunction=Just aggFunc, csField, csAggCast, csAlias} =
let determineFieldName = fromMaybe (cfName csField) csAlias
updatedField = field {csAggFunction = Nothing, csAggCast = Nothing}
modifyField field@CoercibleSelectField{csAggFunction=Just aggFunc, csField=fieldDetails, csAggCast, csAlias} =
let determineFieldName = fromMaybe (cfName fieldDetails) csAlias
isFullRow = cfName fieldDetails == "*" && aggFunc == Count
updatedField = field {csField = fieldDetails{cfFullRow = isFullRow}, csAggFunction = Nothing, csAggCast = Nothing}
hoistedField = Just ((relAggAlias, determineFieldName), (aggFunc, csAggCast, csAlias))
in (updatedField, hoistedField)
modifyField field = (field, Nothing)
Expand Down Expand Up @@ -858,7 +869,7 @@ addNullEmbedFilters (Node rp@ReadPlan{where_=curLogic} forest) = do
newNullFilters rPlans = \case
(CoercibleExpr b lOp trees) ->
CoercibleExpr b lOp <$> (newNullFilters rPlans `traverse` trees)
flt@(CoercibleStmnt (CoercibleFilter (CoercibleField fld [] _ _ _ _) opExpr)) ->
flt@(CoercibleStmnt (CoercibleFilter (CoercibleField fld [] _ _ _ _ _) opExpr)) ->
let foundRP = find (\ReadPlan{relName, relAlias} -> fld == fromMaybe relName relAlias) rPlans in
case (foundRP, opExpr) of
(Just ReadPlan{relAggAlias}, OpExpr b (Is TriNull)) -> Right $ CoercibleStmnt $ CoercibleFilterNullEmbed b relAggAlias
Expand Down
3 changes: 2 additions & 1 deletion src/PostgREST/Plan/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ data CoercibleField = CoercibleField
, cfIRType :: Text -- ^ The native Postgres type of the field, the intermediate (IR) type before mapping.
, cfTransform :: Maybe TransformerProc -- ^ The optional mapping from irType -> targetType.
, cfDefault :: Maybe Text
, cfFullRow :: Bool -- ^ True if the field represents the whole selected row. Used in spread rels: instead of COUNT(*), it does a COUNT(<row>) in order to not mix with other spreaded resources.
} deriving (Eq, Show)

unknownField :: FieldName -> JsonPath -> CoercibleField
unknownField name path = CoercibleField name path False "" Nothing Nothing
unknownField name path = CoercibleField name path False "" Nothing Nothing False

-- | Like an API request LogicTree, but with coercible field information.
data CoercibleLogicTree
Expand Down
2 changes: 1 addition & 1 deletion src/PostgREST/Query/QueryBuilder.hs
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ callPlanToQuery (FunctionCall qi params arguments returnsScalar returnsSetOfScal
KeyParams [] -> "FROM " <> callIt mempty
KeyParams prms -> case arguments of
DirectArgs args -> "FROM " <> callIt (fmtArgs prms args)
JsonArgs json -> fromJsonBodyF json ((\p -> CoercibleField (ppName p) mempty False (ppTypeMaxLength p) Nothing Nothing) <$> prms) False True False <> ", " <>
JsonArgs json -> fromJsonBodyF json ((\p -> CoercibleField (ppName p) mempty False (ppTypeMaxLength p) Nothing Nothing False) <$> prms) False True False <> ", " <>
"LATERAL " <> callIt (fmtParams prms)

callIt :: SQL.Snippet -> SQL.Snippet
Expand Down
1 change: 1 addition & 0 deletions src/PostgREST/Query/SqlFragment.hs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ pgFmtCallUnary :: Text -> SQL.Snippet -> SQL.Snippet
pgFmtCallUnary f x = SQL.sql (encodeUtf8 f) <> "(" <> x <> ")"

pgFmtField :: QualifiedIdentifier -> CoercibleField -> SQL.Snippet
pgFmtField table CoercibleField{cfFullRow=True} = fromQi table
pgFmtField table CoercibleField{cfName=fn, cfJsonPath=[]} = pgFmtColumn table fn
pgFmtField table CoercibleField{cfName=fn, cfToJson=doToJson, cfJsonPath=jp} | doToJson = "to_jsonb(" <> pgFmtColumn table fn <> ")" <> pgFmtJsonPath jp
| otherwise = pgFmtColumn table fn <> pgFmtJsonPath jp
Expand Down
13 changes: 8 additions & 5 deletions test/spec/fixtures/data.sql
Original file line number Diff line number Diff line change
Expand Up @@ -895,11 +895,13 @@ INSERT INTO process_categories VALUES (1, 'Batch');
INSERT INTO process_categories VALUES (2, 'Mass');

TRUNCATE TABLE processes CASCADE;
INSERT INTO processes VALUES (1, 'Process A1', 1, 1);
INSERT INTO processes VALUES (2, 'Process A2', 1, 2);
INSERT INTO processes VALUES (3, 'Process B1', 2, 1);
INSERT INTO processes VALUES (4, 'Process B2', 2, 1);
INSERT INTO processes VALUES (5, 'Process C1', 3, 2);
INSERT INTO processes VALUES (1, 'Process A1', 23, 1, 1);
INSERT INTO processes VALUES (2, 'Process A2', 23, 1, 2);
INSERT INTO processes VALUES (3, 'Process B1', 23, 2, 1);
INSERT INTO processes VALUES (4, 'Process B2', 23, 2, 1);
INSERT INTO processes VALUES (5, 'Process C1', 23, 3, 2);
INSERT INTO processes VALUES (6, 'Process C2', 23, 3, 2);
INSERT INTO processes VALUES (7, 'Process XX', 23, 3, 2);

TRUNCATE TABLE process_costs CASCADE;
INSERT INTO process_costs VALUES (1, 150.00);
Expand All @@ -922,3 +924,4 @@ INSERT INTO process_supervisor VALUES (3, 4);
INSERT INTO process_supervisor VALUES (4, 1);
INSERT INTO process_supervisor VALUES (4, 2);
INSERT INTO process_supervisor VALUES (5, 3);
INSERT INTO process_supervisor VALUES (6, 3);
1 change: 1 addition & 0 deletions test/spec/fixtures/schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -3763,6 +3763,7 @@ create table process_categories (
create table processes (
id int primary key,
name text,
count int,
factory_id int references factories(id),
category_id int references process_categories(id)
);
Expand Down

0 comments on commit 16311f4

Please sign in to comment.