From 2be63b36d6e051611f800ba29577346267851a74 Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Sun, 8 May 2022 19:41:38 -0500 Subject: [PATCH] refactor: self relationship * Add test for self relationship in view --- src/PostgREST/DbStructure.hs | 26 ++++++++---- src/PostgREST/DbStructure/Relationship.hs | 5 +-- src/PostgREST/Request/DbRequestBuilder.hs | 41 +++++++++++-------- .../Feature/Query/EmbedDisambiguationSpec.hs | 15 +++++++ test/spec/fixtures/data.sql | 4 ++ test/spec/fixtures/privileges.sql | 1 + test/spec/fixtures/schema.sql | 12 ++++++ 7 files changed, 74 insertions(+), 30 deletions(-) diff --git a/src/PostgREST/DbStructure.hs b/src/PostgREST/DbStructure.hs index 98a2c43a0..1767b894c 100644 --- a/src/PostgREST/DbStructure.hs +++ b/src/PostgREST/DbStructure.hs @@ -148,6 +148,7 @@ decodeRels = Relationship <$> (QualifiedIdentifier <$> column HD.text <*> column HD.text) <*> (QualifiedIdentifier <$> column HD.text <*> column HD.text) <*> + column HD.bool <*> (M2O <$> column HD.text <*> compositeArrayColumn ((,) <$> compositeField HD.text <*> compositeField HD.text)) decodeViewKeyDeps :: HD.Result [ViewKeyDependency] @@ -341,18 +342,26 @@ addViewM2ORels keyDeps rels = [ Relationship (keyDepView vwTbl) relForeignTable + False (M2O cons $ zipWith (\(_, vCol) (_, fCol)-> (vCol, fCol)) (keyDepCols vwTbl) relColumns) | vwTbl <- viewTableM2Os ] ++ [ Relationship relTable (keyDepView tblVw) + False (M2O cons $ zipWith (\(tCol, _) (_, vCol) -> (tCol, vCol)) relColumns (keyDepCols tblVw)) | tblVw <- tableViewM2Os ] ++ - [ Relationship - (keyDepView vwTbl) - (keyDepView tblVw) + [ + let + vw1 = keyDepView vwTbl + vw2 = keyDepView tblVw + in + Relationship + vw1 + vw2 + (vw1 == vw2) (M2O cons $ zipWith (\(_, vcol1) (_, vcol2) -> (vcol1, vcol2)) (keyDepCols vwTbl) (keyDepCols tblVw)) | vwTbl <- viewTableM2Os , tblVw <- tableViewM2Os ] @@ -360,8 +369,8 @@ addViewM2ORels keyDeps rels = addO2MRels :: [Relationship] -> [Relationship] -addO2MRels rels = rels ++ [ Relationship ft t (O2M cons (swap <$> cols)) - | Relationship t ft (M2O cons cols) <- rels ] +addO2MRels rels = rels ++ [ Relationship ft t isSelf (O2M cons (swap <$> cols)) + | Relationship t ft isSelf (M2O cons cols) <- rels ] -- | Adds a m2m relationship if a table has FKs to two other tables and the FK columns are part of the PK columns addM2MRels :: TablesMap -> [Relationship] -> [Relationship] @@ -370,10 +379,10 @@ addM2MRels tbls rels = rels ++ catMaybes jtCols = S.fromList $ (fst <$> cols) ++ (fst <$> fcols) pkCols = S.fromList $ maybe mempty tablePKCols $ M.lookup jt1 tbls in if S.isSubsetOf jtCols pkCols - then Just $ Relationship t ft (M2M $ Junction jt1 cons1 cons2 (swap <$> cols) (swap <$> fcols)) + then Just $ Relationship t ft (t == ft) (M2M $ Junction jt1 cons1 cons2 (swap <$> cols) (swap <$> fcols)) else Nothing - | Relationship jt1 t (M2O cons1 cols) <- rels - , Relationship jt2 ft (M2O cons2 fcols) <- rels + | Relationship jt1 t _ (M2O cons1 cols) <- rels + , Relationship jt2 ft _ (M2O cons2 fcols) <- rels , jt1 == jt2 , cons1 /= cons2] @@ -607,6 +616,7 @@ allM2ORels pgVer = tab.relname AS table_name, ns2.nspname AS foreign_table_schema, other.relname AS foreign_table_name, + (ns1.nspname, tab.relname) = (ns2.nspname, other.relname) AS is_self, conname AS constraint_name, column_info.cols AS columns FROM pg_constraint, diff --git a/src/PostgREST/DbStructure/Relationship.hs b/src/PostgREST/DbStructure/Relationship.hs index 4a48b39be..aa8c7ec3b 100644 --- a/src/PostgREST/DbStructure/Relationship.hs +++ b/src/PostgREST/DbStructure/Relationship.hs @@ -5,7 +5,6 @@ module PostgREST.DbStructure.Relationship ( Cardinality(..) , Relationship(..) , Junction(..) - , isSelfReference , RelationshipsMap ) where @@ -22,6 +21,7 @@ import Protolude data Relationship = Relationship { relTable :: QualifiedIdentifier , relForeignTable :: QualifiedIdentifier + , relIsSelf :: Bool -- ^ Whether is a self relationship , relCardinality :: Cardinality } deriving (Eq, Ord, Generic, JSON.ToJSON) @@ -50,8 +50,5 @@ data Junction = Junction } deriving (Eq, Ord, Generic, JSON.ToJSON) -isSelfReference :: Relationship -> Bool -isSelfReference r = relTable r == relForeignTable r - -- | Key based on the source table and the foreign table schema type RelationshipsMap = M.HashMap (QualifiedIdentifier, Schema) [Relationship] diff --git a/src/PostgREST/Request/DbRequestBuilder.hs b/src/PostgREST/Request/DbRequestBuilder.hs index b65e09ddc..818b61204 100644 --- a/src/PostgREST/Request/DbRequestBuilder.hs +++ b/src/PostgREST/Request/DbRequestBuilder.hs @@ -51,8 +51,7 @@ import PostgREST.Request.ApiRequest (Action (..), import PostgREST.Request.Preferences import PostgREST.Request.Types -import qualified PostgREST.DbStructure.Relationship as Relationship -import qualified PostgREST.Request.QueryParams as QueryParams +import qualified PostgREST.Request.QueryParams as QueryParams import Protolude hiding (from) @@ -142,19 +141,10 @@ addRels schema allRels parentNode (Node (query@Select{from=tbl}, (nodeName, _, a -- (hint can take table / view values to aid in finding the junction in an m2m relationship) findRel :: Schema -> RelationshipsMap -> NodeName -> NodeName -> Maybe Hint -> Either ApiRequestError Relationship findRel schema allRels origin target hint = - case rel of + case rels of [] -> Left $ NoRelBetween origin target schema [r] -> Right r - -- Here we handle a self reference relationship to not cause a breaking - -- change: In a self reference we get two relationships with the same - -- foreign key and relTable/relFtable but with different - -- cardinalities(m2o/o2m) We output the O2M rel, the M2O rel can be - -- obtained by using the origin column as an embed hint. - rs@[rel0, rel1] -> case (relCardinality rel0, relCardinality rel1, relTable rel0 == relTable rel1 && relForeignTable rel0 == relForeignTable rel1) of - (O2M cons1 _, M2O cons2 _, True) -> if cons1 == cons2 then Right rel0 else Left $ AmbiguousRelBetween origin target rs - (M2O cons1 _, O2M cons2 _, True) -> if cons1 == cons2 then Right rel1 else Left $ AmbiguousRelBetween origin target rs - _ -> Left $ AmbiguousRelBetween origin target rs - rs -> Left $ AmbiguousRelBetween origin target rs + rs -> Left $ AmbiguousRelBetween origin target rs where matchFKSingleCol hint_ card = case card of O2M _ [(col, _)] -> hint_ == col @@ -171,22 +161,37 @@ findRel schema allRels origin target hint = matchJunction hint_ card = case card of M2M Junction{junTable} -> hint_ == qiName junTable _ -> False - rel = filter ( + -- In a self reference we get two relationships with the same + -- foreign key and relTable/relFtable but with different + -- cardinalities(M2O/O2M). We use the convention of getting: + -- + The O2M by using the table name in the target + -- + The M2O by using the column name in the target + -- For doing the above we ignore the M2O when using the table name in the target and + -- we ignore the O2M when using the column name in the target + notM2OSelfRel card isSelf = case card of + M2O _ _ -> not isSelf + _ -> True + notO2MSelfRel card isSelf = case card of + O2M _ _ -> not isSelf + _ -> True + rels = filter ( \Relationship{..} -> case hint of Nothing -> -- /projects?select=clients(*) - target == qiName relForeignTable -- clients + target == qiName relForeignTable -- clients + && notM2OSelfRel relCardinality relIsSelf || -- /projects?select=projects_client_id_fkey(*) matchConstraint target relCardinality -- projects_client_id_fkey || -- /projects?select=client_id(*) matchFKSingleCol target relCardinality -- client_id + && notO2MSelfRel relCardinality relIsSelf Just hnt -> ( -- /projects?select=clients(*) - target == qiName relForeignTable -- clients + target == qiName relForeignTable && notM2OSelfRel relCardinality relIsSelf -- clients || -- /projects?select=projects_client_id_fkey(*) matchConstraint target relCardinality -- projects_client_id_fkey @@ -218,7 +223,7 @@ addJoinConditions previousAlias (Node node@(query@Select{from=tbl,fromAlias=tblA where newAlias = if depth == 0 then tblAlias -- only use the alias on the root node(depth 0) for when the sourceCTEName alias is used for joining - else case Relationship.isSelfReference <$> rel of -- no need to apply the self reference alias on depth 0 only on the next depths + else case relIsSelf <$> rel of -- no need to apply the self reference alias on depth 0 only on the next depths Just True -> Just (qiName tbl <> "_" <> show depth) _ -> Nothing augmentQuery r = @@ -230,7 +235,7 @@ addJoinConditions previousAlias (Node node@(query@Select{from=tbl,fromAlias=tblA -- previousAlias and newAlias are used in the case of self joins getJoinConditions :: Maybe Alias -> Maybe Alias -> Relationship -> [JoinCondition] -getJoinConditions previousAlias newAlias (Relationship QualifiedIdentifier{qiSchema=tSchema, qiName=tN} QualifiedIdentifier{qiName=ftN} card) = +getJoinConditions previousAlias newAlias (Relationship QualifiedIdentifier{qiSchema=tSchema, qiName=tN} QualifiedIdentifier{qiName=ftN} _ card) = case card of M2M (Junction QualifiedIdentifier{qiName=jtn} _ _ jcols1 jcols2) -> (toJoinCondition previousAlias newAlias tN jtn <$> jcols1) ++ (toJoinCondition Nothing Nothing ftN jtn <$> jcols2) diff --git a/test/spec/Feature/Query/EmbedDisambiguationSpec.hs b/test/spec/Feature/Query/EmbedDisambiguationSpec.hs index a1c132427..a986637ac 100644 --- a/test/spec/Feature/Query/EmbedDisambiguationSpec.hs +++ b/test/spec/Feature/Query/EmbedDisambiguationSpec.hs @@ -362,6 +362,21 @@ spec = } }]|] { matchHeaders = [matchContentTypeJson] } + it "embeds parent and then embeds children on a view" $ + get "/job?select=id,parent_id(*),children:job!parent_id(id,parent_id)" `shouldRespondWith` + [json|[ + { + "id": 1, + "parent_id": null, + "children": [ { "id": 2, "parent_id": 1 } ] + }, + { + "id": 2, + "parent_id": { "id": 1, "parent_id": null }, + "children": [] + } + ]|] { 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` diff --git a/test/spec/fixtures/data.sql b/test/spec/fixtures/data.sql index e68252e16..d4893f112 100644 --- a/test/spec/fixtures/data.sql +++ b/test/spec/fixtures/data.sql @@ -771,3 +771,7 @@ INSERT INTO test.xmltest VALUES TRUNCATE TABLE test.oid_test CASCADE; INSERT INTO oid_test(id, oid_col, oid_array_col) VALUES (1, '12345', '{1,2,3,4,5}'::oid[]); + +TRUNCATE TABLE private.internal_job CASCADE; +INSERT INTO private.internal_job (id, parent_id) VALUES (1, null); +INSERT INTO private.internal_job (id, parent_id) VALUES (2, 1); diff --git a/test/spec/fixtures/privileges.sql b/test/spec/fixtures/privileges.sql index d50223069..30e1b2e4d 100644 --- a/test/spec/fixtures/privileges.sql +++ b/test/spec/fixtures/privileges.sql @@ -180,6 +180,7 @@ GRANT ALL ON TABLE , limited_update_items_cpk_view , xmltest , oid_test + , job TO postgrest_test_anonymous; GRANT INSERT ON TABLE insertonly TO postgrest_test_anonymous; diff --git a/test/spec/fixtures/schema.sql b/test/spec/fixtures/schema.sql index cbc73b39f..981df0322 100644 --- a/test/spec/fixtures/schema.sql +++ b/test/spec/fixtures/schema.sql @@ -2593,3 +2593,15 @@ CREATE TABLE oid_test( id int, oid_col oid, oid_array_col oid[]); + +CREATE TABLE private.internal_job +( + id integer NOT NULL, + parent_id integer, + CONSTRAINT internal_job_pkey PRIMARY KEY (id), + CONSTRAINT parent_fk FOREIGN KEY (parent_id) REFERENCES private.internal_job (id) +); + +CREATE VIEW test.job AS +SELECT j.id, j.parent_id +FROM private.internal_job j;