From cb682e25399e6bf1e8f32cbafa63af87eeb08b53 Mon Sep 17 00:00:00 2001 From: Rakesh Emmadi <12475069+rakeshkky@users.noreply.github.com> Date: Wed, 4 Dec 2019 01:30:37 +0530 Subject: [PATCH] fix updating a column with multiple operators causing postgres query error (fix #3432) (#3458) --- server/src-lib/Data/Sequence/NonEmpty.hs | 4 + .../Hasura/GraphQL/Resolve/Mutation.hs | 88 ++++++++++++------- .../article_column_multiple_operators.yaml | 20 +++++ server/tests-py/test_graphql_mutations.py | 3 + 4 files changed, 81 insertions(+), 34 deletions(-) create mode 100644 server/tests-py/queries/graphql_mutation/update/basic/article_column_multiple_operators.yaml diff --git a/server/src-lib/Data/Sequence/NonEmpty.hs b/server/src-lib/Data/Sequence/NonEmpty.hs index 90be127f..6bc4548d 100644 --- a/server/src-lib/Data/Sequence/NonEmpty.hs +++ b/server/src-lib/Data/Sequence/NonEmpty.hs @@ -7,6 +7,7 @@ module Data.Sequence.NonEmpty , toSeq ) where +import qualified Data.Foldable as F import qualified Data.Sequence as Seq import Prelude (Eq, Show, fst, (.)) @@ -17,6 +18,9 @@ newtype NESeq a = NESeq { unNESeq :: (a, Seq.Seq a)} deriving (Show, Eq) +instance F.Foldable NESeq where + foldr f v = F.foldr f v . toSeq + init :: a -> NESeq a init a = NESeq (a, Seq.empty) diff --git a/server/src-lib/Hasura/GraphQL/Resolve/Mutation.hs b/server/src-lib/Hasura/GraphQL/Resolve/Mutation.hs index dd57f8c9..baa6549d 100644 --- a/server/src-lib/Hasura/GraphQL/Resolve/Mutation.hs +++ b/server/src-lib/Hasura/GraphQL/Resolve/Mutation.hs @@ -8,23 +8,27 @@ module Hasura.GraphQL.Resolve.Mutation import Data.Has import Hasura.Prelude -import qualified Data.Aeson as J -import qualified Data.HashMap.Strict as Map -import qualified Data.HashMap.Strict.InsOrd as OMap -import qualified Language.GraphQL.Draft.Syntax as G +import qualified Control.Monad.Validate as MV +import qualified Data.Aeson as J +import qualified Data.HashMap.Strict as Map +import qualified Data.HashMap.Strict.InsOrd as OMap +import qualified Data.HashMap.Strict.InsOrd.Extended as OMap +import qualified Data.Sequence.NonEmpty as NESeq +import qualified Data.Text as T +import qualified Language.GraphQL.Draft.Syntax as G -import qualified Hasura.RQL.DML.Delete as RD -import qualified Hasura.RQL.DML.Returning as RR -import qualified Hasura.RQL.DML.Update as RU +import qualified Hasura.RQL.DML.Delete as RD +import qualified Hasura.RQL.DML.Returning as RR +import qualified Hasura.RQL.DML.Update as RU -import qualified Hasura.RQL.DML.Select as RS -import qualified Hasura.SQL.DML as S +import qualified Hasura.RQL.DML.Select as RS +import qualified Hasura.SQL.DML as S import Hasura.EncJSON import Hasura.GraphQL.Resolve.BoolExp import Hasura.GraphQL.Resolve.Context import Hasura.GraphQL.Resolve.InputValue -import Hasura.GraphQL.Resolve.Select (fromSelSet) +import Hasura.GraphQL.Resolve.Select (fromSelSet) import Hasura.GraphQL.Validate.Field import Hasura.GraphQL.Validate.Types import Hasura.RQL.Types @@ -115,53 +119,69 @@ convertUpdateP1 -> m (RU.AnnUpdG UnresolvedVal) convertUpdateP1 opCtx fld = do -- a set expression is same as a row object - setExpM <- withArgM args "_set" $ convertRowObj colGNameMap + setExpM <- resolveUpdateOperator "_set" $ convertRowObj colGNameMap -- where bool expression to filter column whereExp <- withArg args "where" parseBoolExp -- increment operator on integer columns - incExpM <- withArgM args "_inc" $ + incExpM <- resolveUpdateOperator "_inc" $ convObjWithOp' $ rhsExpOp S.incOp S.intTypeAnn -- append jsonb value - appendExpM <- withArgM args "_append" $ + appendExpM <- resolveUpdateOperator "_append" $ convObjWithOp' $ rhsExpOp S.jsonbConcatOp S.jsonbTypeAnn -- prepend jsonb value - prependExpM <- withArgM args "_prepend" $ + prependExpM <- resolveUpdateOperator "_prepend" $ convObjWithOp' $ lhsExpOp S.jsonbConcatOp S.jsonbTypeAnn -- delete a key in jsonb object - deleteKeyExpM <- withArgM args "_delete_key" $ + deleteKeyExpM <- resolveUpdateOperator "_delete_key" $ convObjWithOp' $ rhsExpOp S.jsonbDeleteOp S.textTypeAnn -- delete an element in jsonb array - deleteElemExpM <- withArgM args "_delete_elem" $ + deleteElemExpM <- resolveUpdateOperator "_delete_elem" $ convObjWithOp' $ rhsExpOp S.jsonbDeleteOp S.intTypeAnn -- delete at path in jsonb value - deleteAtPathExpM <- withArgM args "_delete_at_path" $ convDeleteAtPathObj colGNameMap - mutFlds <- convertMutResp (_fType fld) $ _fSelSet fld + deleteAtPathExpM <- resolveUpdateOperator "_delete_at_path" $ + convDeleteAtPathObj colGNameMap - let resolvedPreSetItems = - Map.toList $ fmap partialSQLExpToUnresolvedVal preSetCols - - let updExpsM = [ setExpM, incExpM, appendExpM, prependExpM + updateItems <- combineUpdateExpressions + [ setExpM, incExpM, appendExpM, prependExpM , deleteKeyExpM, deleteElemExpM, deleteAtPathExpM ] - setItems = resolvedPreSetItems ++ concat (catMaybes updExpsM) - -- atleast one of update operators is expected - -- or preSetItems shouldn't be empty - -- this is not equivalent to (null setItems) - unless (any isJust updExpsM || not (null resolvedPreSetItems)) $ throwVE $ - "atleast any one of _set, _inc, _append, _prepend, " - <> "_delete_key, _delete_elem and " - <> "_delete_at_path operator is expected" + mutFlds <- convertMutResp (_fType fld) $ _fSelSet fld - let unresolvedPermFltr = fmapAnnBoolExp partialSQLExpToUnresolvedVal filterExp - - return $ RU.AnnUpd tn setItems - (unresolvedPermFltr, whereExp) mutFlds allCols + pure $ RU.AnnUpd tn updateItems (unresolvedPermFilter, whereExp) mutFlds allCols where convObjWithOp' = convObjWithOp colGNameMap allCols = Map.elems colGNameMap UpdOpCtx tn _ colGNameMap filterExp preSetCols = opCtx args = _fArguments fld + resolvedPreSetItems = Map.toList $ fmap partialSQLExpToUnresolvedVal preSetCols + unresolvedPermFilter = fmapAnnBoolExp partialSQLExpToUnresolvedVal filterExp + + resolveUpdateOperator operator resolveAction = + (operator,) <$> withArgM args operator resolveAction + + combineUpdateExpressions updateExps = do + let allOperatorNames = map fst updateExps + updateItems = mapMaybe (\(op, itemsM) -> (op,) <$> itemsM) updateExps + -- Atleast any one of operator is expected or preset expressions shouldn't be empty + if null updateItems && null resolvedPreSetItems then + throwVE $ "atleast any one of " <> showNames allOperatorNames <> " is expected" + else do + let itemsWithOps = concatMap (\(op, items) -> map (second (op,)) items) updateItems + validateMultiOps col items = do + when (length items > 1) $ MV.dispute [(col, map fst $ toList items)] + pure $ snd $ NESeq.head items + eitherResult = MV.runValidate $ OMap.traverseWithKey validateMultiOps $ + OMap.groupTuples itemsWithOps + case eitherResult of + -- A column shouldn't be present in more than one operator. + -- If present, then generated UPDATE statement throws unexpected query error + Left columnsWithMultiOps -> throwVE $ + "column found in multiple operators; " + <> T.intercalate ". " + (map (\(col, ops) -> col <<> " in " <> showNames ops) + columnsWithMultiOps) + Right items -> pure $ resolvedPreSetItems <> OMap.toList items convertUpdate :: ( MonadReusability m, MonadError QErr m diff --git a/server/tests-py/queries/graphql_mutation/update/basic/article_column_multiple_operators.yaml b/server/tests-py/queries/graphql_mutation/update/basic/article_column_multiple_operators.yaml new file mode 100644 index 00000000..46c0d252 --- /dev/null +++ b/server/tests-py/queries/graphql_mutation/update/basic/article_column_multiple_operators.yaml @@ -0,0 +1,20 @@ +description: Update an article with a column used in multiple operators +url: /v1/graphql +status: 200 +response: + errors: + - extensions: + path: "$" + code: validation-failed + message: column found in multiple operators; "id" in _set, _inc. "author_id" in _set, _inc +query: + query: | + mutation { + update_article( + _set: {id: 1, author_id: 1} + _inc: {id: 2, author_id: 2} + where: {author_id: {_eq: 1}} + ){ + affected_rows + } + } diff --git a/server/tests-py/test_graphql_mutations.py b/server/tests-py/test_graphql_mutations.py index 67ba951a..03e285fb 100644 --- a/server/tests-py/test_graphql_mutations.py +++ b/server/tests-py/test_graphql_mutations.py @@ -294,6 +294,9 @@ class TestGraphqlUpdateBasic(DefaultTestMutations): def test_no_operator_err(self, hge_ctx, transport): check_query_f(hge_ctx, self.dir() + "/person_error_no_operator.yaml") + def test_column_in_multiple_operators(self, hge_ctx, transport): + check_query_f(hge_ctx, self.dir() + "/article_column_multiple_operators.yaml") + @classmethod def dir(cls): return "queries/graphql_mutation/update/basic"