Move variable position validation for OneOf fields to VariablesInAllo…

…wedPosition Rule
yaacovCR · Sep 29, 2024 · a3dbb17 · a3dbb17
1 parent 4dfae39
commit a3dbb17
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 54 deletions.
diff --git a/src/validation/ValidationContext.ts b/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>;
 }
@@ -226,13 +227,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 --git a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts
@@ -1094,22 +1094,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 --git a/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts b/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 --git a/src/validation/rules/ValuesOfCorrectTypeRule.ts b/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) {
@@ -189,7 +172,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;
@@ -206,29 +188,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 --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/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,