From ff709a65e5d3b53922f0fefafa24becd4368580a Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Wed, 31 Jan 2018 09:11:23 -0500 Subject: [PATCH] Add FROM targets aliasing to avoid conflict in embeds --- CHANGELOG.md | 1 + src/PostgREST/DbRequestBuilder.hs | 56 ++++++++++++++---------- src/PostgREST/QueryBuilder.hs | 24 ++++++----- src/PostgREST/Types.hs | 5 ++- test/Feature/QuerySpec.hs | 71 +++++++++++++++++++++++++++++++ test/fixtures/data.sql | 13 ++++++ test/fixtures/privileges.sql | 2 + test/fixtures/schema.sql | 16 +++++++ 8 files changed, 154 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9880dda25..d6c60a5d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #828, Fix computed column only working in public schema - @steve-chavez - #925, Fix RPC high memory usage by using parametrized query and avoiding json encoding - @steve-chavez +- #987, Fix embedding with self-reference foreign key - @steve-chavez ### Changed diff --git a/src/PostgREST/DbRequestBuilder.hs b/src/PostgREST/DbRequestBuilder.hs index c0cecedec..61e799139 100644 --- a/src/PostgREST/DbRequestBuilder.hs +++ b/src/PostgREST/DbRequestBuilder.hs @@ -60,17 +60,21 @@ readRequest maxRows allRels proc apiRequest = _ -> Nothing + -- Build tree with a Level attribute so when an embed occurs and the parent node has the same name as the child we can differentiate them by having + -- an alias like "node_lvl", this is related to issue #987. buildReadRequest :: [Tree SelectItem] -> ReadRequest buildReadRequest fieldTree = - let rootNodeName = if action == ActionRead then rootTableName else sourceCTEName in - foldr treeEntry (Node (Select [] [rootNodeName] [] [] [] allRange, (rootNodeName, Nothing, Nothing, Nothing)) []) fieldTree + let rootLvl = 1 + rootNodeName = if action == ActionRead then rootTableName else sourceCTEName in + foldr (treeEntry rootLvl) (Node (Select [] [rootNodeName] [] [] [] allRange, (rootNodeName, Nothing, Nothing, Nothing, rootLvl)) []) fieldTree where - treeEntry :: Tree SelectItem -> ReadRequest -> ReadRequest - treeEntry (Node fld@((fn, _),_,alias,relationDetail) fldForest) (Node (q, i) rForest) = + treeEntry :: Level -> Tree SelectItem -> ReadRequest -> ReadRequest + treeEntry lvl (Node fld@((fn, _),_,alias,relationDetail) fldForest) (Node (q, i) rForest) = + let nxtLvl = succ lvl in case fldForest of [] -> Node (q {select=fld:select q}, i) rForest _ -> Node (q, i) $ - foldr treeEntry (Node (Select [] [fn] [] [] [] allRange, (fn, Nothing, alias, relationDetail)) []) fldForest:rForest + foldr (treeEntry nxtLvl) (Node (Select [] [fn] [] [] [] allRange, (fn, Nothing, alias, relationDetail, nxtLvl)) []) fldForest:rForest relations :: [Relation] relations = case action of @@ -105,9 +109,9 @@ augumentRequestWithJoin schema allRels request = >>= addJoinConditions schema addRelations :: Schema -> [Relation] -> Maybe ReadRequest -> ReadRequest -> Either ApiRequestError ReadRequest -addRelations schema allRelations parentNode (Node readNode@(query, (name, _, alias, relationDetail)) forest) = +addRelations schema allRelations parentNode (Node readNode@(query, (name, _, alias, relationDetail, level)) forest) = case parentNode of - (Just (Node (Select{from=[parentNodeTable]}, (_, _, _, _)) _)) -> + (Just (Node (Select{from=[parentNodeTable]}, _) _)) -> Node <$> readNode' <*> forest' where forest' = updateForest $ hush node' @@ -190,13 +194,13 @@ addRelations schema allRelations parentNode (Node readNode@(query, (name, _, ali ) ) allRelations n `colMatches` rc = (toS ("^" <> rc <> "_?(?:|[iI][dD]|[fF][kK])$") :: BS.ByteString) =~ (toS n :: BS.ByteString) - addRel :: (ReadQuery, (NodeName, Maybe Relation, Maybe Alias, Maybe RelationDetail)) -> Relation -> (ReadQuery, (NodeName, Maybe Relation, Maybe Alias, Maybe RelationDetail)) - addRel (query', (n, _, a, _)) r = (query' {from=fromRelation}, (n, Just r, a, Nothing)) + addRel :: (ReadQuery, (NodeName, Maybe Relation, Maybe Alias, Maybe RelationDetail, Level)) -> Relation -> (ReadQuery, (NodeName, Maybe Relation, Maybe Alias, Maybe RelationDetail, Level)) + addRel (query', (n, _, a, _, lvl)) r = (query' {from=fromRelation}, (n, Just r, a, Nothing, lvl)) where fromRelation = map (\t -> if t == n then tableName (relTable r) else t) (from query') _ -> n' <$> updateForest (Just (n' forest)) where - n' = Node (query, (name, Just r, alias, Nothing)) + n' = Node (query, (name, Just r, alias, Nothing, level)) t = Table schema name Nothing True -- !!! TODO find another way to get the table from the query r = Relation t [] t [] Root Nothing Nothing Nothing where @@ -204,7 +208,7 @@ addRelations schema allRelations parentNode (Node readNode@(query, (name, _, ali updateForest n = mapM (addRelations schema allRelations n) forest addJoinConditions :: Schema -> ReadRequest -> Either ApiRequestError ReadRequest -addJoinConditions schema (Node node@(query, nodeProps@(_, relation, _, _)) forest) = +addJoinConditions schema (Node node@(query, nodeProps@(_, relation, _, _, lvl)) forest) = case relation of Just Relation{relType=Root} -> Node node <$> updatedForest -- this is the root node Just rel@Relation{relType=Parent} -> Node (augmentQuery rel, nodeProps) <$> updatedForest @@ -215,23 +219,31 @@ addJoinConditions schema (Node node@(query, nodeProps@(_, relation, _, _)) fores _ -> Left UnknownRelation where updatedForest = mapM (addJoinConditions schema) forest - augmentQuery rel = foldr addJoinCondToReadQuery query (getJoinConds rel) - addJoinCondToReadQuery jc rq@Select{joinConds=jcs} = rq{joinConds=jc:jcs}::ReadQuery + augmentQuery rel = foldr addJoinCondToReadQuery query (getJoinConds lvl rel) + addJoinCondToReadQuery jc rq@Select{joinConds=jcs} = rq{joinConds=jc:jcs} -getJoinConds :: Relation -> [JoinCond] -getJoinConds (Relation t cols ft fcs typ lt lc1 lc2) = +getJoinConds :: Integer -> Relation -> [JoinCond] +getJoinConds level (Relation t cols ft fcs typ lt lc1 lc2) = case typ of - Child -> zipWith (toJoinCond tN ftN) cols fcs - Parent -> zipWith (toJoinCond tN ftN) cols fcs - Many -> zipWith (toJoinCond tN ltN) cols (fromMaybe [] lc1) ++ zipWith (toJoinCond ftN ltN) fcs (fromMaybe [] lc2) + -- JoinCond needs the Level attr to know the tables aliases + -- The level depends on the sql query structure + -- Child has the embed as: + -- SELECT .., COALESCE(SELECT .. FROM ch AS ch_lvl_2 WHERE ch_lvl_2.col = p_lvl_1.col) FROM p AS p_lvl_1 + -- Parent has similar structure regarding the levels + -- Many has the embed as: + -- SELECT .., COALESCE(SELECT .. FROM ch AS ch_lvl_2, gch AS gch_lvl_2 WHERE ch_lvl_2.col = gch_lvl_2.col AND p_lvl_1.acol = ch_lvl_2.acol) + -- FROM p AS p_lvl_1 + Child -> zipWith (toJoinCond (tN, level) (ftN, level - 1)) cols fcs + Parent -> zipWith (toJoinCond (tN, level) (ftN, level - 1)) cols fcs + Many -> zipWith (toJoinCond (tN, level) (ltN, level)) cols (fromMaybe [] lc1) ++ zipWith (toJoinCond (ftN, level - 1) (ltN, level)) fcs (fromMaybe [] lc2) Root -> undefined where s = if typ == Parent then "" else tableSchema t tN = tableName t ftN = tableName ft ltN = fromMaybe "" (tableName <$> lt) - toJoinCond :: Text -> Text -> Column -> Column -> JoinCond - toJoinCond tb ftb c fc = JoinCond (QualifiedIdentifier s tb, colName c) (QualifiedIdentifier s ftb, colName fc) + toJoinCond :: (Text, Integer) -> (Text, Integer) -> Column -> Column -> JoinCond + toJoinCond (tb, tLvl) (ftb, fLvl) c fc = JoinCond (QualifiedIdentifier s tb, colName c, tLvl) (QualifiedIdentifier s ftb, colName fc, fLvl) addFiltersOrdersRanges :: ApiRequest -> Either ApiRequestError (ReadRequest -> ReadRequest) addFiltersOrdersRanges apiRequest = foldr1 (liftA2 (.)) [ @@ -304,7 +316,7 @@ addProperty f (path, a) (Node rn forest) = maybeNode = find fnd forst where fnd :: ReadRequest -> Bool - fnd (Node (_,(n,_,_,_)) _) = n == name + fnd (Node (_,(n,_,_,_,_)) _) = n == name mutateRequest :: ApiRequest -> TableName -> [Text] -> [FieldName] -> Either Response MutateRequest mutateRequest apiRequest tName pkCols fldNames = mapLeft apiRequestError $ @@ -340,7 +352,7 @@ fieldNames (Node (sel, _) forest) = map (fst . view _1) (select sel) ++ map colName fks where fks = concatMap (fromMaybe [] . f) forest - f (Node (_, (_, Just Relation{relFColumns=cols, relType=Parent}, _, _)) _) = Just cols + f (Node (_, (_, Just Relation{relFColumns=cols, relType=Parent}, _, _, _)) _) = Just cols f _ = Nothing -- Traditional filters(e.g. id=eq.1) are added as root nodes of the LogicTree diff --git a/src/PostgREST/QueryBuilder.hs b/src/PostgREST/QueryBuilder.hs index 01551856d..ab69af052 100644 --- a/src/PostgREST/QueryBuilder.hs +++ b/src/PostgREST/QueryBuilder.hs @@ -1,4 +1,6 @@ {-# LANGUAGE FlexibleInstances #-} +{-# LANGUAGE FlexibleContexts #-} +{-# LANGUAGE DuplicateRecordFields #-} {-# OPTIONS_GHC -fno-warn-orphans #-} {-| Module : PostgREST.QueryBuilder @@ -206,7 +208,7 @@ pgFmtLit x = requestToCountQuery :: Schema -> DbRequest -> SqlQuery requestToCountQuery _ (DbMutate _) = undefined -requestToCountQuery schema (DbRead (Node (Select _ _ logicForest _ _ _, (mainTbl, _, _, _)) _)) = +requestToCountQuery schema (DbRead (Node (Select{where_=logicForest}, (mainTbl, _, _, _, _)) _)) = unwords [ "SELECT pg_catalog.count(*)", "FROM ", fromQi qi, @@ -216,15 +218,15 @@ requestToCountQuery schema (DbRead (Node (Select _ _ logicForest _ _ _, (mainTbl qi = removeSourceCTESchema schema mainTbl requestToQuery :: Schema -> Bool -> DbRequest -> SqlQuery -requestToQuery schema isParent (DbRead (Node (Select colSelects tbls logicForest joinConds_ ordts range, (nodeName, maybeRelation, _, _)) forest)) = +requestToQuery schema isParent (DbRead (Node (Select colSelects tbls logicForest joinConds_ ordts range, (nodeName, maybeRelation, _, _, level)) forest)) = query where mainTbl = fromMaybe nodeName (tableName . relTable <$> maybeRelation) - qi = removeSourceCTESchema schema mainTbl - toQi = removeSourceCTESchema schema + tableAlias tbl = tbl <> "_" <> show level + qi = QualifiedIdentifier "" $ tableAlias mainTbl query = unwords [ "SELECT " <> intercalate ", " (map (pgFmtSelectItem qi) colSelects ++ selects), - "FROM " <> intercalate ", " (map (fromQi . toQi) tbls), + "FROM " <> intercalate ", " (map (\t -> fromQi (removeSourceCTESchema schema t) <> " AS " <> pgFmtIdent (tableAlias t)) tbls), unwords joins, ("WHERE " <> intercalate " AND " (map (pgFmtLogicTree qi) logicForest ++ map pgFmtJoinCond joinConds_)) `emptyOnFalse` (null logicForest && null joinConds_), @@ -234,21 +236,21 @@ requestToQuery schema isParent (DbRead (Node (Select colSelects tbls logicForest (joins, selects) = foldr getQueryParts ([],[]) forest getQueryParts :: Tree ReadNode -> ([SqlFragment], [SqlFragment]) -> ([SqlFragment], [SqlFragment]) - getQueryParts (Node n@(_, (name, Just Relation{relType=Child,relTable=Table{tableName=table}}, alias, _)) forst) (j,s) = (j,sel:s) + getQueryParts (Node n@(_, (name, Just Relation{relType=Child,relTable=Table{tableName=table}}, alias, _, _)) forst) (j,s) = (j,sel:s) where sel = "COALESCE((" <> "SELECT json_agg(" <> pgFmtIdent table <> ".*) " <> "FROM (" <> subquery <> ") " <> pgFmtIdent table <> "), '[]') AS " <> pgFmtIdent (fromMaybe name alias) where subquery = requestToQuery schema False (DbRead (Node n forst)) - getQueryParts (Node n@(_, (name, Just Relation{relType=Parent,relTable=Table{tableName=table}}, alias, _)) forst) (j,s) = (joi:j,sel:s) + getQueryParts (Node n@(_, (name, Just Relation{relType=Parent,relTable=Table{tableName=table}}, alias, _, _)) forst) (j,s) = (joi:j,sel:s) where aliasOrName = fromMaybe name alias localTableName = pgFmtIdent $ table <> "_" <> aliasOrName sel = "row_to_json(" <> localTableName <> ".*) AS " <> pgFmtIdent aliasOrName joi = " LEFT JOIN LATERAL( " <> subquery <> " ) AS " <> localTableName <> " ON TRUE " where subquery = requestToQuery schema True (DbRead (Node n forst)) - getQueryParts (Node n@(_, (name, Just Relation{relType=Many,relTable=Table{tableName=table}}, alias, _)) forst) (j,s) = (j,sel:s) + getQueryParts (Node n@(_, (name, Just Relation{relType=Many,relTable=Table{tableName=table}}, alias, _, _)) forst) (j,s) = (j,sel:s) where sel = "COALESCE ((" <> "SELECT json_agg(" <> pgFmtIdent table <> ".*) " @@ -422,8 +424,10 @@ pgFmtFilter table (Filter fld (OpExpr hasNot oper)) = notOp <> " " <> case oper (find ((==) . toLower $ v) ["null","true","false"]) pgFmtJoinCond :: JoinCond -> SqlFragment -pgFmtJoinCond (JoinCond (qi, cName) (fQi, fcName)) = - pgFmtColumn (removeSourceCTESchema (qiSchema qi) (qiName qi)) cName <> " = " <> pgFmtColumn (removeSourceCTESchema (qiSchema fQi) (qiName fQi)) fcName +pgFmtJoinCond (JoinCond (qi, cName, lvl) (fQi, fcName, fLvl)) = + let qiAlias = QualifiedIdentifier "" (qiName qi <> "_" <> show lvl) + fQiAlias = QualifiedIdentifier "" (qiName fQi <> "_" <> show fLvl) in + pgFmtColumn qiAlias cName <> " = " <> pgFmtColumn fQiAlias fcName 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 efb063c8b..a24894ff9 100644 --- a/src/PostgREST/Types.hs +++ b/src/PostgREST/Types.hs @@ -274,13 +274,14 @@ 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 JoinCond = JoinCond (QualifiedIdentifier, FieldName) (QualifiedIdentifier, FieldName) deriving (Show, Eq) +data JoinCond = JoinCond (QualifiedIdentifier, FieldName, Level) (QualifiedIdentifier, FieldName, Level) deriving (Show, Eq) +type Level = Integer data ReadQuery = Select { select::[SelectItem], from::[TableName], where_::[LogicTree], joinConds::[JoinCond], order::[OrderTerm], range_::NonnegRange } deriving (Show, Eq) data MutateQuery = Insert { in_::TableName, insPkCols::[Text], qPayload::PayloadJSON, onConflict:: Maybe PreferResolution, where_::[LogicTree], returning::[FieldName] } | Delete { in_::TableName, where_::[LogicTree], returning::[FieldName] } | Update { in_::TableName, qPayload::PayloadJSON, where_::[LogicTree], returning::[FieldName] } deriving (Show, Eq) -type ReadNode = (ReadQuery, (NodeName, Maybe Relation, Maybe Alias, Maybe RelationDetail)) +type ReadNode = (ReadQuery, (NodeName, Maybe Relation, Maybe Alias, Maybe RelationDetail, Level)) type ReadRequest = Tree ReadNode type MutateRequest = MutateQuery data DbRequest = DbRead ReadRequest | DbMutate MutateRequest diff --git a/test/Feature/QuerySpec.hs b/test/Feature/QuerySpec.hs index 22207d8e1..72530b00f 100644 --- a/test/Feature/QuerySpec.hs +++ b/test/Feature/QuerySpec.hs @@ -427,6 +427,77 @@ spec = do it "can detect fk relations through views to tables in the public schema" $ get "/consumers_view?select=*,orders_view{*}" `shouldRespondWith` 200 + context "tables with self reference foreign keys" $ do + context "one self reference foreign key" $ do + it "embeds parents recursively" $ + get "/family_tree?id=in.(3,4)&select=id,parent(id,name,parent(*))" `shouldRespondWith` + [json|[ + { "id": "3", "parent": { "id": "1", "name": "Parental Unit", "parent": null } }, + { "id": "4", "parent": { "id": "2", "name": "Kid One", "parent": { "id": "1", "name": "Parental Unit", "parent": null } } } + ]|] + { matchHeaders = [matchContentTypeJson] } + + it "embeds childs recursively" $ + get "/family_tree?id=eq.1&select=id,name, childs:family_tree.parent(id,name,childs:family_tree.parent(id,name))" `shouldRespondWith` + [json|[{ + "id": "1", "name": "Parental Unit", "childs": [ + { "id": "2", "name": "Kid One", "childs": [ { "id": "4", "name": "Grandkid One" } ] }, + { "id": "3", "name": "Kid Two", "childs": [ { "id": "5", "name": "Grandkid Two" } ] } + ] + }]|] { matchHeaders = [matchContentTypeJson] } + + it "embeds parent and then embeds childs" $ + get "/family_tree?id=eq.2&select=id,name,parent(id,name,childs:family_tree.parent(id,name))" `shouldRespondWith` + [json|[{ + "id": "2", "name": "Kid One", "parent": { + "id": "1", "name": "Parental Unit", "childs": [ { "id": "2", "name": "Kid One" }, { "id": "3", "name": "Kid Two"} ] + } + }]|] { matchHeaders = [matchContentTypeJson] } + + context "two self reference foreign keys" $ do + it "embeds parents" $ + get "/organizations?select=id,name,referee(id,name),auditor(id,name)&id=eq.3" `shouldRespondWith` + [json|[{ + "id": 3, "name": "Acme", + "referee": { + "id": 1, + "name": "Referee Org" + }, + "auditor": { + "id": 2, + "name": "Auditor Org" + } + }]|] { matchHeaders = [matchContentTypeJson] } + + it "embeds childs" $ do + get "/organizations?select=id,name,refereeds:organizations.referee(id,name)&id=eq.1" `shouldRespondWith` + [json|[{ + "id": 1, "name": "Referee Org", + "refereeds": [ + { + "id": 3, + "name": "Acme" + }, + { + "id": 4, + "name": "Umbrella" + } + ] + }]|] { matchHeaders = [matchContentTypeJson] } + get "/organizations?select=id,name,auditees:organizations.auditor(id,name)&id=eq.2" `shouldRespondWith` + [json|[{ + "id": 2, "name": "Auditor Org", + "auditees": [ + { + "id": 3, + "name": "Acme" + }, + { + "id": 4, + "name": "Umbrella" + } + ] + }]|] { matchHeaders = [matchContentTypeJson] } describe "ordering response" $ do it "by a column asc" $ diff --git a/test/fixtures/data.sql b/test/fixtures/data.sql index 9c0d198e3..a26427ebc 100644 --- a/test/fixtures/data.sql +++ b/test/fixtures/data.sql @@ -346,3 +346,16 @@ INSERT INTO tiobe_pls VALUES ('Java', 1), ('C', 2), ('Python', 4); TRUNCATE TABLE only_pk CASCADE; INSERT INTO only_pk VALUES (1), (2); + +TRUNCATE TABLE family_tree CASCADE; +INSERT INTO family_tree VALUES ('1', 'Parental Unit', NULL); +INSERT INTO family_tree VALUES ('2', 'Kid One', '1'); +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 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); diff --git a/test/fixtures/privileges.sql b/test/fixtures/privileges.sql index 9bf5f02cf..12e89009c 100644 --- a/test/fixtures/privileges.sql +++ b/test/fixtures/privileges.sql @@ -66,6 +66,8 @@ GRANT ALL ON TABLE , employees , tiobe_pls , only_pk + , family_tree + , organizations TO postgrest_test_anonymous; GRANT INSERT ON TABLE insertonly TO postgrest_test_anonymous; diff --git a/test/fixtures/schema.sql b/test/fixtures/schema.sql index ebb6b5345..b693c66ab 100755 --- a/test/fixtures/schema.sql +++ b/test/fixtures/schema.sql @@ -1374,3 +1374,19 @@ create table test.tiobe_pls( name text primary key, rank smallint ); + +create table test.family_tree ( + id text not null primary key, + name text not null, + parent text +); +alter table only test.family_tree add constraint pptr foreign key (parent) references test.family_tree(id); + +create table test.organizations ( + id integer primary key, + name text, + referee integer, + auditor integer +); +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);