diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cba0284c..025d5998b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,9 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #2070, Restrict generated many-to-many relationships - @steve-chavez + Only adds many-to-many relationships when: a table has FKs to two other tables and these FK columns are part of the table's PK columns. - #2278, Allow casting to types with underscores and numbers(e.g. `select=oid_array::_int4`) - @steve-chavez + - #2277, #2238, #1643, Prevent views from breaking one-to-many/many-to-one embeds when using column or FK as target - @steve-chavez + + When using a column or FK as target for embedding(`/tbl?select=*,col-or-fk(*)`), only tables are now detected and views are not. + + You can still use a column or an inferred FK on a view to embed a table(`/view?select=*,col-or-fk(*)`) ### Changed @@ -65,6 +68,9 @@ This project adheres to [Semantic Versioning](http://semver.org/). + This was misleading because the affected rows were not really affected by `max-rows`, only the returned rows were limited - #2070, Restrict generated many-to-many relationships - @steve-chavez + A primary key that contains the foreign key columns is now needed for generating many-to-many relationships. + - #2277, Views now are not detected when embedding using the column or FK as target (`/view?select=*,column(*)`) - @steve-chavez + + This embedding form was easily made ambiguous whenever a new view was added. + + For migrating, clients must be updated to the embedding form of `/view?select=*,other_view!column(*)`. ## [9.0.0] - 2021-11-25 diff --git a/src/PostgREST/DbStructure.hs b/src/PostgREST/DbStructure.hs index 1767b894c..e1a1949f2 100644 --- a/src/PostgREST/DbStructure.hs +++ b/src/PostgREST/DbStructure.hs @@ -149,7 +149,9 @@ decodeRels = (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)) + (M2O <$> column HD.text <*> compositeArrayColumn ((,) <$> compositeField HD.text <*> compositeField HD.text)) <*> + pure False <*> + pure False decodeViewKeyDeps :: HD.Result [ViewKeyDependency] decodeViewKeyDeps = @@ -344,6 +346,8 @@ addViewM2ORels keyDeps rels = relForeignTable False (M2O cons $ zipWith (\(_, vCol) (_, fCol)-> (vCol, fCol)) (keyDepCols vwTbl) relColumns) + True + False | vwTbl <- viewTableM2Os ] ++ [ Relationship @@ -351,6 +355,8 @@ addViewM2ORels keyDeps rels = (keyDepView tblVw) False (M2O cons $ zipWith (\(tCol, _) (_, vCol) -> (tCol, vCol)) relColumns (keyDepCols tblVw)) + False + True | tblVw <- tableViewM2Os ] ++ [ @@ -363,14 +369,16 @@ addViewM2ORels keyDeps rels = vw2 (vw1 == vw2) (M2O cons $ zipWith (\(_, vcol1) (_, vcol2) -> (vcol1, vcol2)) (keyDepCols vwTbl) (keyDepCols tblVw)) + True + True | vwTbl <- viewTableM2Os , tblVw <- tableViewM2Os ] viewRels _ = [] addO2MRels :: [Relationship] -> [Relationship] -addO2MRels rels = rels ++ [ Relationship ft t isSelf (O2M cons (swap <$> cols)) - | Relationship t ft isSelf (M2O cons cols) <- rels ] +addO2MRels rels = rels ++ [ Relationship ft t isSelf (O2M cons (swap <$> cols)) fTableIsView tableIsView + | Relationship t ft isSelf (M2O cons cols) tableIsView fTableIsView <- 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] @@ -379,10 +387,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 (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)) tblIsView fTblisView else Nothing - | Relationship jt1 t _ (M2O cons1 cols) <- rels - , Relationship jt2 ft _ (M2O cons2 fcols) <- rels + | Relationship jt1 t _ (M2O cons1 cols) _ tblIsView <- rels + , Relationship jt2 ft _ (M2O cons2 fcols) _ fTblisView <- rels , jt1 == jt2 , cons1 /= cons2] diff --git a/src/PostgREST/DbStructure/Relationship.hs b/src/PostgREST/DbStructure/Relationship.hs index aa8c7ec3b..0a50c9741 100644 --- a/src/PostgREST/DbStructure/Relationship.hs +++ b/src/PostgREST/DbStructure/Relationship.hs @@ -23,6 +23,8 @@ data Relationship = Relationship , relForeignTable :: QualifiedIdentifier , relIsSelf :: Bool -- ^ Whether is a self relationship , relCardinality :: Cardinality + , relTableIsView :: Bool + , relFTableIsView :: Bool } deriving (Eq, Ord, Generic, JSON.ToJSON) diff --git a/src/PostgREST/Request/DbRequestBuilder.hs b/src/PostgREST/Request/DbRequestBuilder.hs index 818b61204..ab7fe5d28 100644 --- a/src/PostgREST/Request/DbRequestBuilder.hs +++ b/src/PostgREST/Request/DbRequestBuilder.hs @@ -133,12 +133,7 @@ addRels schema allRels parentNode (Node (query@Select{from=tbl}, (nodeName, _, a -- /origin?select=target(*) If more than one relationship is found then the -- request is ambiguous and we return an error. In that case the request can -- be disambiguated by adding precision to the target or by using a hint: --- /origin?select=target!hint(*) The elements will be matched according to --- these rules: --- origin = table / view --- target = table / view / constraint / column-from-origin --- hint = table / view / constraint / column-from-origin / column-from-target --- (hint can take table / view values to aid in finding the junction in an m2m relationship) +-- /origin?select=target!hint(*). The origin can be a table or view. findRel :: Schema -> RelationshipsMap -> NodeName -> NodeName -> Maybe Hint -> Either ApiRequestError Relationship findRel schema allRels origin target hint = case rels of @@ -150,7 +145,7 @@ findRel schema allRels origin target hint = O2M _ [(col, _)] -> hint_ == col M2O _ [(col, _)] -> hint_ == col _ -> False - matchFKRefSingleCol hint_ card = case card of + matchFKRefSingleCol hint_ card = case card of O2M _ [(_, fCol)] -> hint_ == fCol M2O _ [(_, fCol)] -> hint_ == fCol _ -> False @@ -161,50 +156,52 @@ findRel schema allRels origin target hint = matchJunction hint_ card = case card of M2M Junction{junTable} -> hint_ == qiName junTable _ -> False - -- 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 + isM2O card = case card of + M2O _ _ -> True + _ -> False + isO2M card = case card of + O2M _ _ -> True + _ -> False rels = filter ( \Relationship{..} -> - case hint of + -- In a self-relationship we have a single foreign key but two relationships with different cardinalities: M2O/O2M. For disambiguation, we use the convention of getting: + -- TODO: handle one-to-one and many-to-many self-relationships + if relIsSelf + then case hint of + Nothing -> + -- The O2M by using the table name in the target + target == qiName relForeignTable && isO2M relCardinality -- /family_tree?select=children:family_tree(*) + || + -- The M2O by using the column name in the target + matchFKSingleCol target relCardinality && isM2O relCardinality -- /family_tree?select=parent(*) + Just hnt -> + -- /organizations?select=auditees:organizations!auditor(*) + target == qiName relForeignTable && isO2M relCardinality + && matchFKRefSingleCol hnt relCardinality -- auditor + else case hint of + -- target = table / view / constraint / column-from-origin (constraint/column-from-origin can only come from tables https://github.com/PostgREST/postgrest/issues/2277) + -- hint = table / view / constraint / column-from-origin / column-from-target (hint can take table / view values to aid in finding the junction in an m2m relationship) Nothing -> -- /projects?select=clients(*) target == qiName relForeignTable -- clients - && notM2OSelfRel relCardinality relIsSelf || -- /projects?select=projects_client_id_fkey(*) matchConstraint target relCardinality -- projects_client_id_fkey + && not relFTableIsView || -- /projects?select=client_id(*) matchFKSingleCol target relCardinality -- client_id - && notO2MSelfRel relCardinality relIsSelf + && not relFTableIsView Just hnt -> - ( - -- /projects?select=clients(*) - target == qiName relForeignTable && notM2OSelfRel relCardinality relIsSelf -- clients - || - -- /projects?select=projects_client_id_fkey(*) - matchConstraint target relCardinality -- projects_client_id_fkey - || - -- /projects?select=client_id(*) - matchFKSingleCol target relCardinality -- client_id - ) && ( + -- /projects?select=clients(*) + target == qiName relForeignTable -- clients + && ( -- /projects?select=clients!projects_client_id_fkey(*) matchConstraint hnt relCardinality || -- projects_client_id_fkey -- /projects?select=clients!client_id(*) or /projects?select=clients!id(*) - matchFKSingleCol hnt relCardinality || -- client_id - matchFKRefSingleCol hnt relCardinality || -- id + matchFKSingleCol hnt relCardinality || -- client_id + matchFKRefSingleCol hnt relCardinality || -- id -- /users?select=tasks!users_tasks(*) many-to-many between users and tasks matchJunction hnt relCardinality -- users_tasks @@ -235,7 +232,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 a986637ac..62f9bbe2f 100644 --- a/test/spec/Feature/Query/EmbedDisambiguationSpec.hs +++ b/test/spec/Feature/Query/EmbedDisambiguationSpec.hs @@ -13,56 +13,6 @@ spec :: SpecWith ((), Application) spec = describe "resource embedding disambiguation" $ do context "ambiguous requests that give 300 Multiple Choices" $ do - it "errs when there's a table and view that point to the same fk" $ - get "/message?select=id,body,sender(name,sent)" `shouldRespondWith` - [json| - { - "details": [ - { - "cardinality": "many-to-one", - "relationship": "message_sender_fkey using message(sender) and person(id)", - "embedding": "message with person" - }, - { - "cardinality": "many-to-one", - "relationship": "message_sender_fkey using message(sender) and person_detail(id)", - "embedding": "message with person_detail" - } - ], - "hint": "Try changing 'sender' to one of the following: 'person!message_sender_fkey', 'person_detail!message_sender_fkey'. Find the desired relationship in the 'details' key.", - "message": "Could not embed because more than one relationship was found for 'message' and 'sender'", - "code": "PGRST201" - } - |] - { matchStatus = 300 - , matchHeaders = [matchContentTypeJson] - } - - it "errs when there's a table and view that point to the same fk (composite pk)" $ - get "/activities?select=fst_shift(*)" `shouldRespondWith` - [json| - { - "details": [ - { - "cardinality": "one-to-many", - "relationship": "fst_shift using activities(id, schedule_id) and unit_workdays(fst_shift_activity_id, fst_shift_schedule_id)", - "embedding": "activities with unit_workdays" - }, - { - "cardinality": "one-to-many", - "relationship": "fst_shift using activities(id, schedule_id) and unit_workdays_fst_shift(fst_shift_activity_id, fst_shift_schedule_id)", - "embedding": "activities with unit_workdays_fst_shift" - } - ], - "hint": "Try changing 'fst_shift' to one of the following: 'unit_workdays!fst_shift', 'unit_workdays_fst_shift!fst_shift'. Find the desired relationship in the 'details' key.", - "message": "Could not embed because more than one relationship was found for 'activities' and 'fst_shift'", - "code": "PGRST201" - } - |] - { matchStatus = 300 - , matchHeaders = [matchContentTypeJson] - } - it "errs when there are o2m and m2m cardinalities to the target table" $ get "/sites?select=*,big_projects(*)" `shouldRespondWith` [json| @@ -216,7 +166,7 @@ spec = { matchHeaders = [matchContentTypeJson] } it "can request two parents with fks" $ - get "/articleStars?select=createdAt,article(id),user(name)&limit=1" + get "/articleStars?select=createdAt,article:articles(id),user(name)&limit=1" `shouldRespondWith` [json|[{"createdAt":"2015-12-08T04:22:57.472738","article":{"id": 1},"user":{"name": "Angela Martin"}}]|] @@ -488,3 +438,58 @@ spec = "details": null}|] { matchStatus = 400 , matchHeaders = [matchContentTypeJson] } + + context "embedding with col as a target doesn't consider views" $ do + -- https://github.com/PostgREST/postgrest/issues/1643 + it "works with self reference both ways(m2o and o2m)" $ do + get "/test?select=id,parent_id,parent:parent_id(id)" `shouldRespondWith` + [json| [ + { "id": 1, "parent_id": null, "parent": null }, + { "id": 2, "parent_id": 1, "parent": { "id": 1 } } + ] |] + { matchHeaders = [matchContentTypeJson] } + get "/test?select=id,parent_id,childs:test(id)" `shouldRespondWith` + [json| [ + { "id": 1, "parent_id": null, "childs": [ { "id": 2 } ] }, + { "id": 2, "parent_id": 1, "childs": [] } + ] + |] + { matchHeaders = [matchContentTypeJson] } + + -- https://github.com/PostgREST/postgrest/issues/2238 + it "has to be explicit for a view embedding" $ do + get "/adaptation_notifications?select=id,status,series(*)" `shouldRespondWith` + [json| [] |] + { matchHeaders = [matchContentTypeJson] } + get "/adaptation_notifications?select=id,status,series_popularity(*)" `shouldRespondWith` + [json| [] |] + { matchHeaders = [matchContentTypeJson] } + + it "resolves when there's a table and view that point to the same col" $ + get "/message?select=id,body,sender(id,name)&id=eq.5" `shouldRespondWith` + [json| [ + { + "id": 5, + "body": "What's up Jake", + "sender": { + "id": 4, + "name": "Julie" + } + } + ] |] + { matchHeaders = [matchContentTypeJson] } + + it "resolves when there's a table and view that point to the same fk (composite pk)" $ + get "/activities?select=fst_shift(*)" `shouldRespondWith` + [json| [ + { + "fst_shift": [ + { "unit_id": 1, "day": "2019-12-02", "fst_shift_activity_id": 1, "fst_shift_schedule_id": 1, "snd_shift_activity_id": 2, "snd_shift_schedule_id": 3 } + ] + }, + { + "fst_shift": [] + } + ]|] + { matchHeaders = [matchContentTypeJson] } + diff --git a/test/spec/fixtures/data.sql b/test/spec/fixtures/data.sql index d4893f112..f5cc0e6b0 100644 --- a/test/spec/fixtures/data.sql +++ b/test/spec/fixtures/data.sql @@ -775,3 +775,6 @@ INSERT INTO oid_test(id, oid_col, oid_array_col) VALUES (1, '12345', '{1,2,3,4,5 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); + +TRUNCATE TABLE test.test CASCADE; +INSERT INTO test.test (id, parent_id) VALUES (1, null), (2, 1); diff --git a/test/spec/fixtures/privileges.sql b/test/spec/fixtures/privileges.sql index 30e1b2e4d..d87ec5233 100644 --- a/test/spec/fixtures/privileges.sql +++ b/test/spec/fixtures/privileges.sql @@ -181,6 +181,11 @@ GRANT ALL ON TABLE , xmltest , oid_test , job + , series + , adaptation_notifications + , series_popularity + , test + , view_test 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 981df0322..dcc334e8b 100644 --- a/test/spec/fixtures/schema.sql +++ b/test/spec/fixtures/schema.sql @@ -2605,3 +2605,28 @@ CREATE TABLE private.internal_job CREATE VIEW test.job AS SELECT j.id, j.parent_id FROM private.internal_job j; + +-- https://github.com/PostgREST/postgrest/issues/2238 +CREATE TABLE series ( + id bigint PRIMARY KEY, + title text NOT NULL +); + +CREATE TABLE adaptation_notifications ( + id bigint PRIMARY KEY, + series bigint REFERENCES series(id), + status text +); + +CREATE VIEW series_popularity AS +SELECT id, random() AS popularity_score +FROM series; + +-- https://github.com/PostgREST/postgrest/issues/1643 +CREATE TABLE test.test ( + id BIGINT NOT NULL PRIMARY KEY, + parent_id BIGINT CONSTRAINT parent_test REFERENCES test(id) +); + +CREATE OR REPLACE VIEW test.view_test AS + SELECT id FROM test.test;