Merge pull request #4080 from rakeshkky/issue-4035-check-computed-field

fix postgres query error when computed fields are included in returning (fix #4035)

CHANGELOG.md

@@ -104,3 +104,4 @@
 - option to reload remote schemas in 'reload_metadata' API (fix #3792, #4117)
 - console: disable selecting roles without permissions for bulk actions (close #4178) (#4195)
 - console: show remote schema / event trigger intro sections always (#4044)
+- server: fix postgres query error when computed fields included in mutation response (fix #4035)

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
 

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

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
+    mutationResultAlias = Iden $ snakeCaseQualObject qt <> "__mutation_result_alias"
+    allColumnsAlias = Iden $ snakeCaseQualObject qt <> "__all_columns_alias"
+
+    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
 
-    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
+              MOutSinglerowObject annFlds ->
+                let tabFrom = FromIden allColumnsAlias
+                    tabPerm = TablePerm annBoolExpTrue Nothing
+                in S.SESelect $ mkSQLSelect JASSingleObject $
+                   AnnSelG annFlds tabFrom tabPerm noTableArgs strfyNum
 
 
 checkRetCols

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

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
 

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

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
+            }
           }
         }
       }

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:

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;

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:

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;

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
+          }
+        }
+      }
+    }

server/tests-py/test_graphql_mutations.py

@@ -101,6 +101,9 @@ class TestGraphqlInsertPermission:
     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,6 +383,12 @@ class TestGraphqlUpdateJsonB:
 @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")
+
     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 +397,7 @@ class TestGraphqlUpdatePermissions:
 
     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")