Scalar query parameter treated as int32

[airomega]

I have an ID that's been generated using Go's Hash32 func

I ran into an issue using the Int type in my schema discovered that uint32 is not supported as Int in Graphql

I introduced a Uint scalar thanks to this post and updated my schema:

type User{
    hash: Uint!
    forename: String
    surname: String
    email: String!
}

scalar Uint

schema {
  query: Query
}

type Query {
  user(hash: Uint!): user
}

When I run the query { user(hash: 3626262620) { forename } }

I get the following error: "message": "graphql: panic occurred: strconv.ParseInt: parsing "3111942731": value out of range"

The error is being thown here after applySelectionSet

I'm hoping that I've made a mistake. However it looks like all integer numbers passed in a query parameter will be treated as an int of size 32 - the surrounding switch uses scanner.Int as a case but has no check on size.

I understand the graphql spec treats Int as a 32 bit int - however this seems like a bug on queries/scalars? Or am I doing something wrong?

[tonyghita]

I think we need to extend the input coercion component to take into account scalar values.

Currently the assumption is that all numbers will be coerced to either Integer or Float, where the corresponding Go values are int32 and float64.

In the meantime, have you considered using an ID scalar type instead? The ID type serializes to String to sidestep this problem. You could then provide the coercion functionality in your application code instead, until the input coercion functionality is extended to support the use case.

type User {
  id: ID!
  forename: String
  surname: String
  email: String!
}

type Query {
  user(id: ID!): User
}

https://facebook.github.io/graphql/draft/#sec-ID

[airomega]

Thanks for the reply.

I did consider using the graphql.ID type - but I'd really rather avoid doing so as its simply the wrong type. I'd be introducing technical debt into my project.

Happy to assist with this if it's an enhancement. Are there any other considerations before updating?

[pavelnikolov]

According to the GraphQL specification/documentation the integers are 32 bit (AFAIK, this is due to the fact that it is mapped to the integers used by JavaScript in the browsers).

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

[airomega]

Hi pavelnikolov and tonyghita

I feel there's been a misunderstanding and that this should not have been closed.

I addressed the integer specification in my initial description:

I understand the graphql spec treats Int as a 32 bit int - however this seems like a bug on queries/scalars?

This is not an Integer! It's a custom Scalar. To treat all numbers as 32 bit is quite course - especially as it locks down the introduction of larger custom types.

I totally agree the Int types should be restricted to 32 bit - however custom types should not have this restriction.

[pavelnikolov]

This is a very interesting problem... How are you going to pass a number greater than 32bit from a browser? This is not possible, right? Even if we change the backend to support such types it doesn't make sense... In your custom resolver you can always cast the number to whatever type you want. Unless, I'm missing something obvious... 🤔

[airomega]

From the Note at the bottom of the link you posted above

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.

It seems my request is in keeping with the graphql spec.

We pass the requests in the request body which is treated as text (or bytes). The number coercion Dictionary takes places at the receiver. There's no issue in the transport of a larger number

[pavelnikolov]

OK, I understand. I'll reopen the issue, then. Sorry fro the misunderstanding and thank you for your explanation. PR would be welcome.

[airomega]

Great - happy to help

[gotomgo]

Is this not an issue of the UnmarshalGraphQL for a custom scaler type not being called? Regardless of your interpretation of JSON and int32, if I have a custom scaler type that has marshalers, I always want it to be called. In my case, custom marshaling is invoked only if the value is passed as a string, otherwise it is not called and falls thru to BasicLit validation where it barfs

[gotomgo]

The issue being that we are pre custom marshaling and dealing with very basic types (Json types), then the scanner.Int handling has to be way more lenient:

I don't like this code but it works. Note the use of 0 for the radix, instead of 10. This will allow the parser to look for common radix prefixes and do the right thing. Just cuz you don't pass your #'s via Json as 0xFF00 doesn't mean it should not be allowed. Also, none of this code is covered by unit tests, and given how fundamental it is, it should be.

case scanner.Int:
	value, err := strconv.ParseInt(lit.Text, 0, 64)
	if err != nil {
		value, err := strconv.ParseUint(lit.Text, 0, 64)
		if err != nil {
			panic(err)
		}
		return value
	}

	if (value <= math.MaxInt32) && (value >= math.MinInt32) {
		return int32(value)
	}

	return value

[kolanos]

@pavelnikolov This appears to be a bug more than an enhancement? If one defines a custom scalar, it will still overflow because the int32 casting occurs regardless.

[pavelnikolov]

@kolanos Pull requests are welcome

[suessflorian]

I see @gotomgo's approach to be totally valid, that is simply dropping int32 restrictions on deserialisation of literal scanner.Int's https://github.com/graph-gophers/graphql-go/blob/446a2dd13dd5acd3dff51e2978b914df958c194c/types/value.go#L31 Inferring radix from input makes total sense too, no reason for us to restrict radix what a great idea.

To my understanding though, there will need to be two separate changes needed as @pavelnikolov earlier comment

According to the GraphQL specification/documentation the integers are 32 bit (AFAIK, this is due to the fact that it is mapped to the integers used by JavaScript in the browsers).

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

Primitive Graphql Int should be restricted to int32 sizes, so will investigate where to move this validation.

[pavelnikolov]

Are you still looking into this issue? A set of failing unit tests would be a good start if you post such an issue.