Editorial: Fix cases in validation where technically a crash could occur due to non-existent definitions #1180
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
Good call, both of these are covered by separate validation rules and the same issue (e.g. undefined variable) should never produce multiple validation failures.
benjie
added
✏️ Editorial
|
I feel like this is an editorial correction, however since it's in the algorithms I'm going to make it an RFC and we can decide if it's actually editorial during the relevant WG. This is already implemented in GraphQL.js so we could fast-track this to RFC3 IMO. cc @graphql/tsc |
| - Let {variableName} be the name of {variableUsage}. | ||
| - Let {variableDefinition} be the {VariableDefinition} named {variableName} | ||
| defined within {operation}. | ||
| - If no {variableDefinition} can be found, return. |
There was a problem hiding this comment.
Should this be caught before by 5.8.3 All Variable Uses Defined?
There was a problem hiding this comment.
The validation rules don't run in a specific order, they run in parallel / independently. So yes to "is it caught by All Variable Uses Defined?" and no to "before".
There was a problem hiding this comment.
Makes sense. As an implementer, more guidance on the order would be welcome but definitely out of scope for this PR.
3 participants
We have a few cases where in validation rules we rely on a different rule ensuring that the definition for usage exists.
In GraphQL JS we already guard against this in a few places