Skip to content
New issue

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

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

Already on GitHub? # to your account

Statement about truncating fractional values wrong? #312

Closed
andimarek opened this issue May 23, 2017 · 6 comments
Closed

Statement about truncating fractional values wrong? #312

andimarek opened this issue May 23, 2017 · 6 comments

Comments

@andimarek
Copy link
Contributor

In the section about scalars: (https://github.com/facebook/graphql/blob/master/spec/Section%203%20--%20Type%20System.md#scalars)

there is the following statement:

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.

This seems contradictory to this Issue: graphql/graphql-js#827, where it was cleared that fractions should not be truncated.

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented May 23, 2017

As far as I understood, the section of the spec you are quoting describes the output coercion. In this issue, I only touched on the input value coercion. I see these two as separate sets of rules. Input coercion is very strict where output coercion is more a hint to server implementers which only requires producing type-compliant literal regardless or the internal server representation of this scalar value. (though I would prefer to make these hints more strict and recommend to raise an error in case of booleanint or stringint coercion)

@andimarek
Copy link
Contributor Author

Mhh ... not sure I can follow you: result coercion (= output coercion) is serialize in graphlq-js and input coercion is parseValue. Right?

Both are implement the same way:
https://github.com/graphql/graphql-js/blob/master/src/type/scalars.js#L48

Does this mean the ref. impl is still wrong and should truncate the value for serialize?

@OlegIlyenko
Copy link
Contributor

This is a very good point! TBH, I was also a bit confused before when I saw that serialize and parseValue use the same function under the hood.

Though I wouldn't say that graphql-js is not spec-compliant here. Spec doesn't say that if developer returns a string from a resolve function, then execution engine must coerce it to int if there is a valid number inside of this string. I this case following rule applies, I think:

If the server encounters some value that cannot be reasonably coerced to an Int, then it must raise a field error.

I guess it comes back to a definition of reasonable :)

@andimarek
Copy link
Contributor Author

ok ... so the implementation of serialize (result coercion) could differ from parseValue(input coercion): the first could throw away the fraction part, but for input coercion it is not allowed.
Right?

@OlegIlyenko
Copy link
Contributor

At least this is how I understand it :) Assuming that it is possible to return values other than Int from a resolve function. (in sangria, for instance, Int scalar type is defined in terms of scala.Int. This means that it will not even compile in this case)

@andimarek
Copy link
Contributor Author

After thinking more about it: Input coercion in the spec is actually two aspects: Input as literal (implement as parseLiteral) and Input as Variable (parseValue) .

But this is not always clearly communicated: for example in the Int section about "Input coercion" there is only specified what should happen with literals. But above in the general section about "Input coercion" it is dealt with "Input as variable":

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.

This means parseLiteral for Int should only accept Ints, but parseValue should probably be more flexible and also accepts 1.0 as Input. This is the reason (I guess) that parseValue is equal to serialize (in graphql-js at least).

I think the spec could more clearer and distinguish more between the two different kinds of "Input coercion".

leebyron added a commit that referenced this issue Apr 30, 2018
This edits the "result coercion" section of the scalar types to make it clear that data loss is to be avoided and suggests that internal values of the correct type are the ideal scenario.

Fixes #312
leebyron added a commit that referenced this issue Apr 30, 2018
This edits the "result coercion" section of the scalar types to make it clear that data loss is to be avoided and suggests that internal values of the correct type are the ideal scenario.

Fixes #312
leebyron added a commit that referenced this issue Apr 30, 2018
This edits the "result coercion" section of the scalar types to make it clear that data loss is to be avoided and suggests that internal values of the correct type are the ideal scenario.

Fixes #312
leebyron added a commit that referenced this issue Apr 30, 2018
This edits the "result coercion" section of the scalar types to make it clear that data loss is to be avoided and suggests that internal values of the correct type are the ideal scenario.

Fixes #312
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants