Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
move variable position validation for OneOf to `VariablesInAllowedPos…
…itionRule` (#4194) ### TLDR => let's consider moving validation of variable positions with respect to one of into the `VariablesInAllowedPositionRule`, i.e. out of the `ValuesOfCorrectTypeRule`. <hr> 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. <hr> 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. <hr> 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](graphql/graphql-spec#825), 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. :)
- Loading branch information
