Fix #1180, embedding on views with composite pks

Also add CHANGELOG entries for previous fixes.

CHANGELOG.md

@@ -9,6 +9,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- #1182, Fix embedding on views with composite pks - @steve-chavez
+- #1180, Fix embedding on views with subselects in pg10 - @steve-chavez
+
 ## [5.1.0] - 2018-08-31
 
 ### Added

src/PostgREST/DbRequestBuilder.hs

@@ -225,12 +225,12 @@ addJoinConditions schema (Node node@(query, nodeProps@(_, relation, _, _, _)) fo
     addJoinCond jc rq@Select{joinConditions=jcs} = rq{joinConditions=jc:jcs}
 
 getJoinConditions :: Relation -> [JoinCondition]
-getJoinConditions (Relation Table{tableSchema=tSchema, tableName=tN} cols Table{tableName=ftN} fcs typ lt lc1 lc2) =
+getJoinConditions (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 fcs
+        zipWith (toJoinCondition tN ftN) cols fCols
      | typ == Many ->
         let ltN = fromMaybe "" (tableName <$> lt) in
-        zipWith (toJoinCondition tN ltN) cols (fromMaybe [] lc1) ++ zipWith (toJoinCondition ftN ltN) fcs (fromMaybe [] lc2)
+        zipWith (toJoinCondition tN ltN) cols (fromMaybe [] lc1) ++ zipWith (toJoinCondition ftN ltN) fCols (fromMaybe [] lc2)
      | typ == Root -> witness
   where
     toJoinCondition :: Text -> Text -> Column -> Column -> JoinCondition

src/PostgREST/DbStructure.hs

@@ -41,7 +41,7 @@ getDbStructure schema pgVer = do
   keys      <- H.statement () $ allPrimaryKeys tabs
   procs     <- H.statement schema allProcs
 
-  let rels = addManyToManyRelations . addParentRelations $ addViewRelations syns childRels
+  let rels = addManyToManyRelations . addParentRelations $ addViewChildRelations syns childRels
       cols' = addForeignKeys rels cols
       keys' = addViewPrimaryKeys syns keys
 
@@ -243,32 +243,32 @@ Having a Relation{relTable=t1, relColumns=[c1], relFTable=t2, relFColumns=[c2],
 
 t1.c1------t2.c2
 
-When only having a t1_view.c1 synonym, we need to add a View to Table Relation
+When only having a t1_view.c1 synonym, we need to add a View to Table Child Relation
 
          t1.c1----t2.c2         t1.c1----------t2.c2
-                         ->            --------/
+                         ->            ________/
                                       /
       t1_view.c1             t1_view.c1
 
 
-When only having a t2_view.c2 synonym, we need to add a Table to View Relation
+When only having a t2_view.c2 synonym, we need to add a Table to View Child Relation
 
          t1.c1----t2.c2               t1.c1----------t2.c2
-                               ->          \--------
+                               ->          \________
                                                     \
                     t2_view.c2                      t2_view.c1
 
-When having t1_view.c1 and a t2_view.c2 synonyms, we need to add a View to View Relation in addition to the prior
+When having t1_view.c1 and a t2_view.c2 synonyms, we need to add a View to View Child Relation in addition to the prior
 
          t1.c1----t2.c2               t1.c1----------t2.c2
-                               ->          \--------/
+                               ->          \________/
                                            /        \
     t1_view.c1     t2_view.c2     t1_view.c1-------t2_view.c1
 
 The logic for composite pks is similar just need to make sure all the Relation columns have synonyms.
 -}
-addViewRelations :: [Synonym] -> [Relation] -> [Relation]
+addViewChildRelations :: [Synonym] -> [Relation] -> [Relation]
-addViewRelations allSyns = concatMap (\rel ->
+addViewChildRelations allSyns = concatMap (\rel ->
   rel : case rel of
     Relation{relType=Child, relTable, relColumns, relFTable, relFColumns} ->
 
@@ -279,18 +279,22 @@ addViewRelations allSyns = concatMap (\rel ->
           fColsSyns = colSynsGroupedByView relFColumns
           getView :: [Synonym] -> Table
           getView = colTable . snd . unsafeHead
-          syns `allSynsOf` cols = S.fromList (fst <$> syns) == S.fromList cols in
+          syns `allSynsOf` cols = S.fromList (fst <$> syns) == S.fromList cols
+          -- Relation is dependent on the order of relColumns and relFColumns to get the join conditions right in the generated query.
+          -- So we need to change the order of the synonyms to match the relColumns
+          -- This could be avoided if the Relation type is improved with a structure that maintains the association of relColumns and relFColumns
+          syns `sortAccordingTo` columns = sortOn (\(k, _) -> L.lookup k $ zip columns [0::Int ..]) syns in
 
-      -- View Table Relations
+      -- View Table Child Relations
-      [Relation (getView syns) (snd <$> syns) relFTable relFColumns Child Nothing Nothing Nothing
+      [Relation (getView syns) (snd <$> syns `sortAccordingTo` relColumns) relFTable relFColumns Child Nothing Nothing Nothing
         | syns <- colsSyns, syns `allSynsOf` relColumns] ++
 
-      -- Table View Relations
+      -- Table View Child Relations
-      [Relation relTable relColumns (getView fSyns) (snd <$> fSyns) Child Nothing Nothing Nothing
+      [Relation relTable relColumns (getView fSyns) (snd <$> fSyns `sortAccordingTo` relFColumns) Child Nothing Nothing Nothing
         | fSyns <- fColsSyns, fSyns `allSynsOf` relFColumns] ++
 
-      -- View View Relations
+      -- View View Child Relations
-      [Relation (getView syns) (snd <$> syns) (getView fSyns) (snd <$> fSyns) Child Nothing Nothing Nothing
+      [Relation (getView syns) (snd <$> syns `sortAccordingTo` relColumns) (getView fSyns) (snd <$> fSyns `sortAccordingTo` relFColumns) Child Nothing Nothing Nothing
         | syns <- colsSyns, fSyns <- fColsSyns, syns `allSynsOf` relColumns, fSyns `allSynsOf` relFColumns]
 
     _ -> [])

src/PostgREST/Types.hs

@@ -144,6 +144,9 @@ data RelationType = Child | Parent | Many | Root deriving (Show, Eq)
   The name 'Relation' here is used with the meaning
   "What is the relation between the current node and the parent node".
   It has nothing to do with PostgreSQL referring to tables/views as relations.
+  The order of the relColumns and relFColumns should be maintained to get
+  the join conditions right.
+  TODO merge relColumns and relFColumns to a tuple or Data.Bimap
 -}
 data Relation = Relation {
   relTable    :: Table

test/Feature/QuerySpec.hs

@@ -395,6 +395,36 @@ spec = do
       it "works when having a capitalized table name and camelCase fk column" $
         get "/foos?select=*,bars(*)" `shouldRespondWith` 200
 
+      it "works when embedding a view with a table that has a long compound pk" $ do
+        get "/player_view?select=id,contract(purchase_price)&id=in.(1,3,5,7)" `shouldRespondWith`
+          [json|
+            [{"id":1,"contract":[{"purchase_price":10}]},
+             {"id":3,"contract":[{"purchase_price":30}]},
+             {"id":5,"contract":[{"purchase_price":50}]},
+             {"id":7,"contract":[]}] |]
+          { matchHeaders = [matchContentTypeJson] }
+        get "/contract?select=tournament,player_view(first_name)&limit=3" `shouldRespondWith`
+          [json|
+            [{"tournament":"tournament_1","player_view":{"first_name":"first_name_1"}},
+             {"tournament":"tournament_2","player_view":{"first_name":"first_name_2"}},
+             {"tournament":"tournament_3","player_view":{"first_name":"first_name_3"}}] |]
+          { matchHeaders = [matchContentTypeJson] }
+
+      it "works when embedding a view with a view that referes to a table that has a long compound pk" $ do
+        get "/player_view?select=id,contract_view(purchase_price)&id=in.(1,3,5,7)" `shouldRespondWith`
+          [json|
+            [{"id":1,"contract_view":[{"purchase_price":10}]},
+             {"id":3,"contract_view":[{"purchase_price":30}]},
+             {"id":5,"contract_view":[{"purchase_price":50}]},
+             {"id":7,"contract_view":[]}] |]
+          { matchHeaders = [matchContentTypeJson] }
+        get "/contract_view?select=tournament,player_view(first_name)&limit=3" `shouldRespondWith`
+          [json|
+            [{"tournament":"tournament_1","player_view":{"first_name":"first_name_1"}},
+             {"tournament":"tournament_2","player_view":{"first_name":"first_name_2"}},
+             {"tournament":"tournament_3","player_view":{"first_name":"first_name_3"}}] |]
+          { matchHeaders = [matchContentTypeJson] }
+
     describe "path fixed" $ do
       it "works when requesting children 2 levels" $
         get "/clients?id=eq.1&select=id,projects:projects.client_id(id,tasks(id))" `shouldRespondWith`

test/fixtures/data.sql

@@ -442,3 +442,24 @@ INSERT INTO jsonb_test VALUES (1, '{ "a": {"b": 2} }');
 INSERT INTO jsonb_test VALUES (2, '{ "c": [1,2,3] }');
 INSERT INTO jsonb_test VALUES (3, '[{ "d": "test" }]');
 INSERT INTO jsonb_test VALUES (4, '{ "e": 1 }');
+
+TRUNCATE TABLE private.player CASCADE;
+INSERT into private.player
+SELECT
+  generate_series,
+  'first_name_' || generate_series,
+  'last_name_' || generate_series,
+  '2018-10-11'
+FROM generate_series(1, 12);
+
+TRUNCATE TABLE contract CASCADE;
+insert into contract
+select
+  'tournament_' || generate_series,
+  tsrange(now()::timestamp, null),
+  10*generate_series,
+  generate_series,
+  'first_name_' || generate_series,
+  'last_name_' || generate_series,
+  '2018-10-11'
+from generate_series(1, 6);

test/fixtures/privileges.sql

@@ -89,6 +89,9 @@ GRANT ALL ON TABLE
     , foos
     , bars
     , materialized_projects
+    , contract
+    , player_view
+    , contract_view
 TO postgrest_test_anonymous;
 
 GRANT INSERT ON TABLE insertonly TO postgrest_test_anonymous;

test/fixtures/schema.sql

@@ -1579,3 +1579,26 @@ create or replace function test."quotedFunction"("user" text, "fullName" text, "
 returns jsonb AS $$
   select format('{"user": "%s", "fullName": "%s", "SSN": "%s"}', "user", "fullName", "SSN")::jsonb;
 $$ language sql;
+
+create table private.player (
+  id integer not null,
+  first_name text not null,
+  last_name text not null,
+  birth_date date,
+  primary key (last_name, id, first_name, birth_date) -- just for testing a long compound pk
+);
+
+create table test.contract (
+  tournament text not null,
+  time tsrange not null,
+  purchase_price int not null,
+  id integer not null,
+  first_name text not null,
+  last_name text not null,
+  birth_date date,
+  foreign key (last_name, id, first_name, birth_date) references private.player
+);
+
+create view test.player_view as select * from private.player;
+
+create view test.contract_view as select * from test.contract;