Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server: Don't throw validation error while creating remote relationship joining singular type with array type #5603

Merged
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really close 5133? I don't see any mention of list types there..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does. The issue doesn't mention anything about the list type, because the error message thrown is incorrect. Ideally, the error message should have been

Error remote relationship: cannot validate remote relationship because expected type \"[Int]\" but got \"Int\"

instead of

Error remote relationship: cannot validate remote relationship because expected type \"Int\" but got \"Int\"

The reason the list notation is omitted from the error message because of this line

This PR solves that issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message can occur in another case too, while joining an [Int] type to a Int type, where the validation is justified but the error message will be the same

Error remote relationship: cannot validate remote relationship because expected type \"Int\" but got \"Int\"

which is not very helpful.

I'll fix the error message.

codingkarthik marked this conversation as resolved.
Show resolved Hide resolved
- 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)
Expand Down
10 changes: 10 additions & 0 deletions server/src-lib/Hasura/GraphQL/Utils.hs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ module Hasura.GraphQL.Utils
, unwrapTy
, simpleGraphQLQuery
, jsonValueToGValue
, getBaseTyWithNestedLevelsCount
) where

import Hasura.Prelude
Expand Down Expand Up @@ -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
Expand Down
44 changes: 25 additions & 19 deletions server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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 _ ->
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -405,19 +405,25 @@ 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 = 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)))
pure ()
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think technically "singleton" type is called named type but we can fix this comment later.

-- 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
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify this code by using MultiwayIf

if | actualBaseType /= expectedBaseType -> raiseValidationError
   | actualNestingLevel /= expectedNestingLevel && actualNestingLevel /= 0 -> raiseValidationError
   | otherwise -> pure ()
   where
     raiseValidationError = Failure $ pure $ ExpectedTypeButGot expectedType actualType

Of course with appropriate comments.


assertListType :: G.GType -> Validation (NonEmpty ValidationError) ()
assertListType actualType =
Expand Down
Original file line number Diff line number Diff line change
@@ -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
}
}
}
}
Original file line number Diff line number Diff line change
@@ -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"]
tirumaraiselvan marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 8 additions & 0 deletions server/tests-py/remote_schemas/nodejs/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 8 additions & 0 deletions server/tests-py/test_remote_relationships.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down