From 3b85b73af23fcfcb208f0a4107dc8bd153e8be76 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Wed, 8 Apr 2020 15:55:40 +0530 Subject: [PATCH 1/7] add a new field extensions in the ActionWebhookErrorResponse - the extensions can take any json --- server/src-lib/Hasura/GraphQL/Resolve/Action.hs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/server/src-lib/Hasura/GraphQL/Resolve/Action.hs b/server/src-lib/Hasura/GraphQL/Resolve/Action.hs index 6f782a179d9a0..4445ec6ea515a 100644 --- a/server/src-lib/Hasura/GraphQL/Resolve/Action.hs +++ b/server/src-lib/Hasura/GraphQL/Resolve/Action.hs @@ -64,8 +64,15 @@ data ActionWebhookErrorResponse = ActionWebhookErrorResponse { _awerMessage :: !Text , _awerCode :: !(Maybe Text) + , _awerExtensions :: !(Maybe J.Value) } deriving (Show, Eq) -$(J.deriveJSON (J.aesonDrop 5 J.snakeCase) ''ActionWebhookErrorResponse) +$(J.deriveFromJSON (J.aesonDrop 5 J.snakeCase) ''ActionWebhookErrorResponse) + +data ActionWebhookErrorExtension + = ActionWebhookErrorExtension + { _aweeExtension :: !J.Value + } +$(J.deriveJSON (J.aesonDrop 5 J.snakeCase) ''ActionWebhookErrorExtension) data ActionWebhookResponse = AWRArray ![J.Object] @@ -414,10 +421,11 @@ callWebhook manager outputType outputFields reqHeaders confHeaders pure (webhookResponse, mkSetCookieHeaders responseWreq) | HTTP.statusIsClientError responseStatus -> do - ActionWebhookErrorResponse message maybeCode <- + ActionWebhookErrorResponse message maybeCode maybeExtension <- modifyErr ("webhook response: " <>) $ decodeValue responseValue let code = maybe Unexpected ActionWebhookCode maybeCode - qErr = QErr [] responseStatus message code Nothing + qErr = QErr [] responseStatus message code + (maybe Nothing (Just . J.toJSON . ActionWebhookErrorExtension) maybeExtension) throwError qErr | otherwise -> From 628eff5c84794ca1d0d853bf5e6639568f30b3c5 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Thu, 9 Apr 2020 18:18:08 +0530 Subject: [PATCH 2/7] remove code from ActionWebhookErrorResponse --- server/src-lib/Hasura/GraphQL/Resolve/Action.hs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src-lib/Hasura/GraphQL/Resolve/Action.hs b/server/src-lib/Hasura/GraphQL/Resolve/Action.hs index 4445ec6ea515a..fefc0f576d9ce 100644 --- a/server/src-lib/Hasura/GraphQL/Resolve/Action.hs +++ b/server/src-lib/Hasura/GraphQL/Resolve/Action.hs @@ -63,7 +63,6 @@ $(J.deriveJSON (J.aesonDrop 4 J.snakeCase) ''ActionWebhookPayload) data ActionWebhookErrorResponse = ActionWebhookErrorResponse { _awerMessage :: !Text - , _awerCode :: !(Maybe Text) , _awerExtensions :: !(Maybe J.Value) } deriving (Show, Eq) $(J.deriveFromJSON (J.aesonDrop 5 J.snakeCase) ''ActionWebhookErrorResponse) @@ -421,10 +420,9 @@ callWebhook manager outputType outputFields reqHeaders confHeaders pure (webhookResponse, mkSetCookieHeaders responseWreq) | HTTP.statusIsClientError responseStatus -> do - ActionWebhookErrorResponse message maybeCode maybeExtension <- + ActionWebhookErrorResponse message maybeExtension <- modifyErr ("webhook response: " <>) $ decodeValue responseValue - let code = maybe Unexpected ActionWebhookCode maybeCode - qErr = QErr [] responseStatus message code + let qErr = QErr [] responseStatus message Unexpected $ (maybe Nothing (Just . J.toJSON . ActionWebhookErrorExtension) maybeExtension) throwError qErr From e6ffc0a6976cd2748b7408c37966dad7b25a7fe3 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Thu, 9 Apr 2020 18:18:33 +0530 Subject: [PATCH 3/7] update docs and CHANGELOG --- CHANGELOG.md | 1 + docs/graphql/manual/actions/action-handlers.rst | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b979e34a5a3be..41e8eec77c125 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,7 @@ Read more about check constraints on [Postgres Docs](https://www.postgresql.org/ - docs: add note on pg versions for actions (#4034) - docs: add latest prerelease build info (close #4041) (#4048) - docs: add AuthGuardian JWT guide (#3958) +- server: add ability to accept custom json (extensions) while error handling actions (#4001) ## `v1.2.0-beta.2` diff --git a/docs/graphql/manual/actions/action-handlers.rst b/docs/graphql/manual/actions/action-handlers.rst index 0a5588794a5e3..bd01f21422eb7 100644 --- a/docs/graphql/manual/actions/action-handlers.rst +++ b/docs/graphql/manual/actions/action-handlers.rst @@ -59,7 +59,7 @@ An error object looks like: { "message": "", - "code": "" + "extensions": "" } The HTTP status code must be ``4xx`` for an error response. From 44a4b16cc8bd04d513d27237c17eee7e0674d887 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Thu, 9 Apr 2020 21:45:05 +0530 Subject: [PATCH 4/7] add a new data type ErrorExtra - ErrorExtra is used to give the errors context while deriving to JSON --- server/src-lib/Hasura/Db.hs | 2 +- server/src-lib/Hasura/GraphQL/Execute.hs | 2 +- server/src-lib/Hasura/GraphQL/RemoteServer.hs | 2 +- .../src-lib/Hasura/GraphQL/Resolve/Action.hs | 10 +---- server/src-lib/Hasura/RQL/DDL/Schema.hs | 2 +- server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs | 2 +- .../RQL/DDL/Schema/Cache/Dependencies.hs | 2 +- server/src-lib/Hasura/RQL/Types/Error.hs | 42 ++++++++++++++----- .../Hasura/RQL/Types/SchemaCache/Build.hs | 8 ++-- server/src-lib/Hasura/Server/Migrate.hs | 34 +++++++-------- 10 files changed, 61 insertions(+), 45 deletions(-) diff --git a/server/src-lib/Hasura/Db.hs b/server/src-lib/Hasura/Db.hs index 0cebe460e3de9..e91d3ab18d658 100644 --- a/server/src-lib/Hasura/Db.hs +++ b/server/src-lib/Hasura/Db.hs @@ -111,7 +111,7 @@ defaultTxErrorHandler = mkTxErrorHandler (const False) mkTxErrorHandler :: (PGErrorType -> Bool) -> Q.PGTxErr -> QErr mkTxErrorHandler isExpectedError txe = fromMaybe unexpectedError expectedError where - unexpectedError = (internalError "postgres query error") { qeInternal = Just $ J.toJSON txe } + unexpectedError = (internalError "postgres query error") { qeExtra = Just $ EEInternal $ J.toJSON txe } expectedError = uncurry err400 <$> do errorDetail <- Q.getPGStmtErr txe message <- Q.edMessage errorDetail diff --git a/server/src-lib/Hasura/GraphQL/Execute.hs b/server/src-lib/Hasura/GraphQL/Execute.hs index d5c613e297837..704bf8e177d09 100644 --- a/server/src-lib/Hasura/GraphQL/Execute.hs +++ b/server/src-lib/Hasura/GraphQL/Execute.hs @@ -163,7 +163,7 @@ getExecPlanPartial userInfo sc enableAL req = do modErr e = let msg = "query is not in any of the allowlists" - in e{qeInternal = Just $ J.object [ "message" J..= J.String msg]} + in e{qeExtra = Just $ EEInternal $ J.object [ "message" J..= J.String msg]} -- An execution operation, in case of diff --git a/server/src-lib/Hasura/GraphQL/RemoteServer.hs b/server/src-lib/Hasura/GraphQL/RemoteServer.hs index d33ff34ecb9d5..7dec899a971ee 100644 --- a/server/src-lib/Hasura/GraphQL/RemoteServer.hs +++ b/server/src-lib/Hasura/GraphQL/RemoteServer.hs @@ -82,7 +82,7 @@ fetchRemoteSchema manager name def@(RemoteSchemaInfo url headerConf _ timeout) = throwWithInternal msg v = let err = err400 RemoteSchemaError $ T.pack msg - in throwError err{qeInternal = Just $ J.toJSON v} + in throwError err{qeExtra = Just $ EEInternal $ J.toJSON v} httpExceptMsg = "HTTP exception occurred while sending the request to " <> show url diff --git a/server/src-lib/Hasura/GraphQL/Resolve/Action.hs b/server/src-lib/Hasura/GraphQL/Resolve/Action.hs index fefc0f576d9ce..a83b1cd28ef84 100644 --- a/server/src-lib/Hasura/GraphQL/Resolve/Action.hs +++ b/server/src-lib/Hasura/GraphQL/Resolve/Action.hs @@ -67,12 +67,6 @@ data ActionWebhookErrorResponse } deriving (Show, Eq) $(J.deriveFromJSON (J.aesonDrop 5 J.snakeCase) ''ActionWebhookErrorResponse) -data ActionWebhookErrorExtension - = ActionWebhookErrorExtension - { _aweeExtension :: !J.Value - } -$(J.deriveJSON (J.aesonDrop 5 J.snakeCase) ''ActionWebhookErrorExtension) - data ActionWebhookResponse = AWRArray ![J.Object] | AWRObject !J.Object @@ -404,7 +398,7 @@ callWebhook manager outputType outputFields reqHeaders confHeaders if | HTTP.statusIsSuccessful responseStatus -> do let expectingArray = isListType outputType - addInternalToErr e = e{qeInternal = Just webhookResponseObject} + addInternalToErr e = e{qeExtra = Just $ EEInternal $ webhookResponseObject} -- Incase any error, add webhook response in internal modifyQErr addInternalToErr $ do webhookResponse <- decodeValue responseValue @@ -423,7 +417,7 @@ callWebhook manager outputType outputFields reqHeaders confHeaders ActionWebhookErrorResponse message maybeExtension <- modifyErr ("webhook response: " <>) $ decodeValue responseValue let qErr = QErr [] responseStatus message Unexpected $ - (maybe Nothing (Just . J.toJSON . ActionWebhookErrorExtension) maybeExtension) + (maybe Nothing (Just . EEExtensions) maybeExtension) throwError qErr | otherwise -> diff --git a/server/src-lib/Hasura/RQL/DDL/Schema.hs b/server/src-lib/Hasura/RQL/DDL/Schema.hs index 886dbcde882cf..99ed801e885d2 100644 --- a/server/src-lib/Hasura/RQL/DDL/Schema.hs +++ b/server/src-lib/Hasura/RQL/DDL/Schema.hs @@ -101,7 +101,7 @@ runRunSQL RunSQL {..} = do fmap (encJFromJValue @RunSQLRes) . liftTx . Q.multiQE rawSqlErrHandler . Q.fromText where rawSqlErrHandler txe = - (err400 PostgresError "query execution failed") { qeInternal = Just $ toJSON txe } + (err400 PostgresError "query execution failed") { qeExtra = Just $ EEInternal $ toJSON txe } -- see Note [Checking metadata consistency in run_sql] containsDDLKeyword :: Text -> Bool diff --git a/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs b/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs index 25329e7417535..8c36d5b8fcfd0 100644 --- a/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs +++ b/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs @@ -499,7 +499,7 @@ withMetadataCheck cascade action = do checkNewInconsistentMeta originalInconsMeta currentInconsMeta = unless (null newInconsistentObjects) $ throwError (err500 Unexpected "cannot continue due to newly found inconsistent metadata") - { qeInternal = Just $ toJSON newInconsistentObjects } + { qeExtra = Just $ EEInternal $ toJSON newInconsistentObjects } where diffInconsistentObjects = M.difference `on` groupInconsistentMetadataById newInconsistentObjects = nub $ concatMap toList $ diff --git a/server/src-lib/Hasura/RQL/DDL/Schema/Cache/Dependencies.hs b/server/src-lib/Hasura/RQL/DDL/Schema/Cache/Dependencies.hs index 5672014dd6caa..8cfcb885763c3 100644 --- a/server/src-lib/Hasura/RQL/DDL/Schema/Cache/Dependencies.hs +++ b/server/src-lib/Hasura/RQL/DDL/Schema/Cache/Dependencies.hs @@ -66,7 +66,7 @@ performIteration iterationNumber cache inconsistencies dependencies = do -- unless we did something very wrong, so halt the process and abort with some -- debugging information. throwError (err500 Unexpected "schema dependency resolution failed to terminate") - { qeInternal = Just $ object + { qeExtra = Just $ EEInternal $ object [ "inconsistent_objects" .= object [ "old" .= inconsistencies , "new" .= newInconsistencies ] diff --git a/server/src-lib/Hasura/RQL/Types/Error.hs b/server/src-lib/Hasura/RQL/Types/Error.hs index 863352d89450c..14e6b98245216 100644 --- a/server/src-lib/Hasura/RQL/Types/Error.hs +++ b/server/src-lib/Hasura/RQL/Types/Error.hs @@ -3,6 +3,7 @@ module Hasura.RQL.Types.Error ( Code(..) , QErr(..) + , ErrorExtra(..) , encodeQErr , encodeGQLErr , noInternalQErrEnc @@ -138,13 +139,31 @@ instance Show Code where InvalidCustomTypes -> "invalid-custom-types" ActionWebhookCode t -> T.unpack t +-- ErrorExtra is a data type used to give +-- additional information of the error +-- occurred in JSON format. +-- EEInternal is used when there's an error in the HGE +-- or Postgres +-- EEExtensions is used when the error has occurred outside +-- hasura and we get an custom JSON error object +-- Eg: Actions error handling of client side errors +data ErrorExtra + = EEInternal !Value + | EEExtensions !Value + deriving (Show, Eq) + +instance ToJSON ErrorExtra where + toJSON = \case + EEInternal v -> v + EEExtensions v -> v + data QErr = QErr { qePath :: !JSONPath , qeStatus :: !N.Status , qeError :: !T.Text , qeCode :: !Code - , qeInternal :: !(Maybe Value) + , qeExtra :: !(Maybe ErrorExtra) } deriving (Show, Eq) instance ToJSON QErr where @@ -154,12 +173,12 @@ instance ToJSON QErr where , "error" .= msg , "code" .= show code ] - toJSON (QErr jPath _ msg code (Just ie)) = + toJSON (QErr jPath _ msg code (Just err)) = object [ "path" .= encodeJSONPath jPath , "error" .= msg , "code" .= show code - , "internal" .= ie + , "internal" .= err ] noInternalQErrEnc :: QErr -> Value @@ -171,18 +190,21 @@ noInternalQErrEnc (QErr jPath _ msg code _) = ] encodeGQLErr :: Bool -> QErr -> Value -encodeGQLErr includeInternal (QErr jPath _ msg code mIE) = +encodeGQLErr includeInternal (QErr jPath _ msg code mExtra) = object [ "message" .= msg , "extensions" .= extnsObj ] where - extnsObj = object $ bool codeAndPath - (codeAndPath ++ internal) includeInternal + extnsObj = case mExtra of + Nothing -> object codeAndPath + Just (EEExtensions v) -> v + Just (EEInternal v) -> object $ + bool codeAndPath (codeAndPath ++ ["internal" .= v]) includeInternal + codeAndPath = [ "code" .= show code , "path" .= encodeJSONPath jPath ] - internal = maybe [] (\ie -> ["internal" .= ie]) mIE -- whether internal should be included or not encodeQErr :: Bool -> QErr -> Value @@ -203,12 +225,12 @@ encodeJSONPath = format "$" instance Q.FromPGConnErr QErr where fromPGConnErr c = let e = err500 PostgresError "connection error" - in e {qeInternal = Just $ toJSON c} + in e {qeExtra = Just $ EEInternal $ toJSON c} instance Q.FromPGTxErr QErr where fromPGTxErr txe = let e = err500 PostgresError "postgres tx error" - in e {qeInternal = Just $ toJSON txe} + in e {qeExtra = Just $ EEInternal $ toJSON txe} err400 :: Code -> T.Text -> QErr err400 c t = QErr [] N.status400 t c Nothing @@ -241,7 +263,7 @@ internalError = err500 Unexpected throw500WithDetail :: (QErrM m) => T.Text -> Value -> m a throw500WithDetail t detail = - throwError $ (err500 Unexpected t) {qeInternal = Just detail} + throwError $ (err500 Unexpected t) {qeExtra = Just $ EEInternal detail} modifyQErr :: (QErrM m) => (QErr -> QErr) -> m a -> m a diff --git a/server/src-lib/Hasura/RQL/Types/SchemaCache/Build.hs b/server/src-lib/Hasura/RQL/Types/SchemaCache/Build.hs index c597db3efc174..65586c9be0a99 100644 --- a/server/src-lib/Hasura/RQL/Types/SchemaCache/Build.hs +++ b/server/src-lib/Hasura/RQL/Types/SchemaCache/Build.hs @@ -146,11 +146,11 @@ buildSchemaCacheFor objectId = do for_ (M.lookup objectId newInconsistentObjects) $ \matchingObjects -> do let reasons = T.intercalate ", " $ map imReason $ toList matchingObjects - throwError (err400 ConstraintViolation reasons) { qeInternal = Just $ toJSON matchingObjects } + throwError (err400 ConstraintViolation reasons) { qeExtra = Just $ EEInternal $ toJSON matchingObjects } unless (null newInconsistentObjects) $ throwError (err400 Unexpected "cannot continue due to new inconsistent metadata") - { qeInternal = Just $ toJSON (nub . concatMap toList $ M.elems newInconsistentObjects) } + { qeExtra = Just $ EEInternal $ toJSON (nub . concatMap toList $ M.elems newInconsistentObjects) } -- | Like 'buildSchemaCache', but fails if there is any inconsistent metadata. buildSchemaCacheStrict :: (QErrM m, CacheRWM m) => m () @@ -160,7 +160,7 @@ buildSchemaCacheStrict = do let inconsObjs = scInconsistentObjs sc unless (null inconsObjs) $ do let err = err400 Unexpected "cannot continue due to inconsistent metadata" - throwError err{ qeInternal = Just $ toJSON inconsObjs } + throwError err{ qeExtra = Just $ EEInternal $ toJSON inconsObjs } -- | Executes the given action, and if any new 'InconsistentMetadata's are added to the schema -- cache as a result of its execution, raises an error. @@ -175,6 +175,6 @@ withNewInconsistentObjsCheck action = do nub $ concatMap toList $ M.elems (currentObjects `diffInconsistentObjects` originalObjects) unless (null newInconsistentObjects) $ throwError (err500 Unexpected "cannot continue due to newly found inconsistent metadata") - { qeInternal = Just $ toJSON newInconsistentObjects } + { qeExtra = Just $ EEInternal $ toJSON newInconsistentObjects } pure result diff --git a/server/src-lib/Hasura/Server/Migrate.hs b/server/src-lib/Hasura/Server/Migrate.hs index e8ba2a7457ac3..d4c61e37351ba 100644 --- a/server/src-lib/Hasura/Server/Migrate.hs +++ b/server/src-lib/Hasura/Server/Migrate.hs @@ -77,7 +77,7 @@ instance ToEngineLog MigrationResult Hasura where } -- A migration and (hopefully) also its inverse if we have it. --- Polymorphic because `m` can be any `MonadTx`, `MonadIO` when +-- Polymorphic because `m` can be any `MonadTx`, `MonadIO` when -- used in the `migrations` function below. data MigrationPair m = MigrationPair { mpMigrate :: m () @@ -132,7 +132,7 @@ migrateCatalog migrationTime = do _ -> requiredError where requiredError = - (err500 PostgresError requiredMessage) { qeInternal = Just $ A.toJSON e } + (err500 PostgresError requiredMessage) { qeExtra = Just $ EEInternal $ A.toJSON e } requiredMessage = "pgcrypto extension is required, but it could not be created;" <> " encountered unknown postgres error" @@ -159,12 +159,12 @@ migrateCatalog migrationTime = do pure (MRMigrated previousVersion, schemaCache) where neededMigrations = dropWhile ((/= previousVersion) . fst) (migrations False) - + buildCacheAndRecreateSystemMetadata :: m (RebuildableSchemaCache m) buildCacheAndRecreateSystemMetadata = do schemaCache <- buildRebuildableSchemaCache view _2 <$> runCacheRWT schemaCache recreateSystemMetadata - + updateCatalogVersion = setCatalogVersion latestCatalogVersionString migrationTime doesSchemaExist schemaName = @@ -197,29 +197,29 @@ downgradeCatalog opts time = do downgradeFrom previousVersion | previousVersion == dgoTargetVersion opts = do pure MRNothingToDo - | otherwise = + | otherwise = case neededDownMigrations (dgoTargetVersion opts) of - Left reason -> + Left reason -> throw400 NotSupported $ "This downgrade path (from " - <> previousVersion <> " to " - <> dgoTargetVersion opts <> + <> previousVersion <> " to " + <> dgoTargetVersion opts <> ") is not supported, because " <> reason Right path -> do - sequence_ path + sequence_ path unless (dgoDryRun opts) do setCatalogVersion (dgoTargetVersion opts) time pure (MRMigrated previousVersion) - + where - neededDownMigrations newVersion = - downgrade previousVersion newVersion + neededDownMigrations newVersion = + downgrade previousVersion newVersion (reverse (migrations (dgoDryRun opts))) - downgrade + downgrade :: T.Text - -> T.Text + -> T.Text -> [(T.Text, MigrationPair m)] -> Either T.Text [m ()] downgrade lower upper = skipFutureDowngrades where @@ -237,7 +237,7 @@ downgradeCatalog opts time = do | otherwise = skipFutureDowngrades xs dropOlderDowngrades [] = Left "the target version is unrecognized." - dropOlderDowngrades ((x, MigrationPair{ mpDown = Nothing }):_) = + dropOlderDowngrades ((x, MigrationPair{ mpDown = Nothing }):_) = Left $ "there is no available migration back to version " <> x <> "." dropOlderDowngrades ((x, MigrationPair{ mpDown = Just y }):xs) | x == upper = Right [y] @@ -271,7 +271,7 @@ migrations dryRun = if exists then [| Just (runTxOrPrint $(Q.sqlFromFile path)) |] else [| Nothing |] - + migrationsFromFile = map $ \(to :: Integer) -> let from = to - 1 in [| ( $(TH.lift $ T.pack (show from)) @@ -288,7 +288,7 @@ migrations dryRun = where runTxOrPrint :: Q.Query -> m () runTxOrPrint - | dryRun = + | dryRun = liftIO . TIO.putStrLn . Q.getQueryText | otherwise = runTx From f67eb3ab2c36417141813720a1bde07c3750ac1a Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Thu, 9 Apr 2020 22:46:29 +0530 Subject: [PATCH 5/7] add tests when a action handler returns an error status --- server/tests-py/context.py | 18 +++++++++++++++ .../actions/sync/client_error_response.yaml | 17 ++++++++++++++ .../queries/actions/sync/schema_setup.yaml | 22 ++++++++++++++++++ .../queries/actions/sync/schema_teardown.yaml | 8 +++++++ .../actions/sync/server_error_response.yaml | 23 +++++++++++++++++++ server/tests-py/test_actions.py | 12 ++++++++++ 6 files changed, 100 insertions(+) create mode 100644 server/tests-py/queries/actions/sync/client_error_response.yaml create mode 100644 server/tests-py/queries/actions/sync/server_error_response.yaml diff --git a/server/tests-py/context.py b/server/tests-py/context.py index c46be3918f568..e6301d492df1f 100644 --- a/server/tests-py/context.py +++ b/server/tests-py/context.py @@ -190,6 +190,24 @@ def do_POST(self): resp, status = self.mirror_action() self._send_response(status, resp) + elif req_path == "/server-error": + resp = { + "message":"something went wrong", + "extensions": { + "foo1":"baz1" + } + } + self._send_response(501, resp) + + elif req_path == "/client-error": + resp = { + "message":"something went wrong", + "extensions": { + "foo1":"baz1" + } + } + self._send_response(401, resp) + else: self.send_response(HTTPStatus.NO_CONTENT) self.end_headers() diff --git a/server/tests-py/queries/actions/sync/client_error_response.yaml b/server/tests-py/queries/actions/sync/client_error_response.yaml new file mode 100644 index 0000000000000..c6fb4142487fe --- /dev/null +++ b/server/tests-py/queries/actions/sync/client_error_response.yaml @@ -0,0 +1,17 @@ +description: Run an action with a failing handler returning 4xx status +url: /v1/graphql +status: 200 +query: + query: | + mutation { + client_error_action(email:"clarke@gmail.com") { + id + name + } + } + +response: + errors: + - extensions: + foo1: baz1 + message: something went wrong diff --git a/server/tests-py/queries/actions/sync/schema_setup.yaml b/server/tests-py/queries/actions/sync/schema_setup.yaml index b17d08eee252b..9d3d660c3b3c2 100644 --- a/server/tests-py/queries/actions/sync/schema_setup.yaml +++ b/server/tests-py/queries/actions/sync/schema_setup.yaml @@ -87,3 +87,25 @@ args: type: 'InObject!' output_type: 'OutObject' handler: http://127.0.0.1:5593/mirror-action + +- type: create_action + args: + name: server_error_action + definition: + kind: synchronous + arguments: + - name: email + type: 'String!' + output_type: 'OutObject' + handler: http://127.0.0.1:5593/server-error + +- type: create_action + args: + name: client_error_action + definition: + kind: synchronous + arguments: + - name: email + type: 'String!' + output_type: 'OutObject' + handler: http://127.0.0.1:5593/client-error diff --git a/server/tests-py/queries/actions/sync/schema_teardown.yaml b/server/tests-py/queries/actions/sync/schema_teardown.yaml index fbe33d85956b7..b95177f514f84 100644 --- a/server/tests-py/queries/actions/sync/schema_teardown.yaml +++ b/server/tests-py/queries/actions/sync/schema_teardown.yaml @@ -12,6 +12,14 @@ args: args: name: mirror clear_data: true +- type: drop_action + args: + name: server_error_action + clear_data: true +- type: drop_action + args: + name: client_error_action + clear_data: true # clear custom types - type: set_custom_types args: {} diff --git a/server/tests-py/queries/actions/sync/server_error_response.yaml b/server/tests-py/queries/actions/sync/server_error_response.yaml new file mode 100644 index 0000000000000..a71ff4486aeb3 --- /dev/null +++ b/server/tests-py/queries/actions/sync/server_error_response.yaml @@ -0,0 +1,23 @@ +description: Run an action with a failing handler returning 5xx status +url: /v1/graphql +status: 200 +query: + query: | + mutation { + server_error_action(email:"clarke@gmail.com") { + id + name + } + } + +response: + errors: + - extensions: + internal: + webhook_response: + extensions: + foo1: baz1 + message: something went wrong + path: $ + code: unexpected + message: internal error diff --git a/server/tests-py/test_actions.py b/server/tests-py/test_actions.py index 2ea388301ca1a..aaf5a959aa823 100644 --- a/server/tests-py/test_actions.py +++ b/server/tests-py/test_actions.py @@ -96,6 +96,18 @@ def test_set_cookie_header(self, hge_ctx): assert ('Set-Cookie' in resp_headers and resp_headers['Set-Cookie'] == 'abcd'), resp_headers +@use_action_fixtures +class TestActionsErrorHandling: + + @classmethod + def dir(cls): + return 'queries/actions/sync' + + def test_handle_action_server_error(self, hge_ctx): + check_query_f(hge_ctx, self.dir() + '/server_error_response.yaml') + + def test_handle_action_client_error(self, hge_ctx): + check_query_f(hge_ctx, self.dir() + '/client_error_response.yaml') @use_action_fixtures class TestActionsAsync: From 2e988572dbb45425f4238973dd80f45fd0c53fc9 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Mon, 13 Apr 2020 18:38:00 +0530 Subject: [PATCH 6/7] encode the error to JSON before writing to DB --- server/src-lib/Hasura/GraphQL/Resolve/Action.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src-lib/Hasura/GraphQL/Resolve/Action.hs b/server/src-lib/Hasura/GraphQL/Resolve/Action.hs index a83b1cd28ef84..2aa9e25370f33 100644 --- a/server/src-lib/Hasura/GraphQL/Resolve/Action.hs +++ b/server/src-lib/Hasura/GraphQL/Resolve/Action.hs @@ -320,7 +320,7 @@ asyncActionsProcessor cacheRef pgPool httpManager = forever $ do update hdb_catalog.hdb_action_log set errors = $1, status = 'error' where id = $2 - |] (Q.AltJ e, actionId) False + |] (Q.AltJ $ encodeGQLErr True e, actionId) False -- includeInternal set to true due to the status 500 errors setCompleted :: UUID.UUID -> J.Value -> IO () setCompleted actionId responsePayload = From 37210c818d10236fb6549cd7b2ed54279324cc8d Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Mon, 13 Apr 2020 18:44:30 +0530 Subject: [PATCH 7/7] modify the action tests to include extensions in the error handler --- server/tests-py/context.py | 10 +++++++--- .../queries/actions/sync/create_user_fail.yaml | 5 ++--- .../queries/actions/sync/create_users_fail.yaml | 3 +-- server/tests-py/test_actions.py | 7 ++++--- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/server/tests-py/context.py b/server/tests-py/context.py index e6301d492df1f..e0b9de5776815 100644 --- a/server/tests-py/context.py +++ b/server/tests-py/context.py @@ -218,8 +218,10 @@ def create_user(self): if not self.check_email(email_address): response = { - 'message': 'Given email address is not valid', - 'code': 'invalid-email' + 'message': 'Email address is not valid: ' + email_address, + 'extensions':{ + "validation_error": email_address + " is not a valid email" + } } return response, HTTPStatus.BAD_REQUEST @@ -255,7 +257,9 @@ def create_users(self): if not self.check_email(email_address): response = { 'message': 'Email address is not valid: ' + email_address, - 'code': 'invalid-email' + 'extensions': { + "validation_error": email_address + " is not a valid email" + } } return response, HTTPStatus.BAD_REQUEST diff --git a/server/tests-py/queries/actions/sync/create_user_fail.yaml b/server/tests-py/queries/actions/sync/create_user_fail.yaml index 625d5a6f044bb..ac59e0084e79c 100644 --- a/server/tests-py/queries/actions/sync/create_user_fail.yaml +++ b/server/tests-py/queries/actions/sync/create_user_fail.yaml @@ -12,6 +12,5 @@ query: response: errors: - extensions: - path: $ - code: invalid-email - message: Given email address is not valid + validation_error: random-email is not a valid email + message: 'Email address is not valid: random-email' diff --git a/server/tests-py/queries/actions/sync/create_users_fail.yaml b/server/tests-py/queries/actions/sync/create_users_fail.yaml index ca00b83169ac9..8636ed2e5e347 100644 --- a/server/tests-py/queries/actions/sync/create_users_fail.yaml +++ b/server/tests-py/queries/actions/sync/create_users_fail.yaml @@ -13,6 +13,5 @@ query: response: errors: - extensions: - path: $ - code: invalid-email + validation_error: random-email is not a valid email message: 'Email address is not valid: random-email' diff --git a/server/tests-py/test_actions.py b/server/tests-py/test_actions.py index aaf5a959aa823..590da9297d252 100644 --- a/server/tests-py/test_actions.py +++ b/server/tests-py/test_actions.py @@ -150,9 +150,10 @@ def test_create_user_fail(self, hge_ctx): 'create_user': { 'id': action_id, 'errors': { - 'code': 'invalid-email', - 'path': '$', - 'error': 'Given email address is not valid' + 'message': 'Email address is not valid: random-email', + 'extensions': { + 'validation_error': "random-email is not a valid email" + } } } }