Skip to content

Returning too large of a number from Int resolver results in hard to debug errors #391

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.

Sign up for GitHub

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

Jump to bottom
Closed
stubailo opened this issue May 19, 2016 · 3 comments
Closed

Returning too large of a number from Int resolver results in hard to debug errors #391

stubailo opened this issue May 19, 2016 · 3 comments

Comments

@stubailo
Copy link

If I have a field that is supposed to return GraphQLInt, then there are two hard-to-debug things that can happen if I return a number that's not within the allowed range, for example 1463692053071 (a timestamp):

  1. If the field is optional, then I simply get null on the client with no errors at all
  2. If the field is required, then I get the error: Cannot return null for non-nullable field Entry.createdAt. This is extra misleading since I didn't actually return null - I returned a Number that was simply too large.

Sounds like this can just be fixed by adding an error in the coerceInt function instead of silently returning null.
@msakrejda
Copy link

I ran into this same problem and commented on another related issue. As I point out there, I think that behavior is in violation of the spec:

GraphQL servers should coerce non‐int raw values to Int when possible otherwise they must raise a field error. Examples of this may include returning 1 for the floating‐point number 1.0, or 2 for the string "2".

(emphasis added)

Would you accept a patch fixing this behavior?

leebyron added a commit that referenced this issue Jul 20, 2016
@leebyron
[FIX] Throw field errors when failing to coerce Int/Float.
78538bc 
As illustrated by #391, when a value is provided for a field of type Int which cannot be represented by Int (e.g. a 64bit value is provided to the 32bit Int type), then the spec claims a field-error should be raised however currently `null` is returned directly instead.

Spec: https://facebook.github.io/graphql/#sec-Int

This updates to throw meaningful error messages when invalid Int and Float values cannot be coerced without losing information.
@leebyron
Copy link
Contributor

Hey, I really apologize for letting this issue sit. You're absolutely correct @uhoh-itsmaciek that this is a real bug relative to the correct spec language.

I've attached a PR to fix it.

@leebyron leebyron mentioned this issue Jul 20, 2016
Merged
leebyron added a commit that referenced this issue Jul 20, 2016
@leebyron
[FIX] Throw field errors when failing to coerce Int/Float. (#444)
08a00aa 
As illustrated by #391, when a value is provided for a field of type Int which cannot be represented by Int (e.g. a 64bit value is provided to the 32bit Int type), then the spec claims a field-error should be raised however currently `null` is returned directly instead.

Spec: https://facebook.github.io/graphql/#sec-Int

This updates to throw meaningful error messages when invalid Int and Float values cannot be coerced without losing information.
@leebyron leebyron mentioned this issue Jul 20, 2016
Merged
@msakrejda
Copy link

Awesome, thanks @leebyron!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@leebyron @msakrejda @stubailo