Int input value coercion from floating point JSON literal value

[OlegIlyenko]

According to the spec (https://facebook.github.io/graphql/#sec-Scalars):

Since some common serializations (ex. JSON) do not discriminate between integer and floating‐point values, they are interpreted as an integer input value if they have an empty fractional part (ex. 1.0) and otherwise as floating‐point input value.

Based on this description, when I define a value in JSON for a variable of an Int type:

• 123.0 must be considered a valid value and coerce to 123

• 123.456 must be considered an invalid value and should result in error based on Int input value coercion rules (https://facebook.github.io/graphql/#sec-Int):

  When expected as an input type, only integer input values are accepted. All other input values, including strings with numeric content, must raise a query error indicating an incorrect type.

Do I understand it correctly?

I discovered that reference implementation allows 123.456 value for a variable of Int type which does not comply to the mentioned parts of the spec. Here is the line that successfully drops fraction digits:

https://github.com/graphql/graphql-js/blob/master/src/type/scalars.js#L30

[wincent]

I think you've read it correctly, @OlegIlyenko, and that the reference implementation is probably wrong (rather than coercing input types to Int, it should strictly observe the rule that "only integer values are accepted"). I haven't yet checked whether the internal HHVM implementation at FB does this; if it does not, then maybe we should instead consider the possibility that the spec is wrong and the implementation is right.

[OlegIlyenko]

then maybe we should instead consider the possibility that the spec is wrong and the implementation is right.

This would be definitely an important thing to consider. Though as the first step I think it would helpful to look on the issue on itself, and brainstorm on the best definition.

At the moment I feel that the way it's specified in the spec is quite good. Allowing server to coerce 123.45 to Int would involve a data loss which I find quite dangerous and unsettling. In many cases it's not that big deal, but the fact that client sends this data (0.45 in my example) indicates that it assumes/expects that data would be considered on the server side. For example, I can imagine scenario where an input field represents monetary value. At the beginning, API exposed it as a Float this 2 fraction digits, but after some refactorings it became an Int (cent amount). This scenario probably goes against best practices (this is definitely a breaking change), but I still think it's helpful consider the worst case scenario. After the refactoring, 123.45 is still accepted by the server, but the value interpretation it completely off (1.23 $ instead if 123.45 $).

[wincent]

Allowing server to coerce 123.45 to Int would involve a data loss which I find quite dangerous and unsettling.

I think you're right, and that's my sense too (that's why the spec text seems so "intentional" about it).

[mjmahone]

The section immediately above the Int section in the spec discusses coercing a floating point value to an int as it's "canonical" example. We should probably also make sure the spec agrees with itself.

[wincent]

@mjmahone: Are you referring to "Result Coercion" section under 3.1.1 Scalars?

For example, a GraphQL server could be preparing a field with the scalar type Int and encounter a floating‐point number. Since the server must not break the contract by yielding a non‐integer, the server should truncate the fractional value and only yield the integer value.

I don't think that's in conflict, as the thing we're talking about here is "Input Coercion".

[leebyron]

I agree, @OlegIlyenko - the spec language is nicer than what's implemented here. I've put up a PR if we decide to fix this in the library vs proposing a change to spec.