From 673aa25082cde4e6c5a9631e1d4ee0d9f4335cf8 Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Thu, 31 Jan 2019 20:16:59 -0500 Subject: [PATCH] Fix #1221, embedding when having a self join --- CHANGELOG.md | 1 + src/PostgREST/DbRequestBuilder.hs | 39 ++++++++++++++++++++----------- src/PostgREST/QueryBuilder.hs | 35 +++++++++------------------ src/PostgREST/Types.hs | 11 ++++++--- test/Feature/QuerySpec.hs | 36 ++++++++++++++++++++++++++++ test/fixtures/data.sql | 18 ++++++++++---- test/fixtures/privileges.sql | 1 + test/fixtures/schema.sql | 12 +++++++--- 8 files changed, 105 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 084c50a8f..449c26eae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - #1223, Fix incorrect OpenAPI externalDocs url - @steve-chavez +- #1221, Fix embedding other resources when having a self join - @steve-chavez ## [5.2.0] - 2018-12-12 diff --git a/src/PostgREST/DbRequestBuilder.hs b/src/PostgREST/DbRequestBuilder.hs index 3d8c34cca..2cd92dc89 100644 --- a/src/PostgREST/DbRequestBuilder.hs +++ b/src/PostgREST/DbRequestBuilder.hs @@ -76,7 +76,7 @@ readRequest maxRows allRels proc apiRequest = buildReadRequest fieldTree = let rootDepth = 0 rootNodeName = if action == ActionRead then rootTableName else sourceCTEName in - foldr (treeEntry rootDepth) (Node (Select [] rootNodeName [] [] [] [] allRange, (rootNodeName, Nothing, Nothing, Nothing, rootDepth)) []) fieldTree + foldr (treeEntry rootDepth) (Node (Select [] rootNodeName Nothing [] [] [] [] allRange, (rootNodeName, Nothing, Nothing, Nothing, rootDepth)) []) fieldTree where treeEntry :: Depth -> Tree SelectItem -> ReadRequest -> ReadRequest treeEntry depth (Node fld@((fn, _),_,alias,relationDetail) fldForest) (Node (q, i) rForest) = @@ -84,7 +84,7 @@ readRequest maxRows allRels proc apiRequest = case fldForest of [] -> Node (q {select=fld:select q}, i) rForest _ -> Node (q, i) $ - foldr (treeEntry nxtDepth) (Node (Select [] fn [] [] [] [] allRange, (fn, Nothing, alias, relationDetail, nxtDepth)) []) fldForest:rForest + foldr (treeEntry nxtDepth) (Node (Select [] fn Nothing [] [] [] [] allRange, (fn, Nothing, alias, relationDetail, nxtDepth)) []) fldForest:rForest relations :: [Relation] relations = case action of @@ -116,7 +116,7 @@ treeRestrictRange maxRows_ request = pure $ nodeRestrictRange maxRows_ `fmap` re augumentRequestWithJoin :: Schema -> [Relation] -> ReadRequest -> Either ApiRequestError ReadRequest augumentRequestWithJoin schema allRels request = addRelations schema allRels Nothing request - >>= addJoinConditions schema + >>= addJoinConditions schema Nothing addRelations :: Schema -> [Relation] -> Maybe ReadRequest -> ReadRequest -> Either ApiRequestError ReadRequest addRelations schema allRelations parentNode (Node (query@Select{from=tbl}, (nodeName, _, alias, relationDetail, depth)) forest) = @@ -216,10 +216,11 @@ findRelation schema allRelations nodeTableName parentNodeTableName relationDetai ) ) allRelations -addJoinConditions :: Schema -> ReadRequest -> Either ApiRequestError ReadRequest -addJoinConditions schema (Node node@(query, nodeProps@(_, relation, _, _, _)) forest) = +-- previousAlias is only used for the case of self joins +addJoinConditions :: Schema -> Maybe Alias -> ReadRequest -> Either ApiRequestError ReadRequest +addJoinConditions schema previousAlias (Node node@(query@Select{from=tbl}, nodeProps@(_, relation, _, _, depth)) forest) = case relation of - Just Relation{relType=Root} -> Node node <$> updatedForest -- this is the root node + Just Relation{relType=Root} -> Node node <$> updatedForest -- this is the root node Just rel@Relation{relType=Parent} -> Node (augmentQuery rel, nodeProps) <$> updatedForest Just rel@Relation{relType=Child} -> Node (augmentQuery rel, nodeProps) <$> updatedForest Just rel@Relation{relType=Many, relLinkTable=(Just linkTable)} -> @@ -227,13 +228,21 @@ addJoinConditions schema (Node node@(query, nodeProps@(_, relation, _, _, _)) fo Node (rq{implicitJoins=tableName linkTable:implicitJoins rq}, nodeProps) <$> updatedForest _ -> Left UnknownRelation where - updatedForest = mapM (addJoinConditions schema) forest - augmentQuery rel = foldr addJoinCond query (getJoinConditions rel) - addJoinCond :: JoinCondition -> ReadQuery -> ReadQuery - addJoinCond jc rq@Select{joinConditions=jcs} = rq{joinConditions=jc:jcs} + newAlias = case isSelfJoin <$> relation of + Just True + | depth /= 0 -> Just (tbl <> "_" <> show depth) -- root node doesn't get aliased + | otherwise -> Nothing + _ -> Nothing + augmentQuery rel = + foldr + (\jc rq@Select{joinConditions=jcs} -> rq{joinConditions=jc:jcs}) + query{fromAlias=newAlias} + (getJoinConditions previousAlias newAlias rel) + updatedForest = mapM (addJoinConditions schema newAlias) forest -getJoinConditions :: Relation -> [JoinCondition] -getJoinConditions (Relation Table{tableSchema=tSchema, tableName=tN} cols Table{tableName=ftN} fCols typ lt lc1 lc2) = +-- previousAlias and newAlias are used in the case of self joins +getJoinConditions :: Maybe Alias -> Maybe Alias -> Relation -> [JoinCondition] +getJoinConditions previousAlias newAlias (Relation Table{tableSchema=tSchema, tableName=tN} cols Table{tableName=ftN} fCols typ lt lc1 lc2) = if | typ == Child || typ == Parent -> zipWith (toJoinCondition tN ftN) cols fCols | typ == Many -> @@ -243,8 +252,10 @@ getJoinConditions (Relation Table{tableSchema=tSchema, tableName=tN} cols Table{ where toJoinCondition :: Text -> Text -> Column -> Column -> JoinCondition toJoinCondition tb ftb c fc = - JoinCondition (QualifiedIdentifier tSchema tb, Nothing, colName c) - (QualifiedIdentifier tSchema ftb, Nothing, colName fc) + let qi1 = QualifiedIdentifier tSchema tb + qi2 = QualifiedIdentifier tSchema ftb in + JoinCondition (maybe qi1 (QualifiedIdentifier mempty) newAlias, colName c) + (maybe qi2 (QualifiedIdentifier mempty) previousAlias, colName fc) addFiltersOrdersRanges :: ApiRequest -> Either ApiRequestError (ReadRequest -> ReadRequest) addFiltersOrdersRanges apiRequest = foldr1 (liftA2 (.)) [ diff --git a/src/PostgREST/QueryBuilder.hs b/src/PostgREST/QueryBuilder.hs index a658abce6..8ba5f3179 100644 --- a/src/PostgREST/QueryBuilder.hs +++ b/src/PostgREST/QueryBuilder.hs @@ -219,32 +219,21 @@ requestToCountQuery schema (DbRead (Node (Select{where_=logicForest}, (mainTbl, qi = removeSourceCTESchema schema mainTbl requestToQuery :: Schema -> Bool -> DbRequest -> SqlQuery -requestToQuery schema isParent (DbRead (Node (Select colSelects tbl implJoins logicForest joinConditions_ ordts range, (_, maybeRelation, _, _, depth)) forest)) = +requestToQuery schema isParent (DbRead (Node (Select colSelects tbl tblAlias implJoins logicForest joinConditions_ ordts range, _) forest)) = unwords [ "SELECT " <> intercalate ", " (map (pgFmtSelectItem qi) colSelects ++ selects), - "FROM " <> intercalate ", " tables, + "FROM " <> intercalate ", " (tabl : implJs), unwords joins, - ("WHERE " <> intercalate " AND " (map (pgFmtLogicTree qi) logicForest ++ map pgFmtJoinCondition joinConds)) - `emptyOnFalse` (null logicForest && null joinConds), + ("WHERE " <> intercalate " AND " (map (pgFmtLogicTree qi) logicForest ++ map pgFmtJoinCondition joinConditions_)) + `emptyOnFalse` (null logicForest && null joinConditions_), ("ORDER BY " <> intercalate ", " (map (pgFmtOrderTerm qi) ordts)) `emptyOnFalse` null ordts, ("LIMIT " <> maybe "ALL" show (rangeLimit range) <> " OFFSET " <> show (rangeOffset range)) `emptyOnFalse` (isParent || range == allRange) ] where - tbls = tbl:implJoins - isSelfJoin = maybe False (\r -> relType r /= Root && relTable r == relFTable r) maybeRelation - (qi, tables, joinConds) = - let depthAlias name dpth = if dpth /= 0 then name <> "_" <> show dpth else name in -- Root node doesn't get aliased - if isSelfJoin - then ( - QualifiedIdentifier "" (depthAlias tbl depth), - (\t -> fromQi (removeSourceCTESchema schema t) <> " AS " <> pgFmtIdent (depthAlias t depth)) <$> tbls, - (\(JoinCondition (qi1, _, c1) (qi2, _, c2)) -> - JoinCondition (qi1, Just $ depthAlias (qiName qi1) depth, c1) - (qi2, Just $ depthAlias (qiName qi2) (depth - 1), c2)) <$> joinConditions_) - else ( - removeSourceCTESchema schema tbl, - fromQi . removeSourceCTESchema schema <$> tbls, - joinConditions_) + implJs = fromQi . QualifiedIdentifier schema <$> implJoins + mainQi = removeSourceCTESchema schema tbl + tabl = fromQi mainQi <> maybe mempty (\a -> " AS " <> pgFmtIdent a) tblAlias + qi = maybe mainQi (QualifiedIdentifier mempty) tblAlias (joins, selects) = foldr getQueryParts ([],[]) forest @@ -437,11 +426,9 @@ pgFmtFilter table (Filter fld (OpExpr hasNot oper)) = notOp <> " " <> case oper (find ((==) . toLower $ v) ["null","true","false"]) pgFmtJoinCondition :: JoinCondition -> SqlFragment -pgFmtJoinCondition (JoinCondition (qi, al1, col1) (QualifiedIdentifier schema fTable, al2, col2)) = - pgFmtColumn (fromMaybe qi $ aliasToQi al1) col1 <> " = " <> - pgFmtColumn (fromMaybe (removeSourceCTESchema schema fTable) $ aliasToQi al2) col2 - where - aliasToQi al = QualifiedIdentifier "" <$> al +pgFmtJoinCondition (JoinCondition (qi, col1) (QualifiedIdentifier schema fTable, col2)) = + pgFmtColumn qi col1 <> " = " <> + pgFmtColumn (removeSourceCTESchema schema fTable) col2 pgFmtLogicTree :: QualifiedIdentifier -> LogicTree -> SqlFragment pgFmtLogicTree qi (Expr hasNot op forest) = notOp <> " (" <> intercalate (" " <> show op <> " ") (pgFmtLogicTree qi <$> forest) <> ")" diff --git a/src/PostgREST/Types.hs b/src/PostgREST/Types.hs index 44c88e449..e2d27a2e9 100644 --- a/src/PostgREST/Types.hs +++ b/src/PostgREST/Types.hs @@ -184,6 +184,9 @@ data Relation = Relation { , relLinkCols2 :: Maybe [Column] } deriving (Show, Eq) +isSelfJoin :: Relation -> Bool +isSelfJoin r = relType r /= Root && relTable r == relFTable r + -- | Cached attributes of a JSON payload data PayloadJSON = PayloadJSON { -- | This is the raw ByteString that comes from the request body. @@ -307,13 +310,15 @@ type SelectItem = (Field, Maybe Cast, Maybe Alias, Maybe RelationDetail) -- | Path of the embedded levels, e.g "clients.projects.name=eq.." gives Path ["clients", "projects"] type EmbedPath = [Text] data Filter = Filter { field::Field, opExpr::OpExpr } deriving (Show, Eq) -data JoinCondition = JoinCondition (QualifiedIdentifier, Maybe Alias, FieldName) - (QualifiedIdentifier, Maybe Alias, FieldName) deriving (Show, Eq) +data JoinCondition = JoinCondition (QualifiedIdentifier, FieldName) + (QualifiedIdentifier, FieldName) deriving (Show, Eq) data ReadQuery = Select { select :: [SelectItem] , from :: TableName --- | Only used for many to many joins. Parent and Child joins use explicit joins. +-- | A table alias is used in case of self joins + , fromAlias :: Maybe Alias +-- | Only used for Many to Many joins. Parent and Child joins use explicit joins. , implicitJoins :: [TableName] , where_ :: [LogicTree] , joinConditions :: [JoinCondition] diff --git a/test/Feature/QuerySpec.hs b/test/Feature/QuerySpec.hs index 6b52d086e..f8e4fda7f 100644 --- a/test/Feature/QuerySpec.hs +++ b/test/Feature/QuerySpec.hs @@ -589,6 +589,42 @@ spec = do ] }]|] { matchHeaders = [matchContentTypeJson] } + it "embeds other relations(manager) besides the self reference" $ do + get "/organizations?select=name,manager(name),referee(name,manager(name),auditor(name,manager(name))),auditor(name,manager(name),referee(name,manager(name)))&id=eq.5" `shouldRespondWith` + [json|[{ + "name":"Cyberdyne", + "manager":{"name":"Cyberdyne Manager"}, + "referee":{ + "name":"Acme", + "manager":{"name":"Acme Manager"}, + "auditor":{ + "name":"Auditor Org", + "manager":{"name":"Auditor Manager"}}}, + "auditor":{ + "name":"Umbrella", + "manager":{"name":"Umbrella Manager"}, + "referee":{ + "name":"Referee Org", + "manager":{"name":"Referee Manager"}}} + }]|] { matchHeaders = [matchContentTypeJson] } + + get "/organizations?select=name,manager(name),auditees:organizations.auditor(name,manager(name),refereeds:organizations.referee(name,manager(name)))&id=eq.2" `shouldRespondWith` + [json|[{ + "name":"Auditor Org", + "manager":{"name":"Auditor Manager"}, + "auditees":[ + {"name":"Acme", + "manager":{"name":"Acme Manager"}, + "refereeds":[ + {"name":"Cyberdyne", + "manager":{"name":"Cyberdyne Manager"}}, + {"name":"Oscorp", + "manager":{"name":"Oscorp Manager"}}]}, + {"name":"Umbrella", + "manager":{"name":"Umbrella Manager"}, + "refereeds":[]}] + }]|] { matchHeaders = [matchContentTypeJson] } + describe "ordering response" $ do it "by a column asc" $ get "/items?id=lte.2&order=id.asc" diff --git a/test/fixtures/data.sql b/test/fixtures/data.sql index 94dfc4df9..d3921378c 100644 --- a/test/fixtures/data.sql +++ b/test/fixtures/data.sql @@ -354,11 +354,21 @@ INSERT INTO family_tree VALUES ('3', 'Kid Two', '1'); INSERT INTO family_tree VALUES ('4', 'Grandkid One', '2'); INSERT INTO family_tree VALUES ('5', 'Grandkid Two', '3'); +TRUNCATE TABLE managers CASCADE; +INSERT INTO managers VALUES (1, 'Referee Manager'); +INSERT INTO managers VALUES (2, 'Auditor Manager'); +INSERT INTO managers VALUES (3, 'Acme Manager'); +INSERT INTO managers VALUES (4, 'Umbrella Manager'); +INSERT INTO managers VALUES (5, 'Cyberdyne Manager'); +INSERT INTO managers VALUES (6, 'Oscorp Manager'); + TRUNCATE TABLE organizations CASCADE; -INSERT INTO organizations VALUES (1, 'Referee Org', null, null); -INSERT INTO organizations VALUES (2, 'Auditor Org', null, null); -INSERT INTO organizations VALUES (3, 'Acme', 1, 2); -INSERT INTO organizations VALUES (4, 'Umbrella', 1, 2); +INSERT INTO organizations VALUES (1, 'Referee Org', null, null, 1); +INSERT INTO organizations VALUES (2, 'Auditor Org', null, null, 2); +INSERT INTO organizations VALUES (3, 'Acme', 1, 2, 3); +INSERT INTO organizations VALUES (4, 'Umbrella', 1, 2, 4); +INSERT INTO organizations VALUES (5, 'Cyberdyne', 3, 4, 5); +INSERT INTO organizations VALUES (6, 'Oscorp', 3, 4, 6); SET search_path = private, pg_catalog; diff --git a/test/fixtures/privileges.sql b/test/fixtures/privileges.sql index 5c12c5ed3..c78a11172 100644 --- a/test/fixtures/privileges.sql +++ b/test/fixtures/privileges.sql @@ -68,6 +68,7 @@ GRANT ALL ON TABLE , tiobe_pls , only_pk , family_tree + , managers , organizations , authors , books diff --git a/test/fixtures/schema.sql b/test/fixtures/schema.sql index 2f9f22e1d..f0339fb2f 100755 --- a/test/fixtures/schema.sql +++ b/test/fixtures/schema.sql @@ -1115,8 +1115,8 @@ CREATE FUNCTION setprojects(id_l int, id_h int, name text) RETURNS SETOF project $_$; create table images ( - name text not null, - img bytea not null + name text not null, + img bytea not null ); create view images_base64 as ( @@ -1386,11 +1386,17 @@ create table test.family_tree ( ); alter table only test.family_tree add constraint pptr foreign key (parent) references test.family_tree(id); +create table test.managers ( + id integer primary key, + name text +); + create table test.organizations ( id integer primary key, name text, referee integer, - auditor integer + auditor integer, + manager_id integer references managers(id) ); alter table only test.organizations add constraint pptr1 foreign key (referee) references test.organizations(id); alter table only test.organizations add constraint pptr2 foreign key (auditor) references test.organizations(id);