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

[RFC] Introduce Strict and Legacy All Variable Usages Are Allowed #1059

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Nov 10, 2023

For the following schema:

type Query {
  sum(numbers:[Int!]!): Int
}

The following query is currently valid:

query Q ($number: Int = 3) {
  sum(numbers: [1, $number, 3])
}

This query will accept the variables {"number": null} and result in a runtime field error when it turns out that null cannot be used in this non-nullable list value position. This was discussed in depth in the December 2022 Secondary EU WG meeting, resulting in this action item graphql/graphql-wg#1337. Timestamped link to the relevant part of the discussion: https://youtu.be/nkPn-F_UBJo?list=PLP1igyLx8foH30_sDnEZnxV_8pYW3SDtb&t=2702

This PR implements the agreed solution: it introduces a stricter version of the All Variable Usages Are Allowed algorithm which forbids a nullable variable from being used in a non-nullable position; and to support existing documents a legacy version of the old algorithm is maintained (basically a copy/paste).

Under the new strict algorithm, the previous query becomes invalid and you'd need to make the variable type non-nullable:

query Q ($number: Int! = 3) {
  sum(numbers: [1, $number, 3])
}

This still allows the variable $number to be omitted, but it does not allow it to be explicitly null.

IMPORTANT NOTE: this new algorithm does not allow an argument/input object field's default value to be used if a variable is used as the value for that argument/input object field. A workaround is to copy the argument/input object field's default value to the variable definition, but this will mean that changes to the argument/input object field's default value will not be reflected by existing queries. I think this is acceptable, since there's no way to leverage the default value of an argument when passing a literal to it either - I'm in favour of literal/variable equivalence where possible, and I think that the benefits of turning this runtime error into a validation error outweigh the costs.

cc @leebyron @mjmahone @IvanGoncharov as they were participants in the discussion above and contributed to this decision.

@benjie benjie added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Nov 10, 2023
Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 800815d
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/654e1c0d565c2b00095cb28b
😎 Deploy Preview https://deploy-preview-1059--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is well thought out!

Comment on lines +1864 to +1865
which would become invalid under the Strict Algorithm, in which case you should
implement the Legacy Algorithm.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"In which case you must implement the Legacy Algorithm until the existing documents are migrated to satisfy the strict algorithm"

Comment on lines +1875 to +1876
Implement this only if you are not implementing
[Legacy All Variable Usages Are Allowed](#sec-Legacy-All-Variable-Usages-Are-Allowed).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure "Implement this only if" is what we want. If you implement legacy then we also want you implementing strict.

Maybe we should have validation warnings? Where if you implement both:

  • if strict validation succeeds, no error
  • if legacy validation succeeds, warn
  • if legacy validation fails, fail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing both would be redundant, wouldn’t it? But yeah, this shouldn’t be a strict rule, probably a “we recommend you implement this only if” instead, from an efficiency POV.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I like the warn idea.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants