From 858aa325b0ecbef8a0ad509b64f55c6fa8a85024 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 15 Oct 2024 17:42:34 +0300 Subject: [PATCH] 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`.
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](https://github.com/graphql/graphql-spec/pull/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. :) --- 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 4ec1a0ebae..b750cccc28 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; } @@ -237,6 +238,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, }); @@ -244,6 +246,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 0959237ceb..7b1371f8d6 100644 --- a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts +++ b/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 --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 fdc20bcf54..dfab414ad8 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) { @@ -191,7 +174,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; @@ -208,7 +190,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( @@ -216,21 +197,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,