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

LongType doesn't support incoming String values #1162

Closed
jakobmerrild opened this issue Dec 13, 2024 · 11 comments
Closed

LongType doesn't support incoming String values #1162

jakobmerrild opened this issue Dec 13, 2024 · 11 comments

Comments

@jakobmerrild
Copy link
Contributor

Under the assumption that most consumers of the GraphQL schema specified by Sangria is running in JavaScript it seems prudent to allow input coercion from String for the Long scalar type, as JavaScript has no way of precisely representing the full spectrum using it's number data type.

I.e. given the following schema

type Query {
  echo(number: Long!): Long!
}

then the following payload should be accepted

{
  "query": "query EchoLong($number: Long!) { echo(number: $number) }",
  "operationName": "EchoLong",
  "variables": { "number": "123456" }
}
@yanns
Copy link
Contributor

yanns commented Dec 13, 2024

Thanks for the issue and the PR!
To help me make a decision, do you know if the GraphQL specs mention this?
Are you also aware of how other libraries are handling this?

@jakobmerrild
Copy link
Contributor Author

According to the latest spec services that specify custom scalars should include the @specifiedBy url, see https://spec.graphql.org/October2021/#sec-Scalars.Custom-Scalars

However, I don't know if Sangria currently supports that annotation?

W.r.t coercion it is up to the implementor to define what are reasonable rules so long as they are performed without losing precision (if I'm reading the spec correctly)

@jakobmerrild
Copy link
Contributor Author

According to this announcement you can contribute your custom scalars, though given that the announcement is almost 2 years old and there are no new contributions by the community since then, it might not really have taken hold as an idea

@yanns
Copy link
Contributor

yanns commented Dec 13, 2024

Your issue is about input coercion right? https://spec.graphql.org/October2021/#sec-Scalars.Input-Coercion

What you mentioned before is not the same:

Since this coercion behavior is not observable to clients of the GraphQL service, the precise rules of coercion are left to the implementation.

This is for example if the field is a String, and internally it's an Integer, then "the precise rules of coercion are left to the implementation. ".

I assume you're having this issue with a real-world case. Out of curiosity, can you provide more details, like which JavaScript library is used?

@yanns
Copy link
Contributor

yanns commented Dec 13, 2024

Input coercion for the scalar Int do not accept String values:
https://spec.graphql.org/October2021/#sec-Int.Input-Coercion

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

@jakobmerrild
Copy link
Contributor Author

We do indeed have issues with a real-world case.
Using sangria 4.3.2 and sangria-circe 1.3.1 on the server and Apollo Client with Typescript on the consumer side we are running into issues because the client generates the Scalar as something that is backed by a string, but the server doesn't accept string values.

@yanns
Copy link
Contributor

yanns commented Dec 13, 2024

I understand your pain, and that there's no easy fix without touching sangria.
But I'll need a bit of time to think about it, as allowing Strings by default does not seem spec compliant.

@jakobmerrild
Copy link
Contributor Author

Which part of the spec for custom Scalars do you see this breaking? The spec specifically notes that the built-in Int Scalar should only be used for values from -2^32 to 2^32-1 and that values outside this range should be using String or a custom Scalar as not all platforms can handle 64-bit integers, according to the note

@jakobmerrild
Copy link
Contributor Author

There's also this old issue on the graphql-spec itself. Perhaps the better way forward is to take up the call for a champion and introduce the Long type into the spec now that JavaScript supports BigInt as noted in this comment.

@yanns
Copy link
Contributor

yanns commented Dec 14, 2024

There's also this old issue on the graphql-spec itself. Perhaps the better way forward is to take up the call for a champion and introduce the Long type into the spec now that JavaScript supports BigInt as noted in this comment.

Yes that would be nice.
Do you want to volunteer?

In the meantime, I guess that your point about using string is adequate to this part of the spec yes:

Numeric integer values larger than 32-bit should either use String or a custom-defined Scalar type, as not all platforms and transports support encoding integer numbers larger than 32-bit.

@yanns
Copy link
Contributor

yanns commented Dec 16, 2024

@yanns yanns closed this as completed Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants