From bf2de78ea33ff8b30e5b4f4cb7cebe53f0d348f3 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Mon, 17 Aug 2020 14:51:11 +0530 Subject: [PATCH 1/9] server: don't throw validation error when joining singleton type to array type --- .../RQL/DDL/RemoteRelationship/Validate.hs | 22 +++++++++------- ...tionship_joining_singleton_with_array.yaml | 26 +++++++++++++++++++ ...mote_rel_joining_singleton_with_array.yaml | 25 ++++++++++++++++++ .../tests-py/remote_schemas/nodejs/index.js | 8 ++++++ server/tests-py/test_remote_relationships.py | 8 ++++++ 5 files changed, 79 insertions(+), 10 deletions(-) create mode 100644 server/tests-py/queries/remote_schemas/remote_relationships/basic_relationship_joining_singleton_with_array.yaml create mode 100644 server/tests-py/queries/remote_schemas/remote_relationships/setup_remote_rel_joining_singleton_with_array.yaml diff --git a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs index ae137ce5db510..ab83e46d74673 100644 --- a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs +++ b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs @@ -407,16 +407,18 @@ validateType permittedVariables value expectedGType types = assertType :: G.GType -> G.GType -> Validation (NonEmpty ValidationError) () assertType actualType expectedType = do - -- check if both are list types or both are named types - (when - (isListType' actualType /= isListType' expectedType) - (Failure (pure $ ExpectedTypeButGot expectedType actualType))) - -- if list type then check over unwrapped type, else check base types - if isListType' actualType - then assertType (unwrapTy actualType) (unwrapTy expectedType) - else (when - (getBaseTy actualType /= getBaseTy expectedType) - (Failure (pure $ ExpectedTypeButGot expectedType actualType))) + -- The GraphQL spec says that, we can coerce a singleton value into an array + -- value. Which means that if the 'actualType' is a singleton value, like + -- 'Int' we should be able to join this with a remote node, which expects an + -- input argument of type '[Int]' + -- http://spec.graphql.org/June2018/#sec-Type-System.List + case (isListType' actualType,isListType' expectedType) of + (True, True) -> assertType (unwrapTy actualType) (unwrapTy expectedType) + (True, False) -> Failure (pure $ ExpectedTypeButGot expectedType actualType) + _ -> + (when + (getBaseTy actualType /= getBaseTy expectedType) + (Failure (pure $ ExpectedTypeButGot expectedType actualType))) pure () assertListType :: G.GType -> Validation (NonEmpty ValidationError) () diff --git a/server/tests-py/queries/remote_schemas/remote_relationships/basic_relationship_joining_singleton_with_array.yaml b/server/tests-py/queries/remote_schemas/remote_relationships/basic_relationship_joining_singleton_with_array.yaml new file mode 100644 index 0000000000000..49c066fd7cef8 --- /dev/null +++ b/server/tests-py/queries/remote_schemas/remote_relationships/basic_relationship_joining_singleton_with_array.yaml @@ -0,0 +1,26 @@ +description: Simple Remote relationship with singleton input value type joining to an expected array input type +url: /v1/graphql +status: 200 +response: + data: + profiles: + - id: 1 + remoteUsers: + - user_id: 1 + userMessages: + - id: 1 + msg: "You win!" +query: + query: | + query { + profiles(where:{ id: { _eq: 1}}) { + id + remoteUsers { + user_id + userMessages { + id + msg + } + } + } + } diff --git a/server/tests-py/queries/remote_schemas/remote_relationships/setup_remote_rel_joining_singleton_with_array.yaml b/server/tests-py/queries/remote_schemas/remote_relationships/setup_remote_rel_joining_singleton_with_array.yaml new file mode 100644 index 0000000000000..6871d9f1c71f5 --- /dev/null +++ b/server/tests-py/queries/remote_schemas/remote_relationships/setup_remote_rel_joining_singleton_with_array.yaml @@ -0,0 +1,25 @@ +type: bulk +args: +- type: create_remote_relationship + args: + name: remoteUsers + table: profiles + hasura_fields: + - id + remote_schema: my-remote-schema + remote_field: + users: + arguments: + user_ids: "$id" + +- type: create_remote_relationship + args: + name: remoteUsersMultipleIds + table: profiles + hasura_fields: + - id + remote_schema: my-remote-schema + remote_field: + users: + arguments: + user_ids: ["$id"] diff --git a/server/tests-py/remote_schemas/nodejs/index.js b/server/tests-py/remote_schemas/nodejs/index.js index 15b61cbc3bd9e..7ea925e9c0590 100644 --- a/server/tests-py/remote_schemas/nodejs/index.js +++ b/server/tests-py/remote_schemas/nodejs/index.js @@ -53,6 +53,7 @@ const typeDefs = gql` hello: String messages(where: MessageWhereInpObj, includes: IncludeInpObj): [Message] user(user_id: Int!): User + users(user_ids: [Int]!): [User] message(id: Int!) : Message communications(id: Int): [Communication] } @@ -175,6 +176,13 @@ const resolvers = { user: (_, { user_id }) => { return { "user_id": user_id }; }, + users: (parent, args, context, info) => { + var results = [] + for (userId of args.user_ids) { + results.push({"user_id":userId}) + } + return results; + }, communications: (_, { id }) => { var result = allMessages; if(id) { diff --git a/server/tests-py/test_remote_relationships.py b/server/tests-py/test_remote_relationships.py index 42b761f078080..d2274f3599710 100644 --- a/server/tests-py/test_remote_relationships.py +++ b/server/tests-py/test_remote_relationships.py @@ -54,6 +54,9 @@ def test_create_valid(self, hge_ctx): st_code, resp = hge_ctx.v1q_f(self.dir() + 'setup_remote_rel_multiple_fields.yaml') assert st_code == 200, resp + st_code, resp = hge_ctx.v1q_f(self.dir() + 'setup_remote_rel_joining_singleton_with_array.yaml') + assert st_code == 200, resp + # st_code, resp = hge_ctx.v1q_f(self.dir() + 'setup_remote_rel_with_interface.yaml') # assert st_code == 200, resp @@ -157,6 +160,11 @@ def test_basic_relationship_on_object(self, hge_ctx): assert st_code == 200, resp check_query_f(hge_ctx, self.dir() + 'query_with_arr_rel.yaml') + def test_basic_relationship_joining_singleton_to_array(self, hge_ctx): + st_code, resp = hge_ctx.v1q_f(self.dir() + 'setup_remote_rel_joining_singleton_with_array.yaml') + assert st_code == 200, resp + check_query_f(hge_ctx, self.dir() + 'basic_relationship_joining_singleton_with_array.yaml') + def test_basic_array(self, hge_ctx): st_code, resp = hge_ctx.v1q_f(self.dir() + 'setup_remote_rel_array.yaml') assert st_code == 200, resp From 87b1b5718d0273dca874eb8c9733d46a65fa760e Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Mon, 17 Aug 2020 14:55:54 +0530 Subject: [PATCH 2/9] reword a comment --- server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs index ab83e46d74673..7a4c08a19a4f2 100644 --- a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs +++ b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs @@ -408,7 +408,7 @@ validateType permittedVariables value expectedGType types = assertType :: G.GType -> G.GType -> Validation (NonEmpty ValidationError) () assertType actualType expectedType = do -- The GraphQL spec says that, we can coerce a singleton value into an array - -- value. Which means that if the 'actualType' is a singleton value, like + -- value. Which means that if the 'actualType' is a singleton type, like -- 'Int' we should be able to join this with a remote node, which expects an -- input argument of type '[Int]' -- http://spec.graphql.org/June2018/#sec-Type-System.List From 22b4b285aa7f627260875f8503455998b33ef4c3 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Mon, 17 Aug 2020 16:01:54 +0530 Subject: [PATCH 3/9] add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 821301c3e196b..827eeb38f1a9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ If you do have such headers configured, then you must update the header configur - server: add logs for action handlers - server: add request/response sizes in event triggers (and scheduled trigger) logs (#5463) - server: change startup log kind `db_migrate` to `catalog_migrate` (#5531) +- server: don't throw validation error when creating remote relationships joining singleton type with array type with same base type (fixes #5133) - console: handle nested fragments in allowed queries (close #5137) (#5252) - console: update sidebar icons for different action and trigger types (#5445) - console: make add column UX consistent with others (#5486) From 8f2a780d96af320f77652d0ccdcf1769537ca7fb Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Wed, 19 Aug 2020 13:41:19 +0530 Subject: [PATCH 4/9] improve the assertType check while validating remote relationship --- server/src-lib/Hasura/GraphQL/Utils.hs | 10 +++++++ .../RQL/DDL/RemoteRelationship/Validate.hs | 26 +++++++++++-------- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/server/src-lib/Hasura/GraphQL/Utils.hs b/server/src-lib/Hasura/GraphQL/Utils.hs index 10367786700b3..ac192274e85fd 100644 --- a/server/src-lib/Hasura/GraphQL/Utils.hs +++ b/server/src-lib/Hasura/GraphQL/Utils.hs @@ -10,6 +10,7 @@ module Hasura.GraphQL.Utils , unwrapTy , simpleGraphQLQuery , jsonValueToGValue + , getBaseTyWithNestedLevelsCount ) where import Hasura.Prelude @@ -40,6 +41,15 @@ getBaseTy = \case where getBaseTyL = getBaseTy . G.unListType +getBaseTyWithNestedLevelsCount :: G.GType -> (G.NamedType, Int) +getBaseTyWithNestedLevelsCount ty = go ty 0 + where + go :: G.GType -> Int -> (G.NamedType, Int) + go gType ctr = + case gType of + G.TypeNamed _ n -> (n, ctr) + G.TypeList _ lt -> flip go (ctr + 1) (G.unListType lt) + unwrapTy :: G.GType -> G.GType unwrapTy = \case diff --git a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs index 7a4c08a19a4f2..3c01e238e5103 100644 --- a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs +++ b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs @@ -406,20 +406,24 @@ validateType permittedVariables value expectedGType types = "not an input object type")) assertType :: G.GType -> G.GType -> Validation (NonEmpty ValidationError) () -assertType actualType expectedType = do - -- The GraphQL spec says that, we can coerce a singleton value into an array - -- value. Which means that if the 'actualType' is a singleton type, like +assertType actualType expectedType = + -- The GraphQL spec says that, a singleton type can be coerced into an array + -- type. Which means that if the 'actualType' is a singleton type, like -- 'Int' we should be able to join this with a remote node, which expects an -- input argument of type '[Int]' -- http://spec.graphql.org/June2018/#sec-Type-System.List - case (isListType' actualType,isListType' expectedType) of - (True, True) -> assertType (unwrapTy actualType) (unwrapTy expectedType) - (True, False) -> Failure (pure $ ExpectedTypeButGot expectedType actualType) - _ -> - (when - (getBaseTy actualType /= getBaseTy expectedType) - (Failure (pure $ ExpectedTypeButGot expectedType actualType))) - pure () + let (actualBaseType, actualNestingLevel) = getBaseTyWithNestedLevelsCount actualType + (expectedBaseType, expectedNestingLevel) = getBaseTyWithNestedLevelsCount expectedType + in + if actualBaseType == expectedBaseType + then if (actualNestingLevel == expectedNestingLevel || actualNestingLevel == 0) + -- The check of 'actualNestedCount == 0' is the case of coercing a singleton type + -- into an array type + then pure () + else Failure (pure $ ExpectedTypeButGot expectedType actualType) + -- we cannot coerce two types with different nesting levels, + -- for example, we cannot coerce [Int] to [[Int]] + else Failure (pure $ ExpectedTypeButGot expectedType actualType) assertListType :: G.GType -> Validation (NonEmpty ValidationError) () assertListType actualType = From 32766d2e9cf5122073d4e232ff3c931b03d285ca Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Wed, 19 Aug 2020 13:41:57 +0530 Subject: [PATCH 5/9] show the GraphQL type instead of just the base type while throwing error --- server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs index 3c01e238e5103..0d6049a27515a 100644 --- a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs +++ b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs @@ -65,7 +65,7 @@ validateErrorToText (toList -> errs) = TableFieldNonexistent table fieldName -> "field with name " <> fieldName <<> " not found in table " <>> table ExpectedTypeButGot expTy actualTy -> - "expected type " <> getBaseTy expTy <<> " but got " <>> getBaseTy actualTy + "expected type " <> G.showGT expTy <<> " but got " <>> G.showGT actualTy InvalidType ty err -> "type " <> getBaseTy ty <<> err InvalidVariable var _ -> From 0a34d58d6e554a01de7cea69c4d15521aed96dca Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Thu, 20 Aug 2020 11:34:23 +0530 Subject: [PATCH 6/9] refactor assertType to isTypeCoercible --- .../Hasura/RQL/DDL/RemoteRelationship/Validate.hs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs index 0d6049a27515a..2e30e3541680a 100644 --- a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs +++ b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs @@ -363,12 +363,12 @@ validateType permittedVariables value expectedGType types = Just fieldInfo -> bindValidation (columnInfoToNamedType fieldInfo) - (\actualNamedType -> assertType (G.toGT actualNamedType) expectedGType) - G.VInt {} -> assertType (G.toGT $ mkScalarTy PGInteger) expectedGType - G.VFloat {} -> assertType (G.toGT $ mkScalarTy PGFloat) expectedGType - G.VBoolean {} -> assertType (G.toGT $ mkScalarTy PGBoolean) expectedGType + (\actualNamedType -> isTypeCoercible (G.toGT actualNamedType) expectedGType) + G.VInt {} -> isTypeCoercible (G.toGT $ mkScalarTy PGInteger) expectedGType + G.VFloat {} -> isTypeCoercible (G.toGT $ mkScalarTy PGFloat) expectedGType + G.VBoolean {} -> isTypeCoercible (G.toGT $ mkScalarTy PGBoolean) expectedGType G.VNull -> Failure (pure NullNotAllowedHere) - G.VString {} -> assertType (G.toGT $ mkScalarTy PGText) expectedGType + G.VString {} -> isTypeCoercible (G.toGT $ mkScalarTy PGText) expectedGType G.VEnum _ -> Failure (pure UnsupportedEnum) G.VList (G.unListValue -> values) -> do case values of @@ -405,8 +405,8 @@ validateType permittedVariables value expectedGType types = (G.toGT $ G.NamedType name) "not an input object type")) -assertType :: G.GType -> G.GType -> Validation (NonEmpty ValidationError) () -assertType actualType expectedType = +isTypeCoercible :: G.GType -> G.GType -> Validation (NonEmpty ValidationError) () +isTypeCoercible actualType expectedType = -- The GraphQL spec says that, a singleton type can be coerced into an array -- type. Which means that if the 'actualType' is a singleton type, like -- 'Int' we should be able to join this with a remote node, which expects an From a3704b4cff62431bb77e915bf3c238c38f95bb30 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Fri, 21 Aug 2020 12:51:35 +0530 Subject: [PATCH 7/9] Update CHANGELOG.md Co-authored-by: Tirumarai Selvan --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 827eeb38f1a9d..36ab8ff53dbed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ If you do have such headers configured, then you must update the header configur - server: add logs for action handlers - server: add request/response sizes in event triggers (and scheduled trigger) logs (#5463) - server: change startup log kind `db_migrate` to `catalog_migrate` (#5531) -- server: don't throw validation error when creating remote relationships joining singleton type with array type with same base type (fixes #5133) +- server: allow remote relationships joining `type` column with `[type]` input argument as spec allows this coercion (fixes #5133) - console: handle nested fragments in allowed queries (close #5137) (#5252) - console: update sidebar icons for different action and trigger types (#5445) - console: make add column UX consistent with others (#5486) From a34913f24db6cea17c8155ccf83d070dd5099424 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Fri, 21 Aug 2020 13:39:16 +0530 Subject: [PATCH 8/9] update changelog --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36ab8ff53dbed..ceb65031142d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,11 @@ ## Next release +- server: allow remote relationships joining `type` column with `[type]` input argument as spec allows this coercion (fixes #5133) + ### Breaking change -Headers from environment variables starting with `HASURA_GRAPHQL_` are not allowed +Headers from environment variables starting with `HASURA_GRAPHQL_` are not allowed in event triggers, actions & remote schemas. If you do have such headers configured, then you must update the header configuration before upgrading. @@ -21,7 +23,6 @@ If you do have such headers configured, then you must update the header configur - server: add logs for action handlers - server: add request/response sizes in event triggers (and scheduled trigger) logs (#5463) - server: change startup log kind `db_migrate` to `catalog_migrate` (#5531) -- server: allow remote relationships joining `type` column with `[type]` input argument as spec allows this coercion (fixes #5133) - console: handle nested fragments in allowed queries (close #5137) (#5252) - console: update sidebar icons for different action and trigger types (#5445) - console: make add column UX consistent with others (#5486) From 22d563c3b65c75fbaeb829b688497bc72e142b1c Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Fri, 21 Aug 2020 16:21:55 +0530 Subject: [PATCH 9/9] use multiwayif instead of nested if-else --- .../Hasura/RQL/DDL/RemoteRelationship/Validate.hs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs index 2e30e3541680a..2ee67e0191f68 100644 --- a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs +++ b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs @@ -415,15 +415,13 @@ isTypeCoercible actualType expectedType = let (actualBaseType, actualNestingLevel) = getBaseTyWithNestedLevelsCount actualType (expectedBaseType, expectedNestingLevel) = getBaseTyWithNestedLevelsCount expectedType in - if actualBaseType == expectedBaseType - then if (actualNestingLevel == expectedNestingLevel || actualNestingLevel == 0) - -- The check of 'actualNestedCount == 0' is the case of coercing a singleton type - -- into an array type - then pure () - else Failure (pure $ ExpectedTypeButGot expectedType actualType) - -- we cannot coerce two types with different nesting levels, + if | actualBaseType /= expectedBaseType -> raiseValidationError + -- we cannot coerce two types with different nesting levels, -- for example, we cannot coerce [Int] to [[Int]] - else Failure (pure $ ExpectedTypeButGot expectedType actualType) + | (actualNestingLevel == expectedNestingLevel || actualNestingLevel == 0) -> pure () + | otherwise -> raiseValidationError + where + raiseValidationError = Failure (pure $ ExpectedTypeButGot expectedType actualType) assertListType :: G.GType -> Validation (NonEmpty ValidationError) () assertListType actualType =