From a3dbb175d2d492d465be47553525fe9234b445e6 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 17 Sep 2024 20:19:21 +0300 Subject: [PATCH] Move variable position validation for OneOf fields to VariablesInAllowedPosition Rule --- src/validation/ValidationContext.ts | 3 ++ .../__tests__/ValuesOfCorrectTypeRule-test.ts | 16 -------- .../VariablesInAllowedPositionRule-test.ts | 31 ++++++++++++++++ .../rules/ValuesOfCorrectTypeRule.ts | 37 +------------------ .../rules/VariablesInAllowedPositionRule.ts | 27 +++++++++++++- 5 files changed, 60 insertions(+), 54 deletions(-) diff --git a/src/validation/ValidationContext.ts b/src/validation/ValidationContext.ts index d45e7a46a4..fa94e004e8 100644 --- 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; + readonly parentType: Maybe; readonly defaultValue: GraphQLDefaultValueUsage | undefined; readonly fragmentVariableDefinition: Maybe; } @@ -226,6 +227,7 @@ 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, }); @@ -233,6 +235,7 @@ export class ValidationContext extends ASTValidationContext { 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 index 819d103e6a..af06d42501 100644 --- 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 index d0da8f5305..127f146230 100644 --- 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 index d2b566823d..a996097f37 100644 --- 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, - variableDefinitions: { [key: string]: VariableDefinitionNode }, ): void { const keys = Array.from(fieldNodeMap.keys()); const isNotExactlyOneField = keys.length !== 1; @@ -206,7 +188,6 @@ 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( @@ -214,21 +195,5 @@ function validateOneOfInputObject( 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 index 888e49d3dd..dfcedbe547 100644 --- 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,