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

[Breaking] fix(GRAPHQL): fix input coercing for integers and make them GraphQL spec complaint. #7584

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented Mar 16, 2021

GraphQL spec doesn't allow any value apart from integer to be passed to a variable of type Int. So we are disallowing strings to be passed as Int variables. Int64 is our own scalar data type, so we continue to accept string and Integer values for it.
This PR allows passing strings to Int64 values when given directly in query/mutations and disallows string to be passed to Int in variables.

So we will have below final behavior:

`Int32` will accept only `Integer` values, and otherwise will give a coercion error.
`Int64` will accept `Integer` as well as `String` value and will return an error if any other value. And if we can only pass Integer in a string like "1234" otherwise it will give a coercion error.

Int coercion rules in GraphQL spec:
https://spec.graphql.org/June2018/#sec-Int


This change is Reviewable

disallow string --> Int32 variable coercing
@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Mar 16, 2021
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

:lgtm:

Nice change in test.

In the PR title and description, we should correct some typos:

  • Graphql -> GraphQL
  • specs -> spec
  • apart from Int to be -> apart from integer to be (basically, Int anythwere -> integer)
  • variable of type Int32 -> variable of type Int (basically, Int32 anywhere -> Int)

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

@JatinDev543 JatinDev543 changed the title [Breaking] fix(GRAPHQL): fix input coercing for integers and make them graphql specs complaint. [Breaking] fix(GRAPHQL): fix input coercing for integers and make them GraphQL spec complaint. Mar 16, 2021
@JatinDev543 JatinDev543 merged commit 8691809 into release/v21.03 Mar 16, 2021
@JatinDev543 JatinDev543 deleted the jatin/GRAPHQL-1081 branch March 16, 2021 08:28
aman-bansal pushed a commit that referenced this pull request Mar 25, 2021
GraphQL spec doesn't allow any value apart from integer to be passed to a variable of type Int. So we are disallowing strings to be passed as Int variables. Int64 is our own scalar data type, so we continue to accept string and Integer values for it.
This PR allows passing strings to Int64 values when given directly in query/mutations and disallows string to be passed to Int in variables.

So we will have below final behavior:

`Int32` will accept only `Integer` values, and otherwise will give a coercion error.
`Int64` will accept `Integer` as well as `String` value and will return an error if any other value. And if we can only pass Integer in a string like "1234" otherwise it will give a coercion error.
Int coercion rules in GraphQL spec:
https://spec.graphql.org/June2018/#sec-Int
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

2 participants