fix: ignore views on col/fk as target

Also refactor self relationship findRel logic
2022-05-16 12:22:24 -05:00
parent 2be63b36d6
commit f7b173163c
8 changed files with 144 additions and 93 deletions
@@ -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

@@ -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]

@@ -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)

@@ -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)
@@ -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] }
+
@@ -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);
@@ -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;
@@ -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;