Merge pull request #5603 from codingkarthik/remote-relationship-validation-bug-5133

https://github.com/hasura/graphql-engine/pull/5603
2024-12-15 09:22:43 +03:00 · 2020-08-27 02:44:03 +00:00 · 2020-08-27 02:44:03 +00:00 · 15794ad817
commit 15794ad817
parent 10a5579b5b 95b08086a8
7 changed files with 101 additions and 19 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -11,7 +11,6 @@ This release contains the [PDV refactor (#4111)](https://github.com/hasura/graph
  - if a query selects table `bar` through table `foo` via a relationship, the required permissions headers will be the union of the required headers of table `foo` and table `bar` (we used to only check the headers of the root table);
  - if an insert does not have an `on_conflict` clause, it will not require the update permissions headers.

-
 ### Bug fixes and improvements

 (Add entries here in the order of: server, console, cli, docs, others)
@ -19,6 +18,7 @@ This release contains the [PDV refactor (#4111)](https://github.com/hasura/graph
 - server: some mutations that cannot be performed will no longer be in the schema (for instance, `delete_by_pk` mutations won't be shown to users that do not have select permissions on all primary keys) (#4111)
 - server: miscellaneous description changes (#4111)
 - server: treat the absence of `backend_only` configuration and `backend_only: false` equally (closing #5059) (#4111)
+- server: allow remote relationships joining `type` column with `[type]` input argument as spec allows this coercion (fixes #5133)
 - console: allow user to cascade Postgres dependencies when dropping Postgres objects (close #5109) (#5248)
 - console: mark inconsistent remote schemas in the UI (close #5093) (#5181)
 - cli: add missing global flags for seeds command (#5565)
--- a/server/src-lib/Hasura/GraphQL/Utils.hs
+++ b/server/src-lib/Hasura/GraphQL/Utils.hs
@ -5,6 +5,7 @@ module Hasura.GraphQL.Utils
  , mkMapWith
  , showNames
  , simpleGraphQLQuery
+  , getBaseTyWithNestedLevelsCount
  ) where

 import           Hasura.Prelude
@ -17,6 +18,15 @@ import qualified Language.GraphQL.Draft.Syntax as G
 showName :: G.Name -> Text
 showName name = "\"" <> G.unName name <> "\""

+getBaseTyWithNestedLevelsCount :: G.GType -> (G.Name, Int)
+getBaseTyWithNestedLevelsCount ty = go ty 0
+  where
+    go :: G.GType -> Int -> (G.Name, Int)
+    go gType ctr =
+      case gType of
+        G.TypeNamed _ n      -> (n, ctr)
+        G.TypeList  _ gType' -> flip go (ctr + 1) gType'
+
 groupListWith
  :: (Eq k, Hashable k, Foldable t, Functor t)
  => (v -> k) -> t v -> Map.HashMap k (NE.NonEmpty v)
--- a/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs
+++ b/server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs
@ -10,6 +10,7 @@ module Hasura.RQL.DDL.RemoteRelationship.Validate
 import           Data.Foldable
 import           Hasura.GraphQL.Schema.Remote
 import           Hasura.GraphQL.Parser.Column
+import           Hasura.GraphQL.Utils          (getBaseTyWithNestedLevelsCount)
 import           Hasura.Prelude                hiding (first)
 import           Hasura.RQL.Types
 import           Hasura.SQL.Types
@ -333,20 +334,20 @@ validateType permittedVariables value expectedGType schemaDocument =
        Nothing -> throwError (InvalidVariable variable permittedVariables)
        Just fieldInfo -> do
          namedType <- columnInfoToNamedType fieldInfo
-          assertType (mkGraphQLType namedType) expectedGType
+          isTypeCoercible (mkGraphQLType namedType) expectedGType
    G.VInt {} -> do
      intScalarGType <- (mkGraphQLType <$> mkScalarTy PGInteger)
-      assertType intScalarGType expectedGType
+      isTypeCoercible intScalarGType expectedGType
    G.VFloat {} -> do
      floatScalarGType <- (mkGraphQLType <$> mkScalarTy PGFloat)
-      assertType floatScalarGType expectedGType
+      isTypeCoercible floatScalarGType expectedGType
    G.VBoolean {} -> do
      boolScalarGType <- (mkGraphQLType <$> mkScalarTy PGBoolean)
-      assertType boolScalarGType expectedGType
+      isTypeCoercible boolScalarGType expectedGType
    G.VNull -> throwError NullNotAllowedHere
    G.VString {} -> do
      stringScalarGType <- (mkGraphQLType <$> mkScalarTy PGText)
-      assertType stringScalarGType expectedGType
+      isTypeCoercible stringScalarGType expectedGType
    G.VEnum _ -> throwError UnsupportedEnum
    G.VList values -> do
      case values of
@ -390,23 +391,27 @@ validateType permittedVariables value expectedGType schemaDocument =
        Left _ -> throwError $ InvalidGraphQLName $ toSQLTxt scalarType
        Right s -> pure s

-assertType
+isTypeCoercible
  :: (MonadError ValidationError m)
  => G.GType
  -> G.GType
  -> m ()
-assertType actualType expectedType = do
-  -- check if both are list types or both are named types
-  (when
-     (G.isListType actualType /= G.isListType expectedType)
-     (throwError $ ExpectedTypeButGot expectedType actualType))
-  -- if list type then check over unwrapped type, else check base types
-  if G.isListType actualType
-    then assertType (unwrapGraphQLType actualType) (unwrapGraphQLType expectedType)
-    else (when
-            (G.getBaseType actualType /= G.getBaseType expectedType)
-            (throwError $ ExpectedTypeButGot expectedType actualType))
-  pure ()
+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
+  -- input argument of type '[Int]'
+  -- http://spec.graphql.org/June2018/#sec-Type-System.List
+  let (actualBaseType, actualNestingLevel) = getBaseTyWithNestedLevelsCount actualType
+      (expectedBaseType, expectedNestingLevel) = getBaseTyWithNestedLevelsCount expectedType
+  in
+  if | actualBaseType /= expectedBaseType -> raiseValidationError
+       -- we cannot coerce two types with different nesting levels,
+       -- for example, we cannot coerce [Int] to [[Int]]
+     | (actualNestingLevel == expectedNestingLevel || actualNestingLevel == 0) -> pure ()
+     | otherwise -> raiseValidationError
+     where
+       raiseValidationError = throwError $ ExpectedTypeButGot expectedType actualType

 assertListType :: (MonadError ValidationError m) => G.GType -> m ()
 assertListType actualType =
--- 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
@ -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
+          }
+        }
+      }
+    }
--- 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
@ -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"]
--- 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) {
--- a/server/tests-py/test_remote_relationships.py
+++ b/server/tests-py/test_remote_relationships.py
@ -54,6 +54,9 @@ class TestCreateRemoteRelationship:
        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 @@ class TestExecution:
        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