move variable position validation for OneOf to VariablesInAllowedPositionRule (#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. :)

Author: yaacovCR

Diff for: src/validation/ValidationContext.ts

@@ -35,6 +35,7 @@ type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode;
 interface VariableUsage {
   readonly node: VariableNode;
   readonly type: Maybe<GraphQLInputType>;
+  readonly parentType: Maybe<GraphQLInputType>;
   readonly defaultValue: GraphQLDefaultValueUsage | undefined;
   readonly fragmentVariableDefinition: Maybe<VariableDefinitionNode>;
 }
@@ -237,13 +238,15 @@ export class ValidationContext extends ASTValidationContext {
               newUsages.push({
                 node: variable,
                 type: typeInfo.getInputType(),
+                parentType: typeInfo.getParentInputType(),
                 defaultValue: undefined, // fragment variables have a variable default but no location default, which is what this default value represents
                 fragmentVariableDefinition,
               });
             } else {
               newUsages.push({
                 node: variable,
                 type: typeInfo.getInputType(),
+                parentType: typeInfo.getParentInputType(),
                 defaultValue: typeInfo.getDefaultValue(),
                 fragmentVariableDefinition: undefined,
               });

Diff for: src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts

@@ -1156,22 +1156,6 @@ describe('Validate: Values of correct type', () => {
       ]);
     });
 
-    it('Exactly one nullable variable', () => {
-      expectErrors(`
-        query ($string: String) {
-          complicatedArgs {
-            oneOfArgField(oneOfArg: { stringField: $string })
-          }
-        }
-      `).toDeepEqual([
-        {
-          message:
-            'Variable "$string" must be non-nullable to be used for OneOf Input Object "OneOfInput".',
-          locations: [{ line: 4, column: 37 }],
-        },
-      ]);
-    });
-
     it('More than one field', () => {
       expectErrors(`
         {

Diff for: src/validation/__tests__/VariablesInAllowedPositionRule-test.ts

@@ -365,6 +365,37 @@ describe('Validate: Variables are in allowed positions', () => {
     });
   });
 
+  describe('Validates OneOf Input Objects', () => {
+    it('Allows exactly one non-nullable variable', () => {
+      expectValid(`
+        query ($string: String!) {
+          complicatedArgs {
+            oneOfArgField(oneOfArg: { stringField: $string })
+          }
+        }
+      `);
+    });
+
+    it('Forbids one nullable variable', () => {
+      expectErrors(`
+        query ($string: String) {
+          complicatedArgs {
+            oneOfArgField(oneOfArg: { stringField: $string })
+          }
+        }
+      `).toDeepEqual([
+        {
+          message:
+            'Variable "$string" is of type "String" but must be non-nullable to be used for OneOf Input Object "OneOfInput".',
+          locations: [
+            { line: 2, column: 16 },
+            { line: 4, column: 52 },
+          ],
+        },
+      ]);
+    });
+  });
+
   describe('Fragment arguments are validated', () => {
     it('Boolean => Boolean', () => {
       expectValid(`

Diff for: src/validation/rules/ValuesOfCorrectTypeRule.ts

@@ -8,7 +8,6 @@ import type {
   ObjectFieldNode,
   ObjectValueNode,
   ValueNode,
-  VariableDefinitionNode,
 } from '../../language/ast.js';
 import { Kind } from '../../language/kinds.js';
 import { print } from '../../language/printer.js';
@@ -40,17 +39,7 @@ import type { ValidationContext } from '../ValidationContext.js';
 export function ValuesOfCorrectTypeRule(
   context: ValidationContext,
 ): ASTVisitor {
-  let variableDefinitions: { [key: string]: VariableDefinitionNode } = {};
-
   return {
-    OperationDefinition: {
-      enter() {
-        variableDefinitions = {};
-      },
-    },
-    VariableDefinition(definition) {
-      variableDefinitions[definition.variable.name.value] = definition;
-    },
     ListValue(node) {
       // Note: TypeInfo will traverse into a list's item type, so look to the
       // parent input type to check if it is a list.
@@ -84,13 +73,7 @@ export function ValuesOfCorrectTypeRule(
       }
 
       if (type.isOneOf) {
-        validateOneOfInputObject(
-          context,
-          node,
-          type,
-          fieldNodeMap,
-          variableDefinitions,
-        );
+        validateOneOfInputObject(context, node, type, fieldNodeMap);
       }
     },
     ObjectField(node) {
@@ -191,7 +174,6 @@ function validateOneOfInputObject(
   node: ObjectValueNode,
   type: GraphQLInputObjectType,
   fieldNodeMap: Map<string, ObjectFieldNode>,
-  variableDefinitions: { [key: string]: VariableDefinitionNode },
 ): void {
   const keys = Array.from(fieldNodeMap.keys());
   const isNotExactlyOneField = keys.length !== 1;
@@ -208,29 +190,12 @@ function validateOneOfInputObject(
 
   const value = fieldNodeMap.get(keys[0])?.value;
   const isNullLiteral = !value || value.kind === Kind.NULL;
-  const isVariable = value?.kind === Kind.VARIABLE;
 
   if (isNullLiteral) {
     context.reportError(
       new GraphQLError(`Field "${type}.${keys[0]}" must be non-null.`, {
         nodes: [node],
       }),
     );
-    return;
-  }
-
-  if (isVariable) {
-    const variableName = value.name.value;
-    const definition = variableDefinitions[variableName];
-    const isNullableVariable = definition.type.kind !== Kind.NON_NULL_TYPE;
-
-    if (isNullableVariable) {
-      context.reportError(
-        new GraphQLError(
-          `Variable "$${variableName}" must be non-nullable to be used for OneOf Input Object "${type}".`,
-          { nodes: [node] },
-        ),
-      );
-    }
   }
 }

Diff for: src/validation/rules/VariablesInAllowedPositionRule.ts

@@ -11,7 +11,11 @@ import type {
   GraphQLDefaultValueUsage,
   GraphQLType,
 } from '../../type/definition.js';
-import { isNonNullType } from '../../type/definition.js';
+import {
+  isInputObjectType,
+  isNonNullType,
+  isNullableType,
+} from '../../type/definition.js';
 import type { GraphQLSchema } from '../../type/schema.js';
 
 import { isTypeSubTypeOf } from '../../utilities/typeComparators.js';
@@ -42,6 +46,7 @@ export function VariablesInAllowedPositionRule(
         for (const {
           node,
           type,
+          parentType,
           defaultValue,
           fragmentVariableDefinition,
         } of usages) {
@@ -78,6 +83,21 @@ export function VariablesInAllowedPositionRule(
                 ),
               );
             }
+
+            if (
+              isInputObjectType(parentType) &&
+              parentType.isOneOf &&
+              isNullableType(varType)
+            ) {
+              const varTypeStr = inspect(varType);
+              const parentTypeStr = inspect(parentType);
+              context.reportError(
+                new GraphQLError(
+                  `Variable "$${varName}" is of type "${varTypeStr}" but must be non-nullable to be used for OneOf Input Object "${parentTypeStr}".`,
+                  { nodes: [varDef, node] },
+                ),
+              );
+            }
           }
         }
       },
@@ -90,8 +110,11 @@ export function VariablesInAllowedPositionRule(
 
 /**
  * Returns true if the variable is allowed in the location it was found,
- * which includes considering if default values exist for either the variable
+ * including considering if default values exist for either the variable
  * or the location at which it is located.
+ *
+ * OneOf Input Object Type fields are considered separately above to
+ * provide a more descriptive error message.
  */
 function allowedVariableUsage(
   schema: GraphQLSchema,