From 12f67c01d3a2c8d0f8991ff0608d75e0793d53de Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Tue, 11 May 2021 15:06:20 -0700 Subject: [PATCH] Preserve sources of variable values By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`. --- src/execution/collectFields.ts | 11 +-- src/execution/execute.ts | 13 +-- src/execution/values.ts | 60 ++++++++----- .../__tests__/coerceInputValue-test.ts | 85 +++++++++++++++---- src/utilities/coerceInputValue.ts | 40 +++++---- .../rules/SingleFieldSubscriptionsRule.ts | 5 +- 6 files changed, 145 insertions(+), 69 deletions(-) diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 17468b791f0..4b81c6ab919 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -24,6 +24,7 @@ import type { GraphQLSchema } from '../type/schema.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; +import type { VariableValues } from './values.js'; import { getDirectiveValues } from './values.js'; export interface PatchFields { @@ -48,7 +49,7 @@ export interface FieldsAndPatches { export function collectFields( schema: GraphQLSchema, fragments: ObjMap, - variableValues: { [variable: string]: unknown }, + variableValues: VariableValues, runtimeType: GraphQLObjectType, operation: OperationDefinitionNode, ): FieldsAndPatches { @@ -82,7 +83,7 @@ export function collectFields( export function collectSubfields( schema: GraphQLSchema, fragments: ObjMap, - variableValues: { [variable: string]: unknown }, + variableValues: VariableValues, operation: OperationDefinitionNode, returnType: GraphQLObjectType, fieldNodes: ReadonlyArray, @@ -118,7 +119,7 @@ export function collectSubfields( function collectFieldsImpl( schema: GraphQLSchema, fragments: ObjMap, - variableValues: { [variable: string]: unknown }, + variableValues: VariableValues, operation: OperationDefinitionNode, runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, @@ -244,7 +245,7 @@ function collectFieldsImpl( */ function getDeferValues( operation: OperationDefinitionNode, - variableValues: { [variable: string]: unknown }, + variableValues: VariableValues, node: FragmentSpreadNode | InlineFragmentNode, ): undefined | { label: string | undefined } { const defer = getDirectiveValues(GraphQLDeferDirective, node, variableValues); @@ -272,7 +273,7 @@ function getDeferValues( * directives, where `@skip` has higher precedence than `@include`. */ function shouldIncludeNode( - variableValues: { [variable: string]: unknown }, + variableValues: VariableValues, node: FragmentSpreadNode | FieldNode | InlineFragmentNode, ): boolean { const skip = getDirectiveValues(GraphQLSkipDirective, node, variableValues); diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 1bc6c4267bb..8cc520de5bc 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -53,6 +53,7 @@ import { collectSubfields as _collectSubfields, } from './collectFields.js'; import { mapAsyncIterable } from './mapAsyncIterable.js'; +import type { VariableValues } from './values'; import { getArgumentValues, getDirectiveValues, @@ -116,7 +117,7 @@ export interface ExecutionContext { rootValue: unknown; contextValue: unknown; operation: OperationDefinitionNode; - variableValues: { [variable: string]: unknown }; + variableValues: VariableValues; fieldResolver: GraphQLFieldResolver; typeResolver: GraphQLTypeResolver; subscribeFieldResolver: GraphQLFieldResolver; @@ -482,15 +483,15 @@ export function buildExecutionContext( /* c8 ignore next */ const variableDefinitions = operation.variableDefinitions ?? []; - const coercedVariableValues = getVariableValues( + const variableValuesOrErrors = getVariableValues( schema, variableDefinitions, rawVariableValues ?? {}, { maxErrors: 50 }, ); - if (coercedVariableValues.errors) { - return coercedVariableValues.errors; + if (variableValuesOrErrors.errors) { + return variableValuesOrErrors.errors; } return { @@ -499,7 +500,7 @@ export function buildExecutionContext( rootValue, contextValue, operation, - variableValues: coercedVariableValues.coerced, + variableValues: variableValuesOrErrors.variableValues, fieldResolver: fieldResolver ?? defaultFieldResolver, typeResolver: typeResolver ?? defaultTypeResolver, subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver, @@ -787,7 +788,7 @@ export function buildResolveInfo( fragments: exeContext.fragments, rootValue: exeContext.rootValue, operation: exeContext.operation, - variableValues: exeContext.variableValues, + variableValues: exeContext.variableValues.coerced, }; } diff --git a/src/execution/values.ts b/src/execution/values.ts index 420be7ad877..c04e921233f 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -1,7 +1,7 @@ import { inspect } from '../jsutils/inspect.js'; import { keyMap } from '../jsutils/keyMap.js'; import type { Maybe } from '../jsutils/Maybe.js'; -import type { ObjMap } from '../jsutils/ObjMap.js'; +import type { ObjMap, ReadOnlyObjMap } from '../jsutils/ObjMap'; import { printPathArray } from '../jsutils/printPathArray.js'; import { GraphQLError } from '../error/GraphQLError.js'; @@ -14,7 +14,7 @@ import type { import { Kind } from '../language/kinds.js'; import { print } from '../language/printer.js'; -import type { GraphQLField } from '../type/definition.js'; +import type { GraphQLField, GraphQLInputType } from '../type/definition.js'; import { isInputType, isNonNullType } from '../type/definition.js'; import type { GraphQLDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; @@ -26,9 +26,20 @@ import { } from '../utilities/coerceInputValue.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; -type CoercedVariableValues = - | { errors: ReadonlyArray; coerced?: never } - | { coerced: { [variable: string]: unknown }; errors?: never }; +export interface VariableValues { + readonly sources: ReadOnlyObjMap; + readonly coerced: ReadOnlyObjMap; +} + +interface VariableValueSource { + readonly variable: VariableDefinitionNode; + readonly type: GraphQLInputType; + readonly value: unknown; +} + +type VariableValuesOrErrors = + | { variableValues: VariableValues; errors?: never } + | { errors: ReadonlyArray; variableValues?: never }; /** * Prepares an object map of variableValues of the correct type based on the @@ -44,11 +55,11 @@ export function getVariableValues( varDefNodes: ReadonlyArray, inputs: { readonly [variable: string]: unknown }, options?: { maxErrors?: number }, -): CoercedVariableValues { - const errors = []; +): VariableValuesOrErrors { + const errors: Array = []; const maxErrors = options?.maxErrors; try { - const coerced = coerceVariableValues( + const variableValues = coerceVariableValues( schema, varDefNodes, inputs, @@ -63,7 +74,7 @@ export function getVariableValues( ); if (errors.length === 0) { - return { coerced }; + return { variableValues }; } } catch (error) { errors.push(error); @@ -77,8 +88,9 @@ function coerceVariableValues( varDefNodes: ReadonlyArray, inputs: { readonly [variable: string]: unknown }, onError: (error: GraphQLError) => void, -): { [variable: string]: unknown } { - const coercedValues: { [variable: string]: unknown } = {}; +): VariableValues { + const sources: ObjMap = Object.create(null); + const coerced: ObjMap = Object.create(null); for (const varDefNode of varDefNodes) { const varName = varDefNode.variable.name.value; const varType = typeFromAST(schema, varDefNode.type); @@ -96,11 +108,14 @@ function coerceVariableValues( } if (!hasOwnProperty(inputs, varName)) { - if (varDefNode.defaultValue) { - coercedValues[varName] = coerceInputLiteral( - varDefNode.defaultValue, - varType, - ); + const defaultValue = varDefNode.defaultValue; + if (defaultValue) { + sources[varName] = { + variable: varDefNode, + type: varType, + value: undefined, + }; + coerced[varName] = coerceInputLiteral(defaultValue, varType); } else if (isNonNullType(varType)) { onError( new GraphQLError( @@ -123,7 +138,8 @@ function coerceVariableValues( continue; } - coercedValues[varName] = coerceInputValue( + sources[varName] = { variable: varDefNode, type: varType, value }; + coerced[varName] = coerceInputValue( value, varType, (path, invalidValue, error) => { @@ -142,7 +158,7 @@ function coerceVariableValues( ); } - return coercedValues; + return { sources, coerced }; } /** @@ -156,7 +172,7 @@ function coerceVariableValues( export function getArgumentValues( def: GraphQLField | GraphQLDirective, node: FieldNode | DirectiveNode, - variableValues?: Maybe>, + variableValues?: Maybe, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; @@ -192,7 +208,7 @@ export function getArgumentValues( const variableName = valueNode.name.value; if ( variableValues == null || - !hasOwnProperty(variableValues, variableName) + !hasOwnProperty(variableValues.coerced, variableName) ) { if (argDef.defaultValue) { coercedValues[name] = coerceDefaultValue( @@ -208,7 +224,7 @@ export function getArgumentValues( } continue; } - isNull = variableValues[variableName] == null; + isNull = variableValues.coerced[variableName] == null; } if (isNull && isNonNullType(argType)) { @@ -249,7 +265,7 @@ export function getArgumentValues( export function getDirectiveValues( directiveDef: GraphQLDirective, node: { readonly directives?: ReadonlyArray | undefined }, - variableValues?: Maybe>, + variableValues?: Maybe, ): undefined | { [argument: string]: unknown } { const directiveNode = node.directives?.find( (directive) => directive.name.value === directiveDef.name, diff --git a/src/utilities/__tests__/coerceInputValue-test.ts b/src/utilities/__tests__/coerceInputValue-test.ts index b67f09db40d..d61c4150e1e 100644 --- a/src/utilities/__tests__/coerceInputValue-test.ts +++ b/src/utilities/__tests__/coerceInputValue-test.ts @@ -3,11 +3,12 @@ import { describe, it } from 'mocha'; import { identityFunc } from '../../jsutils/identityFunc.js'; import { invariant } from '../../jsutils/invariant.js'; -import type { ObjMap } from '../../jsutils/ObjMap.js'; +import type { ReadOnlyObjMap } from '../../jsutils/ObjMap.js'; import { Kind } from '../../language/kinds.js'; -import { parseValue } from '../../language/parser.js'; +import { Parser, parseValue } from '../../language/parser.js'; import { print } from '../../language/printer.js'; +import { TokenKind } from '../../language/tokenKind.js'; import type { GraphQLInputType } from '../../type/definition.js'; import { @@ -24,6 +25,10 @@ import { GraphQLInt, GraphQLString, } from '../../type/scalars.js'; +import { GraphQLSchema } from '../../type/schema.js'; + +import type { VariableValues } from '../../execution/values.js'; +import { getVariableValues } from '../../execution/values.js'; import { coerceDefaultValue, @@ -451,20 +456,29 @@ describe('coerceInputLiteral', () => { valueText: string, type: GraphQLInputType, expected: unknown, - variables?: ObjMap, + variableValues?: VariableValues, ) { const ast = parseValue(valueText); - const value = coerceInputLiteral(ast, type, variables); + const value = coerceInputLiteral(ast, type, variableValues); expect(value).to.deep.equal(expected); } function testWithVariables( - variables: ObjMap, + variableDefs: string, + inputs: ReadOnlyObjMap, valueText: string, type: GraphQLInputType, expected: unknown, ) { - test(valueText, type, expected, variables); + const parser = new Parser(variableDefs); + parser.expectToken(TokenKind.SOF); + const variableValuesOrErrors = getVariableValues( + new GraphQLSchema({}), + parser.parseVariableDefinitions(), + inputs, + ); + invariant(variableValuesOrErrors.variableValues !== undefined); + test(valueText, type, expected, variableValuesOrErrors.variableValues); } it('converts according to input coercion rules', () => { @@ -663,19 +677,55 @@ describe('coerceInputLiteral', () => { it('accepts variable values assuming already coerced', () => { test('$var', GraphQLBoolean, undefined); - testWithVariables({ var: true }, '$var', GraphQLBoolean, true); - testWithVariables({ var: null }, '$var', GraphQLBoolean, null); - testWithVariables({ var: null }, '$var', nonNullBool, undefined); + testWithVariables( + '($var: Boolean)', + { var: true }, + '$var', + GraphQLBoolean, + true, + ); + testWithVariables( + '($var: Boolean)', + { var: null }, + '$var', + GraphQLBoolean, + null, + ); + testWithVariables( + '($var: Boolean)', + { var: null }, + '$var', + nonNullBool, + undefined, + ); }); it('asserts variables are provided as items in lists', () => { test('[ $foo ]', listOfBool, [null]); test('[ $foo ]', listOfNonNullBool, undefined); - testWithVariables({ foo: true }, '[ $foo ]', listOfNonNullBool, [true]); + testWithVariables( + '($foo: Boolean)', + { foo: true }, + '[ $foo ]', + listOfNonNullBool, + [true], + ); // Note: variables are expected to have already been coerced, so we // do not expect the singleton wrapping behavior for variables. - testWithVariables({ foo: true }, '$foo', listOfNonNullBool, true); - testWithVariables({ foo: [true] }, '$foo', listOfNonNullBool, [true]); + testWithVariables( + '($foo: Boolean)', + { foo: true }, + '$foo', + listOfNonNullBool, + true, + ); + testWithVariables( + '($foo: [Boolean])', + { foo: [true] }, + '$foo', + listOfNonNullBool, + [true], + ); }); it('omits input object fields for unprovided variables', () => { @@ -684,10 +734,13 @@ describe('coerceInputLiteral', () => { requiredBool: true, }); test('{ requiredBool: $foo }', testInputObj, undefined); - testWithVariables({ foo: true }, '{ requiredBool: $foo }', testInputObj, { - int: 42, - requiredBool: true, - }); + testWithVariables( + '($foo: Boolean)', + { foo: true }, + '{ requiredBool: $foo }', + testInputObj, + { int: 42, requiredBool: true }, + ); }); }); diff --git a/src/utilities/coerceInputValue.ts b/src/utilities/coerceInputValue.ts index b192ca7866c..79a19b74bea 100644 --- a/src/utilities/coerceInputValue.ts +++ b/src/utilities/coerceInputValue.ts @@ -6,7 +6,6 @@ import { isIterableObject } from '../jsutils/isIterableObject.js'; import { isObjectLike } from '../jsutils/isObjectLike.js'; import { keyMap } from '../jsutils/keyMap.js'; import type { Maybe } from '../jsutils/Maybe'; -import type { ObjMap } from '../jsutils/ObjMap'; import type { Path } from '../jsutils/Path.js'; import { addPath, pathToArray } from '../jsutils/Path.js'; import { printPathArray } from '../jsutils/printPathArray.js'; @@ -14,13 +13,13 @@ import { suggestionList } from '../jsutils/suggestionList.js'; import { GraphQLError } from '../error/GraphQLError.js'; -import type { ValueNode } from '../language/ast'; +import type { ValueNode } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; import type { GraphQLDefaultValueUsage, GraphQLInputType, -} from '../type/definition'; +} from '../type/definition.js'; import { assertLeafType, isInputObjectType, @@ -30,6 +29,8 @@ import { isRequiredInputField, } from '../type/definition.js'; +import type { VariableValues } from '../execution/values.js'; + type OnErrorCB = ( path: ReadonlyArray, invalidValue: unknown, @@ -205,26 +206,26 @@ function coerceInputValueImpl( export function coerceInputLiteral( valueNode: ValueNode, type: GraphQLInputType, - variables?: Maybe>, + variableValues?: Maybe, ): unknown { if (valueNode.kind === Kind.VARIABLE) { - if (!variables || isMissingVariable(valueNode, variables)) { + if (!variableValues || isMissingVariable(valueNode, variableValues)) { return; // Invalid: intentionally return no value. } - const variableValue = variables[valueNode.name.value]; - if (variableValue === null && isNonNullType(type)) { + const coercedVariableValue = variableValues.coerced[valueNode.name.value]; + if (coercedVariableValue === null && isNonNullType(type)) { return; // Invalid: intentionally return no value. } // Note: This does no further checking that this variable is correct. // This assumes validated has checked this variable is of the correct type. - return variableValue; + return coercedVariableValue; } if (isNonNullType(type)) { if (valueNode.kind === Kind.NULL) { return; // Invalid: intentionally return no value. } - return coerceInputLiteral(valueNode, type.ofType, variables); + return coerceInputLiteral(valueNode, type.ofType, variableValues); } if (valueNode.kind === Kind.NULL) { @@ -234,7 +235,11 @@ export function coerceInputLiteral( if (isListType(type)) { if (valueNode.kind !== Kind.LIST) { // Lists accept a non-list value as a list of one. - const itemValue = coerceInputLiteral(valueNode, type.ofType, variables); + const itemValue = coerceInputLiteral( + valueNode, + type.ofType, + variableValues, + ); if (itemValue === undefined) { return; // Invalid: intentionally return no value. } @@ -242,10 +247,10 @@ export function coerceInputLiteral( } const coercedValue: Array = []; for (const itemNode of valueNode.values) { - let itemValue = coerceInputLiteral(itemNode, type.ofType, variables); + let itemValue = coerceInputLiteral(itemNode, type.ofType, variableValues); if (itemValue === undefined) { if ( - isMissingVariable(itemNode, variables) && + isMissingVariable(itemNode, variableValues) && !isNonNullType(type.ofType) ) { // A missing variable within a list is coerced to null. @@ -275,7 +280,7 @@ export function coerceInputLiteral( const fieldNodes = keyMap(valueNode.fields, (field) => field.name.value); for (const field of Object.values(fieldDefs)) { const fieldNode = fieldNodes[field.name]; - if (!fieldNode || isMissingVariable(fieldNode.value, variables)) { + if (!fieldNode || isMissingVariable(fieldNode.value, variableValues)) { if (isRequiredInputField(field)) { return; // Invalid: intentionally return no value. } @@ -289,7 +294,7 @@ export function coerceInputLiteral( const fieldValue = coerceInputLiteral( fieldNode.value, field.type, - variables, + variableValues, ); if (fieldValue === undefined) { return; // Invalid: intentionally return no value. @@ -303,7 +308,7 @@ export function coerceInputLiteral( const leafType = assertLeafType(type); try { - return leafType.parseLiteral(valueNode, variables); + return leafType.parseLiteral(valueNode, variableValues?.coerced); } catch (_error) { // Invalid: ignore error and intentionally return no value. } @@ -313,11 +318,12 @@ export function coerceInputLiteral( // in the set of variables. function isMissingVariable( valueNode: ValueNode, - variables: Maybe>, + variables: Maybe, ): boolean { return ( valueNode.kind === Kind.VARIABLE && - (variables == null || !hasOwnProperty(variables, valueNode.name.value)) + (variables == null || + !hasOwnProperty(variables.coerced, valueNode.name.value)) ); } diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 4a3d8341240..dde6f187488 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -10,6 +10,7 @@ import { Kind } from '../../language/kinds.js'; import type { ASTVisitor } from '../../language/visitor.js'; import { collectFields } from '../../execution/collectFields.js'; +import type { VariableValues } from '../../execution/values.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -31,9 +32,7 @@ export function SingleFieldSubscriptionsRule( const subscriptionType = schema.getSubscriptionType(); if (subscriptionType) { const operationName = node.name ? node.name.value : null; - const variableValues: { - [variable: string]: any; - } = Object.create(null); + const variableValues: VariableValues = Object.create(null); const document = context.getDocument(); const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) {