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

Update gqlgen init template to default to spec-compliant Int #3409

Merged
merged 20 commits into from
Dec 7, 2024

Conversation

phughes-scwx
Copy link
Contributor

@phughes-scwx phughes-scwx commented Dec 5, 2024

Partial solution to #3214.*

As described in the GraphQL spec, pretty clearly, the built in Int type must be a 32-bit integer:

The Int scalar type represents a signed 32-bit numeric non-fractional value. Response formats that support a 32-bit integer or a number type should use that type to represent this scalar.

[...]

Note
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.

Our solution is not to enforce compliance, but instead default to a compliant setup in the gqlgen init configuration, and to support approaching schemas in a compliant way with the introduction of a new Int64 scalar.

This PR includes the following updates:

  • Add custom scalar built-in helper for Int64.
  • Update init-template for gqlgen.yaml:
    • Update Int to only ever map to the int32 helper.
    • Call out Int64 and reasoning.
    • Also: align with new options found in config docs.
    • Also: add some explanations for confusing topics like "layout."
  • Update docs to support the reasoning for Int -> int32 change (when using gqlgen init).
  • Add tests for the updated config codegen.
    • Update the coverage ignore list to include the generated files. NOTE: being more mindful about ignoring generated files in codegen/testserver seeems to have caused coverage to skyrocket.
  • Add safe-cast helpers to graphql.Int32 unmarshaling, with tests.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

* – I would love to actually do the below as well as the above, and even have tests and other code written for it, but I was stymied by the implementation. If you think this is worthwhile and/or necessary to add this option to accept the above, I would love to create a PR with the state of my effort and get some help / input on how to do it.

TLDR; the above solves the problem, but we may want to add the following work to make it nicer to use.

A more complete solution I envisioned would include another piece of configuration:

# Optional: allow resolvers use Go int for input values when mapping to GraphQL
# Int instead of int32. This is useful when you map Int to int32, but prefer to
# accept ints as inputs for simplicity. This comes in to play when you bind models
# to Int that communicate int32 at a higher priority than int or int64.
skip_int32_bind_for_inputs: true

The concept here is that most Go APIs communicate via the int type, and it will be an annoyance to users to have resolvers and models all default to int32 all the time. This annoyance is important for data that the GraphQL API produces: users should ensure that the data they send conforms to the spec, and use int32 specifically. It also tells them to not use the Int scalar for communicating integers that might overflow 32-bits: instead they can simply use the Int64 (or some other "BigInt") scalar.

However, there is no need for us to enforce this part of the spec on data we consume. So, to be nice to the user (and keep them from just binding Int to graphql.Int as a quick and dirty fix), I think it would be best to allow them to bind Int to graphql.Int (or another int-communicating model) for input model fields and resolver arguments while using graphql.Int32 (or another) for objects, interfaces, enums, resolver result values, etc.

In this solution, the default init template for gqlgen.yaml would be:

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

This setup could be called "compliant config" as opposed to "strict compliant" / "non-compliant" configs.

@coveralls
Copy link

coveralls commented Dec 5, 2024

Coverage Status

coverage: 74.166% (+14.4%) from 59.722%
when pulling bebbcf4 on phughes-scwx:issue-3214-reduced
into ea8b813 on 99designs:master.

@phughes-scwx phughes-scwx marked this pull request as ready for review December 6, 2024 15:56
@phughes-scwx
Copy link
Contributor Author

@StevenACoffman @roeest

@StevenACoffman
Copy link
Collaborator

So I'm looking for the GitHub review "honk and throw money" button, but having trouble finding it. This is great!

The skip_int32_bind_for_inputs proposal (not in this PR) gives me mixed emotions.

Being lax in accepting int64 for int would maybe help people to ease into spec compliance when they upgrade.

While it is less harsh, it is also maybe less straightforward and getting errors only on send but not receive could make it harder to figure out what is happening. I don't feel strongly, and the fact that it is opt-in is nice. It's also not a ton of extra code to maintain.

@StevenACoffman StevenACoffman merged commit b2ed608 into 99designs:master Dec 7, 2024
17 checks passed
@StevenACoffman
Copy link
Collaborator

@phughes-scwx Hey, sorry for merging this if you already worked on the comment tweaks.

I noticed that there's another large PR that has some conflicts with this one, and I wanted to get this in before that one.

I hugely appreciate you doing the yeoman work of updating our docs and getting us closer to spec compliance!

Are you able to make some parallel safecast tweaks to https://github.com/99designs/gqlgen/blob/master/graphql/uint.go ?

@phughes-scwx
Copy link
Contributor Author

Uh no problem. I'll add your suggested tweaks and the uint cast in a follow up.

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

Successfully merging this pull request may close these issues.

3 participants