From 9128c69a805060e9d19ad46c0ebf11f183a42014 Mon Sep 17 00:00:00 2001
From: rakeshkky <12475069+rakeshkky@users.noreply.github.com>
Date: Tue, 10 Mar 2020 18:15:47 +0530
Subject: [PATCH 1/3] fix postgres query error when computed fields included in
 mutation response, fix #4035

---
 .../src-lib/Hasura/GraphQL/Resolve/Insert.hs  |  2 +-
 server/src-lib/Hasura/RQL/DML/Mutation.hs     |  9 +-
 server/src-lib/Hasura/RQL/DML/Returning.hs    | 86 +++++++++++++------
 server/src-lib/Hasura/RQL/GBoolExp.hs         |  2 +-
 server/src-lib/Hasura/RQL/Types/Table.hs      |  5 ++
 server/src-lib/Hasura/SQL/Types.hs            |  5 --
 ...r_user_role_insert_check_perm_success.yaml |  5 ++
 .../insert/permissions/schema_setup.yaml      | 21 ++++-
 .../insert/permissions/schema_teardown.yaml   |  1 +
 .../update/permissions/schema_setup.yaml      | 31 ++++++-
 .../update/permissions/schema_teardown.yaml   |  1 +
 .../permissions/user_update_author.yaml       | 36 ++++++++
 server/tests-py/test_graphql_mutations.py     |  5 +-
 13 files changed, 170 insertions(+), 39 deletions(-)
 create mode 100644 server/tests-py/queries/graphql_mutation/update/permissions/user_update_author.yaml

diff --git a/server/src-lib/Hasura/GraphQL/Resolve/Insert.hs b/server/src-lib/Hasura/GraphQL/Resolve/Insert.hs
index 34ae37c38b7c3..7b0d614ff4d34 100644
--- a/server/src-lib/Hasura/GraphQL/Resolve/Insert.hs
+++ b/server/src-lib/Hasura/GraphQL/Resolve/Insert.hs
@@ -457,7 +457,7 @@ insertMultipleObjects strfyNum role tn multiObjIns addCols mutOutput errP =
       let affRows = sum $ map fst insResps
           columnValues = catMaybes $ map snd insResps
       cteExp <- mkSelCTEFromColVals tn tableColInfos columnValues
-      let sql = toSQL $ RR.mkMutationOutputExp tn (Just affRows) cteExp mutOutput strfyNum
+      let sql = toSQL $ RR.mkMutationOutputExp tn tableColInfos (Just affRows) cteExp mutOutput strfyNum
       runIdentity . Q.getRow
                <$> Q.rawQE dmlTxErrorHandler (Q.fromBuilder sql) [] False
 
diff --git a/server/src-lib/Hasura/RQL/DML/Mutation.hs b/server/src-lib/Hasura/RQL/DML/Mutation.hs
index c201f7fcebd2d..9438880a09f2b 100644
--- a/server/src-lib/Hasura/RQL/DML/Mutation.hs
+++ b/server/src-lib/Hasura/RQL/DML/Mutation.hs
@@ -38,19 +38,19 @@ runMutation mut =
     hasNestedFld $ _mOutput mut
 
 mutateAndReturn :: Mutation -> Q.TxE QErr EncJSON
-mutateAndReturn (Mutation qt (cte, p) mutationOutput _ strfyNum) =
+mutateAndReturn (Mutation qt (cte, p) mutationOutput allCols strfyNum) =
   encJFromBS . runIdentity . Q.getRow
     <$> Q.rawQE dmlTxErrorHandler (Q.fromBuilder $ toSQL selWith)
         (toList p) True
   where
-    selWith = mkMutationOutputExp qt Nothing cte mutationOutput strfyNum
+    selWith = mkMutationOutputExp qt allCols Nothing cte mutationOutput strfyNum
 
 mutateAndSel :: Mutation -> Q.TxE QErr EncJSON
 mutateAndSel (Mutation qt q mutationOutput allCols strfyNum) = do
   -- Perform mutation and fetch unique columns
   MutateResp _ columnVals <- mutateAndFetchCols qt allCols q strfyNum
   selCTE <- mkSelCTEFromColVals qt allCols columnVals
-  let selWith = mkMutationOutputExp qt Nothing selCTE mutationOutput strfyNum
+  let selWith = mkMutationOutputExp qt allCols Nothing selCTE mutationOutput strfyNum
   -- Perform select query and fetch returning fields
   encJFromBS . runIdentity . Q.getRow
     <$> Q.rawQE dmlTxErrorHandler (Q.fromBuilder $ toSQL selWith) [] True
@@ -108,8 +108,7 @@ mkSelCTEFromColVals qt allCols colVals =
   where
     rowAlias = Iden "row"
     extractor = S.selectStar' $ S.QualIden rowAlias $ Just $ S.TypeAnn $ toSQLTxt qt
-    sortedCols = flip sortBy allCols $ \lCol rCol ->
-                 compare (pgiPosition lCol) (pgiPosition rCol)
+    sortedCols = sortCols allCols
     mkTupsFromColVal colVal =
       fmap S.TupleExp $ forM sortedCols $ \ci -> do
         let pgCol = pgiColumn ci
diff --git a/server/src-lib/Hasura/RQL/DML/Returning.hs b/server/src-lib/Hasura/RQL/DML/Returning.hs
index a65d5e248a4ca..344fc79426a43 100644
--- a/server/src-lib/Hasura/RQL/DML/Returning.hs
+++ b/server/src-lib/Hasura/RQL/DML/Returning.hs
@@ -93,12 +93,8 @@ mkDefaultMutFlds = MOutMultirowFields . \case
   where
     mutFlds = [("affected_rows", MCount)]
 
-qualTableToAliasIden :: QualifiedTable -> Iden
-qualTableToAliasIden qt =
-  Iden $ snakeCaseTable qt <> "__mutation_result_alias"
-
-mkMutFldExp :: QualifiedTable -> Maybe Int -> Bool -> MutFld -> S.SQLExp
-mkMutFldExp qt preCalAffRows strfyNum = \case
+mkMutFldExp :: Iden -> Maybe Int -> Bool -> MutFld -> S.SQLExp
+mkMutFldExp cteAlias preCalAffRows strfyNum = \case
   MCount ->
     let countExp = S.SESelect $
           S.mkSelect
@@ -112,28 +108,70 @@ mkMutFldExp qt preCalAffRows strfyNum = \case
         tabPerm = TablePerm annBoolExpTrue Nothing
     in S.SESelect $ mkSQLSelect JASMultipleRows $
        AnnSelG selFlds tabFrom tabPerm noTableArgs strfyNum
-  where
-    cteAlias = qualTableToAliasIden qt
 
+{- Note [Mutation output expression]
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+An example output expression for INSERT mutation:
+
+WITH "<table-name>__mutation_result_alias" AS (
+  INSERT INTO <table-name> (<insert-column>[..])
+  VALUES
+    (<insert-value-row>[..])
+    ON CONFLICT ON CONSTRAINT "<table-constraint-name>" DO NOTHING RETURNING *,
+    -- An extra column expression which performs the 'CHECK' validation
+    CASE
+      WHEN (<CHECK Condition>) THEN NULL
+      ELSE "hdb_catalog"."check_violation"('insert check constraint failed')
+    END
+),
+"<table-name>__all_columns_alias" AS (
+  -- Only extract columns from mutated rows. Columns sorted by ordinal position so that
+  -- resulted rows can be casted to table type.
+  SELECT (<table-column>[..])
+  FROM
+    "<table-name>__mutation_result_alias"
+)
+<SELECT statement to generate mutation response using '<table-name>__all_columns_alias' as FROM>
+-}
+
+-- | Generate mutation output expression with given mutation CTE statement.
+-- See Note [Mutation output expression].
 mkMutationOutputExp
-  :: QualifiedTable -> Maybe Int -> S.CTE -> MutationOutput -> Bool -> S.SelectWith
-mkMutationOutputExp qt preCalAffRows cte mutOutput strfyNum =
-  S.SelectWith [(S.Alias cteAlias, cte)] sel
+  :: QualifiedTable
+  -> [PGColumnInfo]
+  -> Maybe Int
+  -> S.CTE
+  -> MutationOutput
+  -> Bool
+  -> S.SelectWith
+mkMutationOutputExp qt allCols preCalAffRows cte mutOutput strfyNum =
+  S.SelectWith [ (S.Alias mutationResultAlias, cte)
+               , (S.Alias allColumnsAlias, allColumnsSelect)
+               ] sel
   where
-    cteAlias = qualTableToAliasIden qt
-    sel = S.mkSelect { S.selExtr = [S.Extractor extrExp Nothing] }
+    mutationResultAlias = Iden $ snakeCaseQualObject qt <> "__mutation_result_alias"
+    allColumnsAlias = Iden $ snakeCaseQualObject qt <> "__all_columns_alias"
 
-    extrExp = case mutOutput of
-      MOutMultirowFields mutFlds ->
-        let jsonBuildObjArgs = flip concatMap mutFlds $
-              \(FieldName k, mutFld) -> [S.SELit k, mkMutFldExp qt preCalAffRows strfyNum mutFld]
-        in S.SEFnApp "json_build_object" jsonBuildObjArgs Nothing
-
-      MOutSinglerowObject annFlds ->
-        let tabFrom = FromIden cteAlias
-            tabPerm = TablePerm annBoolExpTrue Nothing
-        in S.SESelect $ mkSQLSelect JASSingleObject $
-           AnnSelG annFlds tabFrom tabPerm noTableArgs strfyNum
+    allColumnsSelect = S.CTESelect $ S.mkSelect
+                       { S.selExtr = map S.mkExtr $ map pgiColumn $ sortCols allCols
+                       , S.selFrom = Just $ S.mkIdenFromExp mutationResultAlias
+                       }
+
+    sel = S.mkSelect { S.selExtr = [S.Extractor extrExp Nothing] }
+          where
+            extrExp = case mutOutput of
+              MOutMultirowFields mutFlds ->
+                let jsonBuildObjArgs = flip concatMap mutFlds $
+                      \(FieldName k, mutFld) -> [ S.SELit k
+                                                , mkMutFldExp allColumnsAlias preCalAffRows strfyNum mutFld
+                                                ]
+                in S.SEFnApp "json_build_object" jsonBuildObjArgs Nothing
+
+              MOutSinglerowObject annFlds ->
+                let tabFrom = FromIden allColumnsAlias
+                    tabPerm = TablePerm annBoolExpTrue Nothing
+                in S.SESelect $ mkSQLSelect JASSingleObject $
+                   AnnSelG annFlds tabFrom tabPerm noTableArgs strfyNum
 
 
 checkRetCols
diff --git a/server/src-lib/Hasura/RQL/GBoolExp.hs b/server/src-lib/Hasura/RQL/GBoolExp.hs
index 9eb515e4c86e2..7813d0f6798d6 100644
--- a/server/src-lib/Hasura/RQL/GBoolExp.hs
+++ b/server/src-lib/Hasura/RQL/GBoolExp.hs
@@ -337,7 +337,7 @@ convColRhs tableQual = \case
     curVarNum <- get
     put $ curVarNum + 1
     let newIden  = Iden $ "_be_" <> T.pack (show curVarNum) <> "_"
-                   <> snakeCaseTable relTN
+                   <> snakeCaseQualObject relTN
         newIdenQ = S.QualIden newIden Nothing
     annRelBoolExp <- convBoolRhs' newIdenQ nesAnn
     let backCompExp = foldr (S.BEBin S.AndOp) (S.BELit True) $
diff --git a/server/src-lib/Hasura/RQL/Types/Table.hs b/server/src-lib/Hasura/RQL/Types/Table.hs
index 2443f7fe5b48f..4208450ebb399 100644
--- a/server/src-lib/Hasura/RQL/Types/Table.hs
+++ b/server/src-lib/Hasura/RQL/Types/Table.hs
@@ -44,6 +44,7 @@ module Hasura.RQL.Types.Table
        , getFieldInfoM
        , getPGColumnInfoM
        , getCols
+       , sortCols
        , getRels
        , getComputedFieldInfos
 
@@ -193,6 +194,10 @@ fieldInfoGraphQLNames = \case
 getCols :: FieldInfoMap FieldInfo -> [PGColumnInfo]
 getCols = mapMaybe (^? _FIColumn) . M.elems
 
+-- | Sort columns based on their ordinal position
+sortCols :: [PGColumnInfo] -> [PGColumnInfo]
+sortCols = sortBy (\l r -> compare (pgiPosition l) (pgiPosition r))
+
 getRels :: FieldInfoMap FieldInfo -> [RelInfo]
 getRels = mapMaybe (^? _FIRelationship) . M.elems
 
diff --git a/server/src-lib/Hasura/SQL/Types.hs b/server/src-lib/Hasura/SQL/Types.hs
index b12479ed5a289..3769a5f6f052f 100644
--- a/server/src-lib/Hasura/SQL/Types.hs
+++ b/server/src-lib/Hasura/SQL/Types.hs
@@ -11,7 +11,6 @@ module Hasura.SQL.Types
   , isView
 
   , QualifiedTable
-  , snakeCaseTable
   , QualifiedFunction
 
   , PGDescription(..)
@@ -300,10 +299,6 @@ snakeCaseQualObject (QualifiedObject sn o)
 
 type QualifiedTable = QualifiedObject TableName
 
-snakeCaseTable :: QualifiedObject TableName -> T.Text
-snakeCaseTable (QualifiedObject sn tn) =
-  getSchemaTxt sn <> "_" <> getTableTxt tn
-
 type QualifiedFunction = QualifiedObject FunctionName
 
 newtype PGDescription
diff --git a/server/tests-py/queries/graphql_mutation/insert/permissions/author_user_role_insert_check_perm_success.yaml b/server/tests-py/queries/graphql_mutation/insert/permissions/author_user_role_insert_check_perm_success.yaml
index 548117e51b6ca..71d337d40c620 100644
--- a/server/tests-py/queries/graphql_mutation/insert/permissions/author_user_role_insert_check_perm_success.yaml
+++ b/server/tests-py/queries/graphql_mutation/insert/permissions/author_user_role_insert_check_perm_success.yaml
@@ -30,6 +30,11 @@ query:
             id
             name
             is_registered
+            get_articles(args: {search: "Article"}){
+              id
+              title
+              content
+            }
           }
         }
       }
diff --git a/server/tests-py/queries/graphql_mutation/insert/permissions/schema_setup.yaml b/server/tests-py/queries/graphql_mutation/insert/permissions/schema_setup.yaml
index 9d37fb9ca9248..4dfe8cc8ef82a 100644
--- a/server/tests-py/queries/graphql_mutation/insert/permissions/schema_setup.yaml
+++ b/server/tests-py/queries/graphql_mutation/insert/permissions/schema_setup.yaml
@@ -27,12 +27,31 @@ args:
         author_id INTEGER REFERENCES author(id),
         is_published BOOLEAN,
         published_on TIMESTAMP
-      )
+      );
+
+      CREATE FUNCTION fetch_articles(search text, author_row author)
+      RETURNS SETOF article AS $$
+        SELECT *
+        FROM article
+        WHERE
+          ( title ilike ('%' || search || '%')
+            OR content ilike ('%' || search || '%')
+          ) AND author_id = author_row.id
+      $$ LANGUAGE sql STABLE;
+
 - type: track_table
   args:
     schema: public
     name: article
 
+- type: add_computed_field
+  args:
+    table: author
+    name: get_articles
+    definition:
+      function: fetch_articles
+      table_argument: author_row
+
 #Create resident table
 - type: run_sql
   args:
diff --git a/server/tests-py/queries/graphql_mutation/insert/permissions/schema_teardown.yaml b/server/tests-py/queries/graphql_mutation/insert/permissions/schema_teardown.yaml
index 7ad8d51deef3f..2062062439efa 100644
--- a/server/tests-py/queries/graphql_mutation/insert/permissions/schema_teardown.yaml
+++ b/server/tests-py/queries/graphql_mutation/insert/permissions/schema_teardown.yaml
@@ -13,6 +13,7 @@ args:
     sql: |
       drop table address;
       drop table resident;
+      drop function fetch_articles(text,author);
       drop table article;
       drop table blog;
       drop table author;
diff --git a/server/tests-py/queries/graphql_mutation/update/permissions/schema_setup.yaml b/server/tests-py/queries/graphql_mutation/update/permissions/schema_setup.yaml
index cbc8aa9e0d4d8..6e94f513e2ec3 100644
--- a/server/tests-py/queries/graphql_mutation/update/permissions/schema_setup.yaml
+++ b/server/tests-py/queries/graphql_mutation/update/permissions/schema_setup.yaml
@@ -27,12 +27,30 @@ args:
         is_published BOOLEAN,
         published_on TIMESTAMP,
         PRIMARY KEY (id,version)
-      )
+      );
+
+      CREATE FUNCTION fetch_articles(search text, author_row author)
+      RETURNS SETOF article AS $$
+        SELECT *
+        FROM article
+        WHERE
+          ( title ilike ('%' || search || '%')
+            OR content ilike ('%' || search || '%')
+          ) AND author_id = author_row.id
+      $$ LANGUAGE sql STABLE;
+
 - type: track_table
   args:
     schema: public
     name: article
 
+- type: add_computed_field
+  args:
+    table: author
+    name: get_articles
+    definition:
+      function: fetch_articles
+      table_argument: author_row
 
 #Object relationship
 - type: create_object_relationship
@@ -77,6 +95,17 @@ args:
       filter:
         id: X-HASURA-USER-ID
 
+#Author update permission for user
+- type: create_update_permission
+  args:
+    table: author
+    role: user
+    permission:
+      columns:
+      - name
+      filter:
+        id: X-Hasura-User-Id
+
 #Article select permission for user
 - type: create_select_permission
   args:
diff --git a/server/tests-py/queries/graphql_mutation/update/permissions/schema_teardown.yaml b/server/tests-py/queries/graphql_mutation/update/permissions/schema_teardown.yaml
index 74729fc93c936..bfff52e16e7cf 100644
--- a/server/tests-py/queries/graphql_mutation/update/permissions/schema_teardown.yaml
+++ b/server/tests-py/queries/graphql_mutation/update/permissions/schema_teardown.yaml
@@ -4,6 +4,7 @@ args:
 - type: run_sql
   args:
     sql: |
+      DROP function fetch_articles(text,author);
       DROP table article;
       DROP table author;
       DROP table person;
diff --git a/server/tests-py/queries/graphql_mutation/update/permissions/user_update_author.yaml b/server/tests-py/queries/graphql_mutation/update/permissions/user_update_author.yaml
new file mode 100644
index 0000000000000..d9fd877e0bdaa
--- /dev/null
+++ b/server/tests-py/queries/graphql_mutation/update/permissions/user_update_author.yaml
@@ -0,0 +1,36 @@
+description: Update name of an author
+url: /v1/graphql
+status: 200
+headers:
+  X-Hasura-Role: user
+  X-Hasura-User-Id: '1'
+response:
+  data:
+    update_author:
+      affected_rows: 1
+      returning:
+      - id: 1
+        name: Author 1 updated
+        get_articles:
+        - id: 2
+          title: Article 1
+          content: Sample article 1, content version 1.0.1
+query:
+  query: |
+    mutation {
+      update_author(
+        _set: {name: "Author 1 updated"}
+        where: {id: {_eq: 1}}
+      ){
+        affected_rows
+        returning{
+          id
+          name
+          get_articles(args: {search: "1.0.1"}){
+            id
+            title
+            content
+          }
+        }
+      }
+    }
diff --git a/server/tests-py/test_graphql_mutations.py b/server/tests-py/test_graphql_mutations.py
index 34d3bc3d72142..5e1aeb0a92c3f 100644
--- a/server/tests-py/test_graphql_mutations.py
+++ b/server/tests-py/test_graphql_mutations.py
@@ -380,6 +380,9 @@ def dir(cls):
 @use_mutation_fixtures
 class TestGraphqlUpdatePermissions:
 
+    def test_user_update_author(self, hge_ctx, transport):
+        check_query_f(hge_ctx, self.dir() + "/user_update_author.yaml", transport)
+
     def test_user_can_update_unpublished_article(self, hge_ctx, transport):
         check_query_f(hge_ctx, self.dir() + "/user_can_update_unpublished_article.yaml")
 
@@ -388,7 +391,7 @@ def test_user_cannot_update_published_version_col(self, hge_ctx, transport):
 
     def test_user_cannot_update_another_users_article(self, hge_ctx, transport):
         check_query_f(hge_ctx, self.dir() + "/user_cannot_update_another_users_article.yaml")
-    
+
     def test_user_cannot_publish(self, hge_ctx, transport):
         check_query_f(hge_ctx, self.dir() + "/user_cannot_publish.yaml")
 

From 27d1401ee6853438b5c1c5b22e69ec5ffa1642ba Mon Sep 17 00:00:00 2001
From: rakeshkky <12475069+rakeshkky@users.noreply.github.com>
Date: Wed, 11 Mar 2020 11:02:58 +0530
Subject: [PATCH 2/3] update CHANGELOG.md

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c13dc096c8d01..bd8681a0adc81 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -64,3 +64,4 @@
 - fix casting citext column type (fix #2818) (#3861)
 - Add downgrade command (close #1156) (#3760)
 - persist mix files only when coverage is enabled (#3844)
+- fix postgres query error when computed fields included in mutation response (fix #4035)

From 8c2b5ad9d395f80f88270b4b63c34895f7c9e0fe Mon Sep 17 00:00:00 2001
From: rakeshkky <12475069+rakeshkky@users.noreply.github.com>
Date: Wed, 11 Mar 2020 14:45:02 +0530
Subject: [PATCH 3/3] mark skip server upgrade test for new tests

---
 server/tests-py/test_graphql_mutations.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/server/tests-py/test_graphql_mutations.py b/server/tests-py/test_graphql_mutations.py
index 5e1aeb0a92c3f..6f22f30583a4a 100644
--- a/server/tests-py/test_graphql_mutations.py
+++ b/server/tests-py/test_graphql_mutations.py
@@ -101,6 +101,9 @@ def test_user_err_unexpected_constraint(self, hge_ctx, transport):
     def test_role_has_no_permissions_err(self, hge_ctx, transport):
         check_query_f(hge_ctx, self.dir() + "/address_permission_error.yaml")
 
+    # This test captures a bug in the previous release
+    # Avoiding this test for server upgrades
+    @pytest.mark.skip_server_upgrade_test
     def test_author_user_role_insert_check_perm_success(self, hge_ctx, transport):
         check_query_f(hge_ctx, self.dir() + "/author_user_role_insert_check_perm_success.yaml")
 
@@ -380,8 +383,11 @@ def dir(cls):
 @use_mutation_fixtures
 class TestGraphqlUpdatePermissions:
 
+    # This test captures a bug in the previous release
+    # Avoiding this test for server upgrades
+    @pytest.mark.skip_server_upgrade_test
     def test_user_update_author(self, hge_ctx, transport):
-        check_query_f(hge_ctx, self.dir() + "/user_update_author.yaml", transport)
+        check_query_f(hge_ctx, self.dir() + "/user_update_author.yaml")
 
     def test_user_can_update_unpublished_article(self, hge_ctx, transport):
         check_query_f(hge_ctx, self.dir() + "/user_can_update_unpublished_article.yaml")