Gqlgen not compliant with graphql spec for Int type

[roeest]

What happened?

Gqlgen treats the Int graphql type as a golang 64 bit integer. The graphql spec states that the Int graphql type represents a 32 bit integer. This causes issues with clients that assume the underlying type of an integer is 32 bits either by overflowing them or by making them error out altogether.

What did you expect?

Behavior that matches the graphql spec

1. Error when specifying an Int input that overflows 32bit integers
2. Truncate Int results that overflow or even better return an indicative error

Minimal graphql.schema and models to reproduce

Any schema with an Int in an input or a regular type

versions

• go run github.com/99designs/gqlgen version? v0.17.49
• go version? go version go1.22.4 darwin/arm64

[phughes-scwx]

@roeest does updating the gqlgen.yaml models section so that:

models:
  # ...
  Int:
    model:
      - github.com/99designs/gqlgen/graphql.Int32

Make the server spec-compliant, as you see it?

[roeest]

@roeest does updating the gqlgen.yaml models section so that:

models:
  # ...
  Int:
    model:
      - github.com/99designs/gqlgen/graphql.Int32

Make the server spec-compliant, as you see it?

For a greenfield schema this would work, but with some caveats. Mapping Int to graphql.Int32 means your go struct needs to use int32 as its underlying type or alternatively have a resolver function that casts the int value to a int32. If your data has 64 bit integers that are being used beyond their first 32 bits they can potentially be truncated so you'd need to add a Int64 scalar or something to that effect and use it as the equivalent graphql type for each field. You can reuse graphql.Int and graphql.Int64 as mappings for your graphql Int64 scalar FWIW.

The problem is with existing schemas. Making your schema spec compliant results in a breaking change to the api, no matter how you spin it. Returning 32 bit values in apis that returned 64 bit values will truncate the result. Similarly inputs that use 64bit values can error out when being unmarshaled which is also a breaking change. Changing the type of a specific field to a Int64 scalar won't necessarily break the operations, but it is a breaking change in the schema from the client's perspective.

I don't know what the best path forward is here, but gqlgen should at least implement some preventative measures to ensure users aren't shooting themselves in the foot when it comes to spec compliance.

[phughes-scwx]

I am trying to imagine a PR to introduce this here, and what I come up with is one that changes the init behavior and documentation... Including documentation that would specifically point out using Int64 / BigInt scalars. I think existing schemas are SOL on this: using int32 across the resolver interface is pretty much the best / only way to ensure implementors take this into consideration.

Are there any updates that you think should / must be made to either the scalar (Int, Int32, Int64) implementations or the codegen / graphql package to ensure we error correctly? I'm imagining adding a safecast step to Int32 Marshal / Unmarshal... but I feel like I'm missing something maybe.

[phughes-scwx]

Ah I think I see now: we should have the graphql Int map to int32 directly in the resolver when generating, not int.

I think that, in the context of the suggested change to the default yaml setup, we can say that it's not really important to have the inputs map to int32 (since a client that sends that data will receive an error), but it would be useful to have result values map to int32 so that we are certain to handle it in the resolver implementation.

This is a pretty drastic change: I think maybe introducing a configuration setting in the yaml to do this and then advocating for that to become the default as well may be the best route to take...

[roeest]

I think the current marshal/unmarshal methods would work correctly as is, it's more about making sure that the right ones are being used by the mappings in the configuration. I was playing around with a CI step that parsed the configuration and made sure that the mapping for Int and Int64 exist and are correct (only allow mapping Int to graphql.Int32 and vice-versa). I think the default behavior when not specifying a mapping for Int is to add a mapping to the 64 bit types, so that would obviously need to be changed. You'd probably need a flag here to allow people to use the old behavior so as to not break their existing apis.

Built in Int64 scalar support would be nice, similarly to other built in scalars (UUID, Time etc.)

[phughes-scwx]

OK, this above PR actually solves the issue, but is pretty strict. I failed at my attempt to introduce a "partial compliance" mode to the config that allows Int to map to int/int64 for inputs but not for results. Will link to a partially completed version of that in a branch if anyone is interested in helping.

[phughes-scwx]

I think this can be closed given #3409. Anything else you want, @roeest?