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

codingkarthik · 2020-08-17T10:33:22Z

The issue arises when the user tries to make a remote relationship type, joining a singleton type with an array type. For example, joining Int with [Int].

Description

Changelog

CHANGELOG.md is updated with user-facing content relevant to this PR. If no changelog is required, then add the no-changelog-required label.

Affected components

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

No
Yes
- Updated docs with SQL for downgrading the catalog

Metadata

Does this PR add a new Metadata feature?

No
Yes
- Does run_sql auto manages the new metadata through schema diffing?
  - Yes
  - Not required
- Does run_sql auto manages the definitions of metadata on renaming?
  - Yes
  - Not required
- Does export_metadata/replace_metadata supports the new metadata added?
  - Yes
  - Not required

GraphQL

No new GraphQL schema is generated
New GraphQL schema is being generated:
- New types and typenames are correlated

Breaking changes

…rray type

hasura-bot · 2020-08-17T11:00:11Z

Review app for commit 22b4b28 deployed to Heroku: https://hge-ci-pull-5603.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5603-22b4b285

jberryman · 2020-08-17T21:33:30Z

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)


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

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.

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.

jberryman · 2020-08-17T21:36:36Z

server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs

-    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


This comment is great. While you are here can you give assertType a better name? I guess what we're actually doing here is asserting that actualType is coercible to expectedType?

Also, is this deficient in other ways according to the spec?

I guess what we're actually doing here is asserting that actualType is coercible to expectedType?

Yes, that's what we're doing. But I'm not sure, what can be a better name here? Perhaps, isTypeCoercible?

Also, is this deficient in other ways according to the spec?

Yes, it was. My patch would have allowed to make a remote relationship joining types [[Int]] and [[[Int]]], which is illegal, as specified in the GraphQL spec.

I have fixed this in the next commit

isTypeCoercible sounds good to me!

netlify · 2020-08-19T08:24:35Z

Deploy preview for hasura-docs ready!

Built with commit a34913f

https://deploy-preview-5603--hasura-docs.netlify.app

hasura-bot · 2020-08-19T08:39:32Z

Review app for commit 32766d2 deployed to Heroku: https://hge-ci-pull-5603.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5603-32766d2e

hasura-bot · 2020-08-20T06:29:01Z

Review app for commit 0a34d58 deployed to Heroku: https://hge-ci-pull-5603.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5603-0a34d58d

CHANGELOG.md

...eries/remote_schemas/remote_relationships/setup_remote_rel_joining_singleton_with_array.yaml

Co-authored-by: Tirumarai Selvan <tiru@hasura.io>

tirumaraiselvan · 2020-08-21T07:29:56Z

server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs

-  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


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

tirumaraiselvan · 2020-08-21T07:31:55Z

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: allow remote relationships joining `type` column with `[type]` input argument as spec allows this coercion (fixes #5133)


oops, can you push this to Next release section also?

hasura-bot · 2020-08-21T07:36:32Z

Review app for commit a3704b4 deployed to Heroku: https://hge-ci-pull-5603.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5603-a3704b4c

rakeshkky · 2020-08-21T08:27:50Z

server/src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs

+       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)


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.

hasura-bot · 2020-08-21T08:40:03Z

Review app for commit d4e2955 deployed to Heroku: https://hge-ci-pull-5603.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5603-d4e2955e

hasura-bot · 2020-08-21T11:08:17Z

Review app for commit 22d563c deployed to Heroku: https://hge-ci-pull-5603.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5603-22d563c3

hasura-bot · 2020-08-26T08:06:38Z

Review app for commit 95b0808 deployed to Heroku: https://hge-ci-pull-5603.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5603-95b08086

hasura-bot · 2020-08-27T02:44:07Z

Review app https://hge-ci-pull-5603.herokuapp.com is deleted

…-validation-bug-5133 hasura#5603

codingkarthik added 3 commits August 17, 2020 14:51

server: don't throw validation error when joining singleton type to a…

bf2de78

…rray type

reword a comment

87b1b57

add changelog

22b4b28

codingkarthik requested a review from a team as a code owner August 17, 2020 10:33

codingkarthik requested review from rakeshkky and jberryman August 17, 2020 10:33

jberryman reviewed Aug 17, 2020

View reviewed changes

codingkarthik added 2 commits August 19, 2020 13:41

improve the assertType check while validating remote relationship

8f2a780

show the GraphQL type instead of just the base type while throwing error

32766d2

refactor assertType to isTypeCoercible

0a34d58

codingkarthik requested a review from jberryman August 20, 2020 06:17

jberryman approved these changes Aug 20, 2020

View reviewed changes

codingkarthik requested a review from tirumaraiselvan August 21, 2020 04:42

tirumaraiselvan reviewed Aug 21, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

tirumaraiselvan reviewed Aug 21, 2020

View reviewed changes

...eries/remote_schemas/remote_relationships/setup_remote_rel_joining_singleton_with_array.yaml Show resolved Hide resolved

Update CHANGELOG.md

a3704b4

Co-authored-by: Tirumarai Selvan <tiru@hasura.io>

tirumaraiselvan reviewed Aug 21, 2020

View reviewed changes

tirumaraiselvan approved these changes Aug 21, 2020

View reviewed changes

tirumaraiselvan reviewed Aug 21, 2020

View reviewed changes

update changelog

a34913f

codingkarthik added the s/do-not-merge Do not merge this pull request to master label Aug 21, 2020

Merge branch 'master' into remote-relationship-validation-bug-5133

d4e2955

rakeshkky reviewed Aug 21, 2020

View reviewed changes

codingkarthik mentioned this pull request Aug 21, 2020

[v1.3.0-beta.2] Error remote relationship: cannot validate remote relationship because expected type \"Int\" but got \"Int\" #5133

Closed

use multiwayif instead of nested if-else

22d563c

Merge branch 'master' into remote-relationship-validation-bug-5133

95b0808

codingkarthik removed the s/do-not-merge Do not merge this pull request to master label Aug 26, 2020

jberryman approved these changes Aug 26, 2020

View reviewed changes

codingkarthik added the auto-update-auto-merge label Aug 27, 2020

kodiakhq bot merged commit 15794ad into hasura:master Aug 27, 2020

tirumaraiselvan added this to the v1.3.3 milestone Nov 3, 2020

codingkarthik pushed a commit to codingkarthik/graphql-engine that referenced this pull request Nov 6, 2020

Merge pull request hasura#5603 from codingkarthik/remote-relationship…

746fc20

…-validation-bug-5133 hasura#5603

This was referenced Oct 10, 2024

[Snyk] Security upgrade express from 4.16.4 to 4.21.1 Allymahmoud/graphql-engine#835

Open

[Snyk] Security upgrade express from 4.16.4 to 4.21.1 Allymahmoud/graphql-engine#836

Open

sumodgeorge mentioned this pull request Oct 10, 2024

[Snyk] Security upgrade express from 4.16.3 to 4.21.1 sumodgeorge/graphql-engine#155

Open

Allymahmoud mentioned this pull request Oct 11, 2024

[Snyk] Security upgrade express from 4.18.1 to 4.21.1 Allymahmoud/graphql-engine#837

Open

sumodgeorge mentioned this pull request Oct 12, 2024

[Snyk] Security upgrade express from 4.16.3 to 4.21.1 sumodgeorge/graphql-engine#156

Open

OKEAMAH mentioned this pull request Oct 12, 2024

[Snyk] Security upgrade express from 4.19.2 to 4.21.1 OKEAMAH/graphql-engine#81

Open

This was referenced Oct 14, 2024

[Snyk] Security upgrade express from 4.16.3 to 4.21.1 Allymahmoud/graphql-engine#842

Open

[Snyk] Security upgrade express from 4.16.3 to 4.21.1 Allymahmoud/graphql-engine#845

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

codingkarthik commented Aug 17, 2020 •

edited by tirumaraiselvan

Loading

hasura-bot commented Aug 17, 2020

jberryman Aug 17, 2020

codingkarthik Aug 19, 2020

codingkarthik Aug 19, 2020

jberryman Aug 17, 2020

jberryman Aug 17, 2020

codingkarthik Aug 19, 2020

codingkarthik Aug 19, 2020

jberryman Aug 19, 2020

netlify bot commented Aug 19, 2020 •

edited

Loading

hasura-bot commented Aug 19, 2020

hasura-bot commented Aug 20, 2020

tirumaraiselvan Aug 21, 2020

tirumaraiselvan Aug 21, 2020

codingkarthik Aug 21, 2020

hasura-bot commented Aug 21, 2020

rakeshkky Aug 21, 2020

hasura-bot commented Aug 21, 2020

hasura-bot commented Aug 21, 2020

hasura-bot commented Aug 26, 2020

hasura-bot commented Aug 27, 2020

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

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

Conversation

codingkarthik commented Aug 17, 2020 • edited by tirumaraiselvan Loading

Description

Changelog

Affected components

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Metadata

GraphQL

Breaking changes

hasura-bot commented Aug 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Aug 19, 2020 • edited Loading

hasura-bot commented Aug 19, 2020

hasura-bot commented Aug 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasura-bot commented Aug 21, 2020

Choose a reason for hiding this comment

hasura-bot commented Aug 21, 2020

hasura-bot commented Aug 21, 2020

hasura-bot commented Aug 26, 2020

hasura-bot commented Aug 27, 2020

codingkarthik commented Aug 17, 2020 •

edited by tirumaraiselvan

Loading

netlify bot commented Aug 19, 2020 •

edited

Loading