move variable position validation for OneOf to VariablesInAllowedPositionRule

[yaacovCR]

TLDR => let's consider moving validation of variable positions with respect to one of into the VariablesInAllowedPositionRule, i.e. out of the ValuesOfCorrectTypeRule.

---

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 ValuesOfCorrectTypeRule into a validateInputLiteral() 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 @oneOf directive tells the validator/executor that we have to supply exactly one of these fields (and it cannot be given the value null). In essence, we are saying that when @oneOf is 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 to validateInputLiteral(), 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 ValuesOfCorrectTypeRule to collect all those operation variable definitions? If we move this bit to VariablesInAllowedPositionRule, 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 ValuesOfCorrectTypeRule or the VariablesInAllowedPositionRule, 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 existing ValuesOfCorrectTypeRule -- 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 the VariablesInAllowedPositionRule.

Anyway, TLDR TLDR => let's consider moving validation of variable positions with respect to one of into the VariablesInAllowedPositionRule, i.e. out of the ValuesOfCorrectTypeRule. Discussion of allowed variable positions just belongs within that rule, just look at the rule name. :)

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	a3dbb17
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/66f96be3b41758000984d90d
😎 Deploy Preview	https://deploy-preview-4194--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[github-actions]

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[benjie]

Looks good to me!

separate PR to add additional tests, they belong to ValuesOfCorrectType, while this PR is related to oneOf and VariablesInAllowedPosition

[yaacovCR]

Adding link to PR to change this within the @oneOf proposed spec edits: benjie/graphql-spec#1

[yaacovCR]

@benjie @JoviDeCroock do you think we can merge this PR, it's a proof of concept for benjie/graphql-spec#1 , the simpler version of the oneOf Spec edits? This was discussed at the latest WG meeting without clear resolution as far as I can tell, with @benjie favoring this change, but waiting for feedback from @leebyron, others suggesting that moving things around between different validation rules could be considered non-breaking, so it could even be considered (closer) to an editorial type change.

I would like to merge this PR, because it's blocking the remainder of the default value coercion stack, and I would hate to have to wait another several weeks on that.

[JoviDeCroock]

From my side this looks great and makes a lot of sense

[yaacovCR]

I'm going to go ahead and merge it, because it has no change in functionality, unblocks us, and has two different approvals. We can always revert this before releasing v17 if it turns out to have been the wrong move.

[benjie]

LGTM 👍