move variable position validation for OneOf to VariablesInAllowedPositionRule
#4194
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
yaacovCR
force-pushed
the
one-of-tweaks
branch
from
8ccde45
to
cb33ff1
Compare
yaacovCR
changed the title
VariablesInAllowedPositionRule
yaacovCR
changed the title
VariablesInAllowedPositionRuleVariablesInAllowedPositionRule
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TLDR => let's consider moving validation of variable positions with respect to one of into the
VariablesInAllowedPositionRule, i.e. out of theValuesOfCorrectTypeRule.This work was pulled out of the work rebasing the Default Value Coercion/Validation PR stack at #3814 on the OneOf and Experimental Fragment Variables changes.
In that PR stack (work originally done by @leebyron within #3049), Lee extracts the validation done by the
ValuesOfCorrectTypeRuleinto avalidateInputLiteral()utility that can be called at validation-time or at run-time. At validation time,validateInputLiteral()validates statically, is not supplied variables or their types, and does not validate variables, while at run-time it is supplied variables and does validate them.With OneOf Input Objects, we have a situation in which Input Objects will define technically nullable/optional input fields, but the addition of the
@oneOfdirective tells the validator/executor that we have to supply exactly one of these fields (and it cannot be given the valuenull). In essence, we are saying that when@oneOfis applied, the fields of the input object are no longer technically nullable positions, although they are still optional. This was for a combination of type reasoning, backwards compatibility, and simplicity, working overall within the constraints of GraphQL where only nullable fields are optional.So, to validate variable usage with OneOf Input Object, we need to make sure that a variable with a nullable type is not allowed to show up in a field position on a OneOf Input Object. Where should this be done? Currently, this is done within the
ValuesOfCorrectTypeRule, which for this purpose was modified to collect the operation variable definitions. @leebyron 's changes in the abovementioned stack extracts this tovalidateInputLiteral(), where we run into a problem, we don't have access to the operation (or experimental fragment variables)! Lee's work preserving variable sources and definitions organizes the definitions by source, so if we are analyzing statically, we don't have the source or the definitions.There are two paths forwards.
One idea is to modify Lee's work, and split the definitions from the sources, and supply them to
validateInputLiteral()even when working statically, which complicates the signature of a number of functions.What if we take a step back, and ask ourselves if we should have really modified
ValuesOfCorrectTypeRuleto collect all those operation variable definitions? If we move this bit toVariablesInAllowedPositionRule, then we avoid the entire complexity. That's what this PR does.How did this happen anyway? Shouldn't it be clear from the spec change in which validation rule the changes should have gone? Actually....not exactly. According to the proposed spec changes, this work was not done within the
ValuesOfCorrectTypeRuleor theVariablesInAllowedPositionRule, but instead within a wholly new "OneOf Input Object Rule." That's not the way it got implemented, and in my opinion for good reason! I'm planning on separately submit some comments on the RFC to that effect, that we can eliminate the need for the new rule, and fold the changes into the existingValuesOfCorrectTypeRule-- which basically says that if it can't coerce, it's not valid, and because of the coercion changes does not require any actual new text -- except within theVariablesInAllowedPositionRule.Anyway, TLDR TLDR => let's consider moving validation of variable positions with respect to one of into the
VariablesInAllowedPositionRule, i.e. out of theValuesOfCorrectTypeRule. Discussion of allowed variable positions just belongs within that rule, just look at the rule name. :)