From 100b7c0924ab7d6534ada9b60938c68bdb12f2b0 Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Wed, 22 May 2019 03:28:18 +0530 Subject: [PATCH 1/5] fix response for remote schema queries over ws (fix #2246) remote schema queries over websocket would return nested data key in payload --- server/graphql-engine.cabal | 1 + .../src-lib/Hasura/GraphQL/Transport/WebSocket.hs | 14 ++++++++++++-- server/tests-py/test_schema_stitching.py | 12 +++++++----- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/server/graphql-engine.cabal b/server/graphql-engine.cabal index 1fd26e46456fb..20f43afdded65 100644 --- a/server/graphql-engine.cabal +++ b/server/graphql-engine.cabal @@ -40,6 +40,7 @@ library , mtl , aeson , aeson-casing + , lens-aeson , unordered-containers , template-haskell , hashable diff --git a/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs b/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs index ff361e1918e03..1145532411a06 100644 --- a/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs +++ b/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs @@ -10,10 +10,12 @@ import qualified Control.Concurrent.Async as A import qualified Control.Concurrent.STM as STM import qualified Data.Aeson as J import qualified Data.Aeson.Casing as J +import qualified Data.Aeson.Lens as J import qualified Data.Aeson.TH as J import qualified Data.ByteString.Lazy as BL import qualified Data.CaseInsensitive as CI import qualified Data.HashMap.Strict as Map +import qualified Data.IORef as IORef import qualified Data.Text as T import qualified Data.Text.Encoding as TE import qualified Data.Time.Clock as TC @@ -25,8 +27,8 @@ import qualified Network.WebSockets as WS import qualified StmContainers.Map as STMMap import Control.Concurrent (threadDelay) +import Control.Lens ((^?)) import Data.ByteString (ByteString) -import qualified Data.IORef as IORef import Hasura.EncJSON import qualified Hasura.GraphQL.Execute as E @@ -285,9 +287,17 @@ onStart serverEnv wsConn (StartMsg opId q) msgRaw = catchAndIgnore $ do let payload = J.encode $ _wpPayload sockPayload resp <- runExceptT $ E.execRemoteGQ httpMgr userInfo reqHdrs payload rsi opDef - either postExecErr sendSuccResp resp + either postExecErr sendRemoteSucc resp sendCompleted + sendRemoteSucc resp = do + let respBs = encJToBS resp + case respBs ^? J.key "data" of + Just val -> sendSuccResp $ encJFromJValue val + Nothing -> postExecErr $ err500 Unexpected $ + "Failed parsing GraphQL response from remote: " + <> bsToTxt respBs + WSServerEnv logger pgExecCtx lqMap gCtxMapRef httpMgr _ sqlGenCtx planCache _ enableAL = serverEnv diff --git a/server/tests-py/test_schema_stitching.py b/server/tests-py/test_schema_stitching.py index e47bca1378138..68f3b7657e76c 100644 --- a/server/tests-py/test_schema_stitching.py +++ b/server/tests-py/test_schema_stitching.py @@ -301,11 +301,12 @@ def test_remote_query(self, ws_client): } """ query_id = ws_client.gen_id() - resp = ws_client.send_query({'query': query},query_id = query_id,timeout=5) + resp = ws_client.send_query({'query': query}, query_id=query_id, + timeout=5) try: ev = next(resp) assert ev['type'] == 'data' and ev['id'] == query_id, ev - assert ev['payload']['data']['data']['user']['username'] == 'john' + assert ev['payload']['data']['user']['username'] == 'john' finally: ws_client.stop(query_id) @@ -321,12 +322,13 @@ def test_remote_mutation(self, ws_client): } """ query_id = ws_client.gen_id() - resp = ws_client.send_query({'query': query},query_id = query_id,timeout=5) + resp = ws_client.send_query({'query': query}, query_id=query_id, + timeout=5) try: ev = next(resp) assert ev['type'] == 'data' and ev['id'] == query_id, ev - assert ev['payload']['data']['data']['createUser']['user']['id'] == 42 - assert ev['payload']['data']['data']['createUser']['user']['username'] == 'foobar' + assert ev['payload']['data']['createUser']['user']['id'] == 42 + assert ev['payload']['data']['createUser']['user']['username'] == 'foobar' finally: ws_client.stop(query_id) From 5433029641d23b5fd673f8eac7d0cf1c65bf5dbb Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Wed, 22 May 2019 17:57:42 +0530 Subject: [PATCH 2/5] handle errors as well --- .../Hasura/GraphQL/Transport/WebSocket.hs | 29 ++++++++++++++----- server/tests-py/test_schema_stitching.py | 22 ++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs b/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs index 1145532411a06..6a95b5b57dc68 100644 --- a/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs +++ b/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs @@ -287,16 +287,31 @@ onStart serverEnv wsConn (StartMsg opId q) msgRaw = catchAndIgnore $ do let payload = J.encode $ _wpPayload sockPayload resp <- runExceptT $ E.execRemoteGQ httpMgr userInfo reqHdrs payload rsi opDef - either postExecErr sendRemoteSucc resp + either postExecErr sendRemoteResp resp sendCompleted - sendRemoteSucc resp = do + sendRemoteResp resp = do let respBs = encJToBS resp - case respBs ^? J.key "data" of - Just val -> sendSuccResp $ encJFromJValue val - Nothing -> postExecErr $ err500 Unexpected $ - "Failed parsing GraphQL response from remote: " - <> bsToTxt respBs + when (not $ isValidGqlResp respBs) $ do + logOpEv $ ODQueryErr $ invalidGqlErr respBs + postExecErr $ invalidGqlErr respBs + + case J.decodeStrict respBs of + Just jVal -> do + let res = J.encode $ J.object [ "type" J..= ("data" :: Text) + , "id" J..= opId + , "payload" J..= (jVal :: J.Value) + ] + liftIO $ WS.sendMsg wsConn res + Nothing -> do + logOpEv $ ODQueryErr $ invalidGqlErr respBs + postExecErr $ invalidGqlErr respBs + + invalidGqlErr resp = err500 Unexpected $ + "Failed parsing GraphQL response from remote: " <> bsToTxt resp + + isValidGqlResp resp = + isJust $ (resp ^? J.key "data") <|> (resp ^? J.key "errors") WSServerEnv logger pgExecCtx lqMap gCtxMapRef httpMgr _ sqlGenCtx planCache _ enableAL = serverEnv diff --git a/server/tests-py/test_schema_stitching.py b/server/tests-py/test_schema_stitching.py index 68f3b7657e76c..b6ff8ebd51fb5 100644 --- a/server/tests-py/test_schema_stitching.py +++ b/server/tests-py/test_schema_stitching.py @@ -310,6 +310,28 @@ def test_remote_query(self, ws_client): finally: ws_client.stop(query_id) + def test_remote_query_error(self, ws_client): + query = """ + query { + user(id: 2) { + blah + username + } + } + """ + query_id = ws_client.gen_id() + resp = ws_client.send_query({'query': query}, query_id=query_id, + timeout=5) + try: + ev = next(resp) + print(ev) + assert ev['type'] == 'data' and ev['id'] == query_id, ev + assert 'errors' in ev['payload'] + assert ev['payload']['errors'][0]['message'] == \ + 'Cannot query field "blah" on type "User".' + finally: + ws_client.stop(query_id) + def test_remote_mutation(self, ws_client): query = """ mutation { From b6e3f459706bfdcd0ee1d30018569969ba3e32a6 Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Mon, 27 May 2019 15:09:53 +0530 Subject: [PATCH 3/5] fix better handling of remote GQL response --- server/graphql-engine.cabal | 1 - .../Hasura/GraphQL/Transport/WebSocket.hs | 51 ++++++++++--------- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/server/graphql-engine.cabal b/server/graphql-engine.cabal index 20f43afdded65..1fd26e46456fb 100644 --- a/server/graphql-engine.cabal +++ b/server/graphql-engine.cabal @@ -40,7 +40,6 @@ library , mtl , aeson , aeson-casing - , lens-aeson , unordered-containers , template-haskell , hashable diff --git a/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs b/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs index 6a95b5b57dc68..bb8ab572022a8 100644 --- a/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs +++ b/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs @@ -10,7 +10,6 @@ import qualified Control.Concurrent.Async as A import qualified Control.Concurrent.STM as STM import qualified Data.Aeson as J import qualified Data.Aeson.Casing as J -import qualified Data.Aeson.Lens as J import qualified Data.Aeson.TH as J import qualified Data.ByteString.Lazy as BL import qualified Data.CaseInsensitive as CI @@ -27,7 +26,6 @@ import qualified Network.WebSockets as WS import qualified StmContainers.Map as STMMap import Control.Concurrent (threadDelay) -import Control.Lens ((^?)) import Data.ByteString (ByteString) import Hasura.EncJSON @@ -137,6 +135,21 @@ data WSServerEnv , _wseEnableAllowlist :: !Bool } +data RemoteGQResp + = RGRDataResp !J.Value + | RGRErrResp ![J.Value] + +instance J.FromJSON RemoteGQResp where + parseJSON = J.withObject "RemoteGQResp" $ \obj -> do + mVal <- obj J..:? "data" + case mVal of + Just dat -> return $ RGRDataResp dat + Nothing -> do + mErrs <- obj J..:? "errors" + case mErrs of + Just val -> return $ RGRErrResp val + Nothing -> fail "not a valid GraphQL response" + onConn :: L.Logger -> CorsPolicy -> WS.OnConnH WSConnData onConn (L.Logger logger) corsPolicy wsId requestHead = do res <- runExceptT $ do @@ -290,28 +303,15 @@ onStart serverEnv wsConn (StartMsg opId q) msgRaw = catchAndIgnore $ do either postExecErr sendRemoteResp resp sendCompleted - sendRemoteResp resp = do - let respBs = encJToBS resp - when (not $ isValidGqlResp respBs) $ do - logOpEv $ ODQueryErr $ invalidGqlErr respBs - postExecErr $ invalidGqlErr respBs - - case J.decodeStrict respBs of - Just jVal -> do - let res = J.encode $ J.object [ "type" J..= ("data" :: Text) - , "id" J..= opId - , "payload" J..= (jVal :: J.Value) - ] - liftIO $ WS.sendMsg wsConn res - Nothing -> do - logOpEv $ ODQueryErr $ invalidGqlErr respBs - postExecErr $ invalidGqlErr respBs - - invalidGqlErr resp = err500 Unexpected $ - "Failed parsing GraphQL response from remote: " <> bsToTxt resp - - isValidGqlResp resp = - isJust $ (resp ^? J.key "data") <|> (resp ^? J.key "errors") + sendRemoteResp resp = + case J.eitherDecodeStrict (encJToBS resp) of + Left e -> postExecErr $ invalidGqlErr $ T.pack e + Right res -> case res of + RGRDataResp val -> sendSuccResp $ encJFromJValue val + RGRErrResp errs -> remotePostExecErr errs + + invalidGqlErr err = err500 Unexpected $ + "Failed parsing GraphQL response from remote: " <> err WSServerEnv logger pgExecCtx lqMap gCtxMapRef httpMgr _ sqlGenCtx planCache _ enableAL = serverEnv @@ -342,6 +342,9 @@ onStart serverEnv wsConn (StartMsg opId q) msgRaw = catchAndIgnore $ do sendMsg wsConn $ SMData $ DataMsg opId $ GQExecError $ pure $ errFn False qErr + remotePostExecErr jVal = + sendMsg wsConn $ SMData $ DataMsg opId $ GQExecError jVal + -- why wouldn't pre exec error use graphql response? preExecErr qErr = do let errFn = getErrFn errRespTy From 7e07f5e141bdcaa91de0497e5bed34395b313156 Mon Sep 17 00:00:00 2001 From: Shahidh K Muhammed Date: Mon, 27 May 2019 16:03:14 +0530 Subject: [PATCH 4/5] fix version parsing for dev builds --- server/src-lib/Hasura/Server/Version.hs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/src-lib/Hasura/Server/Version.hs b/server/src-lib/Hasura/Server/Version.hs index bb44fb294b8d4..35f57ea842985 100644 --- a/server/src-lib/Hasura/Server/Version.hs +++ b/server/src-lib/Hasura/Server/Version.hs @@ -6,6 +6,7 @@ module Hasura.Server.Version where import Control.Lens ((^.), (^?)) +import Data.Either (isLeft) import qualified Data.SemVer as V import qualified Data.Text as T @@ -24,9 +25,7 @@ currentVersion :: T.Text currentVersion = version isDevVersion :: Bool -isDevVersion = case parsedVersion of - Left _ -> False - Right _ -> True +isDevVersion = isLeft parsedVersion rawVersion :: T.Text rawVersion = "versioned/" <> version From 28b76e5ad6c92b8c818d16da0ffb2899ec0a6108 Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Tue, 28 May 2019 12:37:05 +0530 Subject: [PATCH 5/5] improved data types for handling remote and hasura GQL responses --- .../Hasura/GraphQL/Transport/HTTP/Protocol.hs | 29 +++++++++++++++++ .../Hasura/GraphQL/Transport/WebSocket.hs | 32 ++++--------------- .../GraphQL/Transport/WebSocket/Protocol.hs | 4 +-- 3 files changed, 38 insertions(+), 27 deletions(-) diff --git a/server/src-lib/Hasura/GraphQL/Transport/HTTP/Protocol.hs b/server/src-lib/Hasura/GraphQL/Transport/HTTP/Protocol.hs index b69c04e9aacbd..f6ac8e5dc0f67 100644 --- a/server/src-lib/Hasura/GraphQL/Transport/HTTP/Protocol.hs +++ b/server/src-lib/Hasura/GraphQL/Transport/HTTP/Protocol.hs @@ -11,6 +11,9 @@ module Hasura.GraphQL.Transport.HTTP.Protocol , encodeGQResp , GQResp(..) , isExecError + , RemoteGqlResp(..) + , GraphqlResponse(..) + , encodeGraphqlResponse ) where import Hasura.EncJSON @@ -96,3 +99,29 @@ encodeGQResp gqResp = GQSuccess r -> [("data", encJFromLBS r)] GQPreExecError e -> [("errors", encJFromJValue e)] GQExecError e -> [("data", "null"), ("errors", encJFromJValue e)] + +-- | Represents GraphQL response from a remote server +data RemoteGqlResp + = RemoteGqlResp + { _rgqrData :: !(Maybe J.Value) + , _rgqrErrors :: !(Maybe [J.Value]) + , _rgqrExtensions :: !(Maybe J.Value) + } deriving (Show, Eq) +$(J.deriveFromJSON (J.aesonDrop 5 J.camelCase) ''RemoteGqlResp) + +encodeRemoteGqlResp :: RemoteGqlResp -> EncJSON +encodeRemoteGqlResp (RemoteGqlResp d e ex) = + encJFromAssocList [ ("data", encJFromJValue d) + , ("errors", encJFromJValue e) + , ("extensions", encJFromJValue ex) + ] + +-- | Represents a proper GraphQL response +data GraphqlResponse + = GRHasura !GQResp + | GRRemote !RemoteGqlResp + +encodeGraphqlResponse :: GraphqlResponse -> EncJSON +encodeGraphqlResponse = \case + GRHasura resp -> encodeGQResp resp + GRRemote resp -> encodeRemoteGqlResp resp diff --git a/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs b/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs index bb8ab572022a8..8af0e42c2c53c 100644 --- a/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs +++ b/server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs @@ -135,21 +135,6 @@ data WSServerEnv , _wseEnableAllowlist :: !Bool } -data RemoteGQResp - = RGRDataResp !J.Value - | RGRErrResp ![J.Value] - -instance J.FromJSON RemoteGQResp where - parseJSON = J.withObject "RemoteGQResp" $ \obj -> do - mVal <- obj J..:? "data" - case mVal of - Just dat -> return $ RGRDataResp dat - Nothing -> do - mErrs <- obj J..:? "errors" - case mErrs of - Just val -> return $ RGRErrResp val - Nothing -> fail "not a valid GraphQL response" - onConn :: L.Logger -> CorsPolicy -> WS.OnConnH WSConnData onConn (L.Logger logger) corsPolicy wsId requestHead = do res <- runExceptT $ do @@ -305,10 +290,8 @@ onStart serverEnv wsConn (StartMsg opId q) msgRaw = catchAndIgnore $ do sendRemoteResp resp = case J.eitherDecodeStrict (encJToBS resp) of - Left e -> postExecErr $ invalidGqlErr $ T.pack e - Right res -> case res of - RGRDataResp val -> sendSuccResp $ encJFromJValue val - RGRErrResp errs -> remotePostExecErr errs + Left e -> postExecErr $ invalidGqlErr $ T.pack e + Right res -> sendMsg wsConn $ SMData $ DataMsg opId (GRRemote res) invalidGqlErr err = err500 Unexpected $ "Failed parsing GraphQL response from remote: " <> err @@ -340,10 +323,7 @@ onStart serverEnv wsConn (StartMsg opId q) msgRaw = catchAndIgnore $ do let errFn = getErrFn errRespTy logOpEv $ ODQueryErr qErr sendMsg wsConn $ SMData $ DataMsg opId $ - GQExecError $ pure $ errFn False qErr - - remotePostExecErr jVal = - sendMsg wsConn $ SMData $ DataMsg opId $ GQExecError jVal + GRHasura $ GQExecError $ pure $ errFn False qErr -- why wouldn't pre exec error use graphql response? preExecErr qErr = do @@ -355,7 +335,8 @@ onStart serverEnv wsConn (StartMsg opId q) msgRaw = catchAndIgnore $ do sendMsg wsConn $ SMErr $ ErrorMsg opId err sendSuccResp encJson = - sendMsg wsConn $ SMData $ DataMsg opId $ GQSuccess $ encJToLBS encJson + sendMsg wsConn $ SMData $ DataMsg opId $ + GRHasura $ GQSuccess $ encJToLBS encJson withComplete :: ExceptT () IO () -> ExceptT () IO a withComplete action = do @@ -365,7 +346,8 @@ onStart serverEnv wsConn (StartMsg opId q) msgRaw = catchAndIgnore $ do -- on change, send message on the websocket liveQOnChange resp = - WS.sendMsg wsConn $ encodeServerMsg $ SMData $ DataMsg opId resp + WS.sendMsg wsConn $ encodeServerMsg $ SMData $ + DataMsg opId (GRHasura resp) catchAndIgnore :: ExceptT () IO () -> IO () catchAndIgnore m = void $ runExceptT m diff --git a/server/src-lib/Hasura/GraphQL/Transport/WebSocket/Protocol.hs b/server/src-lib/Hasura/GraphQL/Transport/WebSocket/Protocol.hs index 6cb25b3a8a14f..45180bd568903 100644 --- a/server/src-lib/Hasura/GraphQL/Transport/WebSocket/Protocol.hs +++ b/server/src-lib/Hasura/GraphQL/Transport/WebSocket/Protocol.hs @@ -67,7 +67,7 @@ instance J.FromJSON ClientMsg where data DataMsg = DataMsg { _dmId :: !OperationId - , _dmPayload :: !GQResp + , _dmPayload :: !GraphqlResponse } data ErrorMsg @@ -131,7 +131,7 @@ encodeServerMsg msg = SMData (DataMsg opId payload) -> [ encTy SMT_GQL_DATA , ("id", encJFromJValue opId) - , ("payload", encodeGQResp payload) + , ("payload", encodeGraphqlResponse payload) ] SMErr (ErrorMsg opId payload) ->