From a13d98520ee14d36a3bc76ed821d869902908353 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Tue, 11 May 2021 15:06:20 -0700 Subject: [PATCH 1/4] 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 | 83 +++++++------- src/execution/execute.ts | 19 ++-- src/execution/values.ts | 103 +++++++++++++----- .../__tests__/coerceInputValue-test.ts | 85 ++++++++++++--- src/utilities/coerceInputValue.ts | 36 +++--- .../rules/SingleFieldSubscriptionsRule.ts | 5 +- 6 files changed, 218 insertions(+), 113 deletions(-) diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index d1916867d6..e47c1575b3 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -25,22 +25,18 @@ import type { GraphQLSchema } from '../type/schema.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; import type { GraphQLVariableSignature } from './getVariableSignature.js'; -import { experimentalGetArgumentValues, getDirectiveValues } from './values.js'; +import type { VariableValues } from './values.js'; +import { getDirectiveValues, getFragmentVariableValues } from './values.js'; export interface DeferUsage { label: string | undefined; parentDeferUsage: DeferUsage | undefined; } -export interface FragmentVariables { - signatures: ObjMap; - values: ObjMap; -} - export interface FieldDetails { node: FieldNode; deferUsage?: DeferUsage | undefined; - fragmentVariables?: FragmentVariables | undefined; + fragmentVariableValues?: VariableValues | undefined; } export type FieldGroup = ReadonlyArray; @@ -55,7 +51,7 @@ export interface FragmentDetails { interface CollectFieldsContext { schema: GraphQLSchema; fragments: ObjMap; - variableValues: { [variable: string]: unknown }; + variableValues: VariableValues; operation: OperationDefinitionNode; runtimeType: GraphQLObjectType; visitedFragmentNames: Set; @@ -73,7 +69,7 @@ interface CollectFieldsContext { export function collectFields( schema: GraphQLSchema, fragments: ObjMap, - variableValues: { [variable: string]: unknown }, + variableValues: VariableValues, runtimeType: GraphQLObjectType, operation: OperationDefinitionNode, ): { @@ -114,7 +110,7 @@ export function collectFields( export function collectSubfields( schema: GraphQLSchema, fragments: ObjMap, - variableValues: { [variable: string]: unknown }, + variableValues: VariableValues, operation: OperationDefinitionNode, returnType: GraphQLObjectType, fieldGroup: FieldGroup, @@ -136,14 +132,14 @@ export function collectSubfields( for (const fieldDetail of fieldGroup) { const selectionSet = fieldDetail.node.selectionSet; if (selectionSet) { - const { deferUsage, fragmentVariables } = fieldDetail; + const { deferUsage, fragmentVariableValues } = fieldDetail; collectFieldsImpl( context, selectionSet, subGroupedFieldSet, newDeferUsages, deferUsage, - fragmentVariables, + fragmentVariableValues, ); } } @@ -161,7 +157,7 @@ function collectFieldsImpl( groupedFieldSet: AccumulatorMap, newDeferUsages: Array, deferUsage?: DeferUsage, - fragmentVariables?: FragmentVariables, + fragmentVariableValues?: VariableValues, ): void { const { schema, @@ -175,19 +171,25 @@ function collectFieldsImpl( for (const selection of selectionSet.selections) { switch (selection.kind) { case Kind.FIELD: { - if (!shouldIncludeNode(selection, variableValues, fragmentVariables)) { + if ( + !shouldIncludeNode(selection, variableValues, fragmentVariableValues) + ) { continue; } groupedFieldSet.add(getFieldEntryKey(selection), { node: selection, deferUsage, - fragmentVariables, + fragmentVariableValues, }); break; } case Kind.INLINE_FRAGMENT: { if ( - !shouldIncludeNode(selection, variableValues, fragmentVariables) || + !shouldIncludeNode( + selection, + variableValues, + fragmentVariableValues, + ) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { continue; @@ -196,7 +198,7 @@ function collectFieldsImpl( const newDeferUsage = getDeferUsage( operation, variableValues, - fragmentVariables, + fragmentVariableValues, selection, deferUsage, ); @@ -208,7 +210,7 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, deferUsage, - fragmentVariables, + fragmentVariableValues, ); } else { newDeferUsages.push(newDeferUsage); @@ -218,7 +220,7 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, newDeferUsage, - fragmentVariables, + fragmentVariableValues, ); } @@ -230,7 +232,7 @@ function collectFieldsImpl( const newDeferUsage = getDeferUsage( operation, variableValues, - fragmentVariables, + fragmentVariableValues, selection, deferUsage, ); @@ -238,7 +240,11 @@ function collectFieldsImpl( if ( !newDeferUsage && (visitedFragmentNames.has(fragName) || - !shouldIncludeNode(selection, variableValues, fragmentVariables)) + !shouldIncludeNode( + selection, + variableValues, + fragmentVariableValues, + )) ) { continue; } @@ -252,17 +258,14 @@ function collectFieldsImpl( } const fragmentVariableSignatures = fragment.variableSignatures; - let newFragmentVariables: FragmentVariables | undefined; + let newFragmentVariableValues: VariableValues | undefined; if (fragmentVariableSignatures) { - newFragmentVariables = { - signatures: fragmentVariableSignatures, - values: experimentalGetArgumentValues( - selection, - Object.values(fragmentVariableSignatures), - variableValues, - fragmentVariables, - ), - }; + newFragmentVariableValues = getFragmentVariableValues( + selection, + fragmentVariableSignatures, + variableValues, + fragmentVariableValues, + ); } if (!newDeferUsage) { @@ -273,7 +276,7 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, deferUsage, - newFragmentVariables, + newFragmentVariableValues, ); } else { newDeferUsages.push(newDeferUsage); @@ -283,7 +286,7 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, newDeferUsage, - newFragmentVariables, + newFragmentVariableValues, ); } break; @@ -299,8 +302,8 @@ function collectFieldsImpl( */ function getDeferUsage( operation: OperationDefinitionNode, - variableValues: { [variable: string]: unknown }, - fragmentVariables: FragmentVariables | undefined, + variableValues: VariableValues, + fragmentVariableValues: VariableValues | undefined, node: FragmentSpreadNode | InlineFragmentNode, parentDeferUsage: DeferUsage | undefined, ): DeferUsage | undefined { @@ -308,7 +311,7 @@ function getDeferUsage( GraphQLDeferDirective, node, variableValues, - fragmentVariables, + fragmentVariableValues, ); if (!defer) { @@ -336,14 +339,14 @@ function getDeferUsage( */ function shouldIncludeNode( node: FragmentSpreadNode | FieldNode | InlineFragmentNode, - variableValues: { [variable: string]: unknown }, - fragmentVariables: FragmentVariables | undefined, + variableValues: VariableValues, + fragmentVariableValues: VariableValues | undefined, ): boolean { const skip = getDirectiveValues( GraphQLSkipDirective, node, variableValues, - fragmentVariables, + fragmentVariableValues, ); if (skip?.if === true) { return false; @@ -353,7 +356,7 @@ function shouldIncludeNode( GraphQLIncludeDirective, node, variableValues, - fragmentVariables, + fragmentVariableValues, ); if (include?.if === false) { return false; diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 782fda7537..404d267411 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -75,6 +75,7 @@ import type { StreamRecord, } from './types.js'; import { DeferredFragmentRecord } from './types.js'; +import type { VariableValues } from './values.js'; import { experimentalGetArgumentValues, getArgumentValues, @@ -139,7 +140,7 @@ export interface ExecutionContext { rootValue: unknown; contextValue: unknown; operation: OperationDefinitionNode; - variableValues: { [variable: string]: unknown }; + variableValues: VariableValues; fieldResolver: GraphQLFieldResolver; typeResolver: GraphQLTypeResolver; subscribeFieldResolver: GraphQLFieldResolver; @@ -510,15 +511,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 { @@ -527,7 +528,7 @@ export function buildExecutionContext( rootValue, contextValue, operation, - variableValues: coercedVariableValues.coerced, + variableValues: variableValuesOrErrors.variableValues, fieldResolver: fieldResolver ?? defaultFieldResolver, typeResolver: typeResolver ?? defaultTypeResolver, subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver, @@ -753,7 +754,7 @@ function executeField( fieldGroup[0].node, fieldDef.args, exeContext.variableValues, - fieldGroup[0].fragmentVariables, + fieldGroup[0].fragmentVariableValues, ); // The resolve function's optional third argument is a context value that @@ -842,7 +843,7 @@ export function buildResolveInfo( ), rootValue: exeContext.rootValue, operation: exeContext.operation, - variableValues: exeContext.variableValues, + variableValues: exeContext.variableValues.coerced, }; } @@ -1062,7 +1063,7 @@ function getStreamUsage( GraphQLStreamDirective, fieldGroup[0].node, exeContext.variableValues, - fieldGroup[0].fragmentVariables, + fieldGroup[0].fragmentVariableValues, ); if (!stream) { @@ -1091,7 +1092,7 @@ function getStreamUsage( const streamedFieldGroup: FieldGroup = fieldGroup.map((fieldDetails) => ({ node: fieldDetails.node, deferUsage: undefined, - fragmentVariables: fieldDetails.fragmentVariables, + fragmentVariables: fieldDetails.fragmentVariableValues, })); const streamUsage = { diff --git a/src/execution/values.ts b/src/execution/values.ts index 5ac0c7656c..b2e9818127 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -1,6 +1,6 @@ import { inspect } from '../jsutils/inspect.js'; import type { Maybe } from '../jsutils/Maybe.js'; -import type { ObjMap } from '../jsutils/ObjMap.js'; +import type { ObjMap, ReadOnlyObjMap } from '../jsutils/ObjMap.js'; import { printPathArray } from '../jsutils/printPathArray.js'; import { GraphQLError } from '../error/GraphQLError.js'; @@ -25,13 +25,22 @@ import { coerceInputValue, } from '../utilities/coerceInputValue.js'; -import type { FragmentVariables } from './collectFields.js'; import type { GraphQLVariableSignature } from './getVariableSignature.js'; import { getVariableSignature } from './getVariableSignature.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 signature: GraphQLVariableSignature; + 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 @@ -47,11 +56,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, @@ -66,7 +75,7 @@ export function getVariableValues( ); if (errors.length === 0) { - return { coerced }; + return { variableValues }; } } catch (error) { errors.push(error); @@ -80,10 +89,12 @@ 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 varSignature = getVariableSignature(schema, varDefNode); + if (varSignature instanceof GraphQLError) { onError(varSignature); continue; @@ -91,11 +102,13 @@ function coerceVariableValues( const { name: varName, type: varType } = varSignature; if (!Object.hasOwn(inputs, varName)) { - if (varSignature.defaultValue) { - coercedValues[varName] = coerceDefaultValue( - varSignature.defaultValue, - varType, - ); + const defaultValue = varSignature.defaultValue; + if (defaultValue) { + sources[varName] = { + signature: varSignature, + value: undefined, + }; + coerced[varName] = coerceDefaultValue(defaultValue, varType); } else if (isNonNullType(varType)) { const varTypeStr = inspect(varType); onError( @@ -104,6 +117,11 @@ function coerceVariableValues( { nodes: varDefNode }, ), ); + } else { + sources[varName] = { + signature: varSignature, + value: undefined, + }; } continue; } @@ -120,7 +138,8 @@ function coerceVariableValues( continue; } - coercedValues[varName] = coerceInputValue( + sources[varName] = { signature: varSignature, value }; + coerced[varName] = coerceInputValue( value, varType, (path, invalidValue, error) => { @@ -139,7 +158,35 @@ function coerceVariableValues( ); } - return coercedValues; + return { sources, coerced }; +} + +export function getFragmentVariableValues( + fragmentSpreadNode: FragmentSpreadNode, + fragmentSignatures: ReadOnlyObjMap, + variableValues: VariableValues, + fragmentVariableValues?: Maybe, +): VariableValues { + const varSignatures: Array = []; + const sources = Object.create(null); + for (const [varName, varSignature] of Object.entries(fragmentSignatures)) { + varSignatures.push(varSignature); + sources[varName] = { + signature: varSignature, + value: + fragmentVariableValues?.sources[varName]?.value ?? + variableValues.sources[varName]?.value, + }; + } + + const coerced = experimentalGetArgumentValues( + fragmentSpreadNode, + varSignatures, + variableValues, + fragmentVariableValues, + ); + + return { sources, coerced }; } /** @@ -153,7 +200,7 @@ function coerceVariableValues( export function getArgumentValues( def: GraphQLField | GraphQLDirective, node: FieldNode | DirectiveNode, - variableValues?: Maybe>, + variableValues?: Maybe, ): { [argument: string]: unknown } { return experimentalGetArgumentValues(node, def.args, variableValues); } @@ -161,8 +208,8 @@ export function getArgumentValues( export function experimentalGetArgumentValues( node: FieldNode | DirectiveNode | FragmentSpreadNode, argDefs: ReadonlyArray, - variableValues: Maybe>, - fragmentVariables?: Maybe, + variableValues: Maybe, + fragmentVariables?: Maybe, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; @@ -197,12 +244,12 @@ export function experimentalGetArgumentValues( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; - const scopedVariableValues = fragmentVariables?.signatures[variableName] - ? fragmentVariables.values + const scopedVariableValues = fragmentVariables?.sources[variableName] + ? fragmentVariables : variableValues; if ( scopedVariableValues == null || - !Object.hasOwn(scopedVariableValues, variableName) + !Object.hasOwn(scopedVariableValues.coerced, variableName) ) { if (argDef.defaultValue) { coercedValues[name] = coerceDefaultValue( @@ -218,7 +265,7 @@ export function experimentalGetArgumentValues( } continue; } - isNull = scopedVariableValues[variableName] == null; + isNull = scopedVariableValues.coerced[variableName] == null; } if (isNull && isNonNullType(argType)) { @@ -265,8 +312,8 @@ export function experimentalGetArgumentValues( export function getDirectiveValues( directiveDef: GraphQLDirective, node: { readonly directives?: ReadonlyArray | undefined }, - variableValues?: Maybe>, - fragmentVariables?: Maybe, + variableValues?: Maybe, + fragmentVariableValues?: Maybe, ): undefined | { [argument: string]: unknown } { const directiveNode = node.directives?.find( (directive) => directive.name.value === directiveDef.name, @@ -277,7 +324,7 @@ export function getDirectiveValues( directiveNode, directiveDef.args, variableValues, - fragmentVariables, + fragmentVariableValues, ); } } diff --git a/src/utilities/__tests__/coerceInputValue-test.ts b/src/utilities/__tests__/coerceInputValue-test.ts index 78dede7b81..0a71e39cba 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, @@ -557,20 +562,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', () => { @@ -789,19 +803,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', () => { @@ -810,10 +860,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 f912fea4e4..3122edb2ab 100644 --- a/src/utilities/coerceInputValue.ts +++ b/src/utilities/coerceInputValue.ts @@ -4,7 +4,6 @@ import { invariant } from '../jsutils/invariant.js'; import { isIterableObject } from '../jsutils/isIterableObject.js'; import { isObjectLike } from '../jsutils/isObjectLike.js'; import type { Maybe } from '../jsutils/Maybe.js'; -import type { ObjMap } from '../jsutils/ObjMap.js'; import type { Path } from '../jsutils/Path.js'; import { addPath, pathToArray } from '../jsutils/Path.js'; import { printPathArray } from '../jsutils/printPathArray.js'; @@ -28,7 +27,7 @@ import { isRequiredInputField, } from '../type/definition.js'; -import type { FragmentVariables } from '../execution/collectFields.js'; +import type { VariableValues } from '../execution/values.js'; type OnErrorCB = ( path: ReadonlyArray, @@ -229,21 +228,21 @@ function coerceInputValueImpl( export function coerceInputLiteral( valueNode: ValueNode, type: GraphQLInputType, - variableValues?: Maybe>, - fragmentVariableValues?: Maybe, + variableValues?: Maybe, + fragmentVariableValues?: Maybe, ): unknown { if (valueNode.kind === Kind.VARIABLE) { - const variableValue = getVariableValue( + const coercedVariableValue = getCoercedVariableValue( valueNode, variableValues, fragmentVariableValues, ); - if (variableValue == null && isNonNullType(type)) { + 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)) { @@ -287,8 +286,11 @@ export function coerceInputLiteral( if (itemValue === undefined) { if ( itemNode.kind === Kind.VARIABLE && - getVariableValue(itemNode, variableValues, fragmentVariableValues) == - null && + getCoercedVariableValue( + itemNode, + variableValues, + fragmentVariableValues, + ) == null && !isNonNullType(type.ofType) ) { // A missing variable within a list is coerced to null. @@ -323,7 +325,7 @@ export function coerceInputLiteral( if ( !fieldNode || (fieldNode.value.kind === Kind.VARIABLE && - getVariableValue( + getCoercedVariableValue( fieldNode.value, variableValues, fragmentVariableValues, @@ -369,24 +371,24 @@ export function coerceInputLiteral( const leafType = assertLeafType(type); try { - return leafType.parseLiteral(valueNode, variableValues); + return leafType.parseLiteral(valueNode, variableValues?.coerced); } catch (_error) { // Invalid: ignore error and intentionally return no value. } } // Retrieves the variable value for the given variable node. -function getVariableValue( +function getCoercedVariableValue( variableNode: VariableNode, - variableValues: Maybe>, - fragmentVariableValues: Maybe | undefined, + variableValues: Maybe, + fragmentVariableValues: Maybe, ): unknown { const varName = variableNode.name.value; - if (fragmentVariableValues?.signatures[varName]) { - return fragmentVariableValues.values[varName]; + if (fragmentVariableValues?.sources[varName] !== undefined) { + return fragmentVariableValues.coerced[varName]; } - return variableValues?.[varName]; + return variableValues?.coerced[varName]; } /** diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index dfefc6cdd8..b5cddef755 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -11,6 +11,7 @@ import type { FragmentDetails, } from '../../execution/collectFields.js'; import { collectFields } from '../../execution/collectFields.js'; +import type { VariableValues } from '../../execution/values.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -36,9 +37,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) { From aa1371f593e7b0ddec6e711f7309e171d1bd947d Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Wed, 25 Sep 2024 15:45:19 +0300 Subject: [PATCH 2/4] for consistency --- src/execution/execute.ts | 2 +- src/execution/values.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 404d267411..939104c430 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -1092,7 +1092,7 @@ function getStreamUsage( const streamedFieldGroup: FieldGroup = fieldGroup.map((fieldDetails) => ({ node: fieldDetails.node, deferUsage: undefined, - fragmentVariables: fieldDetails.fragmentVariableValues, + fragmentVariablesValues: fieldDetails.fragmentVariableValues, })); const streamUsage = { diff --git a/src/execution/values.ts b/src/execution/values.ts index b2e9818127..2bc1b55117 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -209,7 +209,7 @@ export function experimentalGetArgumentValues( node: FieldNode | DirectiveNode | FragmentSpreadNode, argDefs: ReadonlyArray, variableValues: Maybe, - fragmentVariables?: Maybe, + fragmentVariablesValues?: Maybe, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; @@ -244,8 +244,8 @@ export function experimentalGetArgumentValues( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; - const scopedVariableValues = fragmentVariables?.sources[variableName] - ? fragmentVariables + const scopedVariableValues = fragmentVariablesValues?.sources[variableName] + ? fragmentVariablesValues : variableValues; if ( scopedVariableValues == null || @@ -280,7 +280,7 @@ export function experimentalGetArgumentValues( valueNode, argType, variableValues, - fragmentVariables, + fragmentVariablesValues, ); if (coercedValue === undefined) { // Note: ValuesOfCorrectTypeRule validation should catch this before From c81c719c15d1b20d6fa547dd8bf0ba5fb1dbc574 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Wed, 25 Sep 2024 16:00:09 +0300 Subject: [PATCH 3/4] expose VariableValues breaking change, have to do it in v17 or would have to wait until v18 --- src/execution/__tests__/executor-test.ts | 14 +++++++++++++- src/execution/execute.ts | 2 +- src/type/definition.ts | 4 +++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/execution/__tests__/executor-test.ts b/src/execution/__tests__/executor-test.ts index 6cb6858e46..aa84f7cc94 100644 --- a/src/execution/__tests__/executor-test.ts +++ b/src/execution/__tests__/executor-test.ts @@ -240,7 +240,19 @@ describe('Execute: Handles basic execution tasks', () => { expect(resolvedInfo).to.deep.include({ fieldNodes: [field], path: { prev: undefined, key: 'result', typename: 'Test' }, - variableValues: { var: 'abc' }, + variableValues: { + sources: { + var: { + signature: { + name: 'var', + type: GraphQLString, + defaultValue: undefined, + }, + value: 'abc', + }, + }, + coerced: { var: 'abc' }, + }, }); }); diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 939104c430..6909765041 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -843,7 +843,7 @@ export function buildResolveInfo( ), rootValue: exeContext.rootValue, operation: exeContext.operation, - variableValues: exeContext.variableValues.coerced, + variableValues: exeContext.variableValues, }; } diff --git a/src/type/definition.ts b/src/type/definition.ts index 4ebb5ae928..6ce4115a87 100644 --- a/src/type/definition.ts +++ b/src/type/definition.ts @@ -40,6 +40,8 @@ import type { import { Kind } from '../language/kinds.js'; import { print } from '../language/printer.js'; +import type { VariableValues } from '../execution/values.js'; + import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped.js'; import { assertEnumValueName, assertName } from './assertName.js'; @@ -897,7 +899,7 @@ export interface GraphQLResolveInfo { readonly fragments: ObjMap; readonly rootValue: unknown; readonly operation: OperationDefinitionNode; - readonly variableValues: { [variable: string]: unknown }; + readonly variableValues: VariableValues; } /** From c3e220da31f831075bbe9e98beaaa0d036847c93 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Wed, 25 Sep 2024 16:02:16 +0300 Subject: [PATCH 4/4] don't even list a source value if there wasn't one, just like it's not in the map --- src/execution/values.ts | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/execution/values.ts b/src/execution/values.ts index 2bc1b55117..cce0d1d12f 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -35,7 +35,7 @@ export interface VariableValues { interface VariableValueSource { readonly signature: GraphQLVariableSignature; - readonly value: unknown; + readonly value?: unknown; } type VariableValuesOrErrors = @@ -104,10 +104,7 @@ function coerceVariableValues( if (!Object.hasOwn(inputs, varName)) { const defaultValue = varSignature.defaultValue; if (defaultValue) { - sources[varName] = { - signature: varSignature, - value: undefined, - }; + sources[varName] = { signature: varSignature }; coerced[varName] = coerceDefaultValue(defaultValue, varType); } else if (isNonNullType(varType)) { const varTypeStr = inspect(varType); @@ -118,10 +115,7 @@ function coerceVariableValues( ), ); } else { - sources[varName] = { - signature: varSignature, - value: undefined, - }; + sources[varName] = { signature: varSignature }; } continue; } @@ -244,7 +238,9 @@ export function experimentalGetArgumentValues( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; - const scopedVariableValues = fragmentVariablesValues?.sources[variableName] + const scopedVariableValues = fragmentVariablesValues?.sources[ + variableName + ] ? fragmentVariablesValues : variableValues; if (