diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index 526fc77cd8c..b2239132033 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -1198,11 +1198,12 @@ describe('Execute: Handles inputs', () => { fieldWithNullableStringInput(input: $value) } `); - expect(result).to.deep.equal({ - data: { - fieldWithNullableStringInput: null, - }, - }); + + expect(result).to.have.property('errors'); + expect(result.errors).to.have.length(1); + expect(result.errors?.at(0)?.message).to.match( + /Argument "value" of required type "String!"/, + ); }); it('when the definition has a default and is provided', () => { @@ -1237,22 +1238,42 @@ describe('Execute: Handles inputs', () => { }); }); - it('when the definition has a non-nullable default and is provided null', () => { + it('when a definition has a default, is not provided, and spreads another fragment', () => { const result = executeQueryWithFragmentArguments(` query { - ...a(value: null) + ...a } - fragment a($value: String! = "B") on TestType { - fieldWithNullableStringInput(input: $value) + fragment a($a: String! = "B") on TestType { + ...b(b: $a) + } + fragment b($b: String!) on TestType { + fieldWithNonNullableStringInput(input: $b) } `); expect(result).to.deep.equal({ data: { - fieldWithNullableStringInput: 'null', + fieldWithNonNullableStringInput: '"B"', }, }); }); + it('when the definition has a non-nullable default and is provided null', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: null) + } + fragment a($value: String! = "B") on TestType { + fieldWithNullableStringInput(input: $value) + } + `); + + expect(result).to.have.property('errors'); + expect(result.errors).to.have.length(1); + expect(result.errors?.at(0)?.message).to.match( + /Argument "value" of non-null type "String!"/, + ); + }); + it('when the definition has no default and is not provided', () => { const result = executeQueryWithFragmentArguments(` query { @@ -1303,6 +1324,27 @@ describe('Execute: Handles inputs', () => { }); }); + it ('when a fragment-variable is shadowed by an intermediate fragment-spread but defined in the operation-variables', () => { + const result = executeQueryWithFragmentArguments(` + query($x: String = "A") { + ...a + } + fragment a($x: String) on TestType { + ...b + } + + fragment b on TestType { + fieldWithNullableStringInput(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: + '"A"', + }, + }); + }); + it('when a fragment is used with different args', () => { const result = executeQueryWithFragmentArguments(` query($x: String = "Hello") { diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index e917d1a5177..1266d50e5f6 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -22,10 +22,9 @@ import { } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; -import { substituteFragmentArguments } from '../utilities/substituteFragmentArguments.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; -import { getDirectiveValues } from './values.js'; +import { getArgumentValuesFromSpread, getDirectiveValues } from './values.js'; export interface DeferUsage { label: string | undefined; @@ -35,6 +34,7 @@ export interface DeferUsage { export interface FieldDetails { node: FieldNode; deferUsage: DeferUsage | undefined; + fragmentVariableValues?: ObjMap | undefined; } export type FieldGroup = ReadonlyArray; @@ -44,10 +44,10 @@ export type GroupedFieldSet = ReadonlyMap; interface CollectFieldsContext { schema: GraphQLSchema; fragments: ObjMap; - variableValues: { [variable: string]: unknown }; operation: OperationDefinitionNode; runtimeType: GraphQLObjectType; visitedFragmentNames: Set; + variableValues: { [variable: string]: unknown }; } /** @@ -74,8 +74,8 @@ export function collectFields( const context: CollectFieldsContext = { schema, fragments, - variableValues, runtimeType, + variableValues, operation, visitedFragmentNames: new Set(), }; @@ -85,6 +85,7 @@ export function collectFields( operation.selectionSet, groupedFieldSet, newDeferUsages, + variableValues, ); return { groupedFieldSet, newDeferUsages }; } @@ -114,8 +115,8 @@ export function collectSubfields( const context: CollectFieldsContext = { schema, fragments, - variableValues, runtimeType: returnType, + variableValues, operation, visitedFragmentNames: new Set(), }; @@ -130,6 +131,7 @@ export function collectSubfields( node.selectionSet, subGroupedFieldSet, newDeferUsages, + undefined, fieldDetail.deferUsage, ); } @@ -141,18 +143,20 @@ export function collectSubfields( }; } +// eslint-disable-next-line max-params function collectFieldsImpl( context: CollectFieldsContext, selectionSet: SelectionSetNode, groupedFieldSet: AccumulatorMap, newDeferUsages: Array, + fragmentVariableValues?: ObjMap, deferUsage?: DeferUsage, ): void { const { schema, fragments, - variableValues, runtimeType, + variableValues, operation, visitedFragmentNames, } = context; @@ -160,12 +164,14 @@ function collectFieldsImpl( for (const selection of selectionSet.selections) { switch (selection.kind) { case Kind.FIELD: { - if (!shouldIncludeNode(variableValues, selection)) { + const vars = fragmentVariableValues ?? variableValues; + if (!shouldIncludeNode(vars, selection)) { continue; } groupedFieldSet.add(getFieldEntryKey(selection), { node: selection, deferUsage, + fragmentVariableValues: fragmentVariableValues ?? undefined, }); break; } @@ -190,6 +196,7 @@ function collectFieldsImpl( selection.selectionSet, groupedFieldSet, newDeferUsages, + fragmentVariableValues, deferUsage, ); } else { @@ -199,6 +206,7 @@ function collectFieldsImpl( selection.selectionSet, groupedFieldSet, newDeferUsages, + fragmentVariableValues, newDeferUsage, ); } @@ -231,27 +239,43 @@ function collectFieldsImpl( continue; } - const fragmentSelectionSet = substituteFragmentArguments( - fragment, - selection, - ); + // We need to introduce a concept of shadowing: + // + // - when a fragment defines a variable that is in the parent scope but not given + // in the fragment-spread we need to look at this variable as undefined and check + // whether the definition has a defaultValue, if not remove it from the variableValues. + // - when a fragment does not define a variable we need to copy it over from the parent + // scope as that variable can still get used in spreads later on in the selectionSet. + // - when a value is passed in through the fragment-spread we need to copy over the key-value + // into our variable-values. + const fragmentArgValues = fragment.variableDefinitions + ? getArgumentValuesFromSpread( + selection, + schema, + fragment.variableDefinitions, + variableValues, + fragmentVariableValues, + ) + : undefined; if (!newDeferUsage) { visitedFragmentNames.add(fragmentName); collectFieldsImpl( context, - fragmentSelectionSet, + fragment.selectionSet, groupedFieldSet, newDeferUsages, + fragmentArgValues, deferUsage, ); } else { newDeferUsages.push(newDeferUsage); collectFieldsImpl( context, - fragmentSelectionSet, + fragment.selectionSet, groupedFieldSet, newDeferUsages, + fragmentArgValues, newDeferUsage, ); } diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 99582b828dc..47d8bcfb0a6 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -721,9 +721,10 @@ function executeField( // variables scope to fulfill any variable references. // TODO: find a way to memoize, in case this field is within a List type. const args = getArgumentValues( - fieldDef, fieldGroup[0].node, + fieldDef.args, exeContext.variableValues, + fieldGroup[0].fragmentVariableValues, ); // The resolve function's optional third argument is a context value that @@ -1028,7 +1029,7 @@ function getStreamUsage( const stream = getDirectiveValues( GraphQLStreamDirective, fieldGroup[0].node, - exeContext.variableValues, + fieldGroup[0].fragmentVariableValues ?? exeContext.variableValues, ); if (!stream) { @@ -2051,7 +2052,12 @@ function executeSubscription( // Build a JS object of arguments from the field.arguments AST, using the // variables scope to fulfill any variable references. - const args = getArgumentValues(fieldDef, fieldNodes[0], variableValues); + const args = getArgumentValues( + fieldNodes[0], + fieldDef.args, + variableValues, + undefined, + ); // The resolve function's optional third argument is a context value that // is provided to every resolve function within an execution. It is commonly diff --git a/src/execution/values.ts b/src/execution/values.ts index 5511911c782..4565922670c 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -8,12 +8,13 @@ import { GraphQLError } from '../error/GraphQLError.js'; import type { DirectiveNode, FieldNode, + FragmentSpreadNode, VariableDefinitionNode, } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; import { print } from '../language/printer.js'; -import type { GraphQLField } from '../type/definition.js'; +import type { GraphQLArgument } 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'; @@ -21,6 +22,7 @@ import type { GraphQLSchema } from '../type/schema.js'; import { coerceInputValue } from '../utilities/coerceInputValue.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; import { valueFromAST } from '../utilities/valueFromAST.js'; +import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped.js'; type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } @@ -68,6 +70,14 @@ export function getVariableValues( return { errors }; } +/** + * Prepares an object map of argument values given a list of argument + * definitions and list of argument AST nodes. + * + * Note: The returned value is a plain Object with a prototype, since it is + * exposed to user code. Care should be taken to not pull values from the + * Object prototype. + */ function coerceVariableValues( schema: GraphQLSchema, varDefNodes: ReadonlyArray, @@ -149,18 +159,17 @@ function coerceVariableValues( * Object prototype. */ export function getArgumentValues( - def: GraphQLField | GraphQLDirective, node: FieldNode | DirectiveNode, - variableValues?: Maybe>, + argDefs: ReadonlyArray | undefined, + variableValues: Maybe>, + fragmentArgValues?: Maybe>, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; + const argNodeMap = new Map( + node.arguments?.map((arg) => [arg.name.value, arg]), + ); - // FIXME: https://github.com/graphql/graphql-js/issues/2203 - /* c8 ignore next */ - const argumentNodes = node.arguments ?? []; - const argNodeMap = new Map(argumentNodes.map((arg) => [arg.name.value, arg])); - - for (const argDef of def.args) { + for (const argDef of argDefs ?? []) { const name = argDef.name; const argType = argDef.type; const argumentNode = argNodeMap.get(name); @@ -179,29 +188,134 @@ export function getArgumentValues( } const valueNode = argumentNode.value; - let isNull = valueNode.kind === Kind.NULL; + let hasValue = valueNode.kind !== Kind.NULL; if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; if ( - variableValues == null || - !Object.hasOwn(variableValues, variableName) + fragmentArgValues != null && + Object.hasOwn(fragmentArgValues, variableName) ) { - if (argDef.defaultValue !== undefined) { + hasValue = fragmentArgValues[variableName] != null; + if (!hasValue && argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; - } else if (isNonNullType(argType)) { - throw new GraphQLError( - `Argument "${name}" of required type "${inspect(argType)}" ` + - `was provided the variable "$${variableName}" which was not provided a runtime value.`, - { nodes: valueNode }, - ); + continue; + } + } else if ( + variableValues != null && + Object.hasOwn(variableValues, variableName) + ) { + hasValue = variableValues[variableName] != null; + } else if (argDef.defaultValue !== undefined) { + coercedValues[name] = argDef.defaultValue; + continue; + } else if (isNonNullType(argType)) { + throw new GraphQLError( + `Argument "${name}" of required type "${inspect(argType)}" ` + + `was provided the variable "$${variableName}" which was not provided a runtime value.`, + { nodes: valueNode }, + ); + } else { + continue; + } + } + + if (!hasValue && isNonNullType(argType)) { + throw new GraphQLError( + `Argument "${name}" of non-null type "${inspect(argType)}" ` + + 'must not be null.', + { nodes: valueNode }, + ); + } + + // TODO: Make this follow the spec more closely + const coercedValue = valueFromAST(valueNode, argType, { + ...variableValues, + ...fragmentArgValues, + }); + + if (coercedValue === undefined) { + // Note: ValuesOfCorrectTypeRule validation should catch this before + // execution. This is a runtime check to ensure execution does not + // continue with an invalid argument value. + throw new GraphQLError( + `Argument "${name}" has invalid value ${print(valueNode)}.`, + { nodes: valueNode }, + ); + } + coercedValues[name] = coercedValue; + } + return coercedValues; +} + +export function getArgumentValuesFromSpread( + /** NOTE: For error annotations only */ + node: FragmentSpreadNode, + schema: GraphQLSchema, + fragmentVarDefs: ReadonlyArray, + variableValues: Maybe>, + fragmentArgValues?: Maybe>, +): { [argument: string]: unknown } { + const coercedValues: { [argument: string]: unknown } = {}; + const argNodeMap = new Map( + node.arguments?.map((arg) => [arg.name.value, arg]), + ); + + for (const varDef of fragmentVarDefs) { + const name = varDef.variable.name.value; + const argType = typeFromAST(schema, varDef.type); + const argumentNode = argNodeMap.get(name); + + if (argumentNode == null) { + if (varDef.defaultValue !== undefined) { + coercedValues[name] = valueFromASTUntyped(varDef.defaultValue); + } else if (isNonNullType(argType)) { + throw new GraphQLError( + `Argument "${name}" of required type "${inspect(argType)}" ` + + 'was not provided.', + { nodes: node }, + ); + } else { + coercedValues[name] = undefined; + } + continue; + } + + const valueNode = argumentNode.value; + + let hasValue = valueNode.kind !== Kind.NULL; + if (valueNode.kind === Kind.VARIABLE) { + const variableName = valueNode.name.value; + if ( + fragmentArgValues != null && + Object.hasOwn(fragmentArgValues, variableName) + ) { + hasValue = fragmentArgValues[variableName] != null; + if (!hasValue && varDef.defaultValue !== undefined) { + coercedValues[name] = valueFromASTUntyped(varDef.defaultValue); + continue; } + } else if ( + variableValues != null && + Object.hasOwn(variableValues, variableName) + ) { + hasValue = variableValues[variableName] != null; + } else if (varDef.defaultValue !== undefined) { + coercedValues[name] = valueFromASTUntyped(varDef.defaultValue); continue; + } else if (isNonNullType(argType)) { + throw new GraphQLError( + `Argument "${name}" of required type "${inspect(argType)}" ` + + `was provided the variable "$${variableName}" which was not provided a runtime value.`, + { nodes: valueNode }, + ); + } else { + coercedValues[name] = undefined; + hasValue = false; } - isNull = variableValues[variableName] == null; } - if (isNull && isNonNullType(argType)) { + if (!hasValue && isNonNullType(argType)) { throw new GraphQLError( `Argument "${name}" of non-null type "${inspect(argType)}" ` + 'must not be null.', @@ -209,7 +323,15 @@ export function getArgumentValues( ); } - const coercedValue = valueFromAST(valueNode, argType, variableValues); + // TODO: Make this follow the spec more closely + let coercedValue; + if (argType && isInputType(argType)) { + coercedValue = valueFromAST(valueNode, argType, { + ...variableValues, + ...fragmentArgValues, + }); + } + if (coercedValue === undefined) { // Note: ValuesOfCorrectTypeRule validation should catch this before // execution. This is a runtime check to ensure execution does not @@ -245,6 +367,6 @@ export function getDirectiveValues( ); if (directiveNode) { - return getArgumentValues(directiveDef, directiveNode, variableValues); + return getArgumentValues(directiveNode, directiveDef.args, variableValues); } } diff --git a/src/utilities/substituteFragmentArguments.ts b/src/utilities/substituteFragmentArguments.ts deleted file mode 100644 index 3fc62440476..00000000000 --- a/src/utilities/substituteFragmentArguments.ts +++ /dev/null @@ -1,84 +0,0 @@ -import type { Maybe } from '../jsutils/Maybe.js'; - -import type { - ArgumentNode, - FragmentDefinitionNode, - FragmentSpreadNode, - SelectionSetNode, - ValueNode, - VariableDefinitionNode, -} from '../language/ast.js'; -import { Kind } from '../language/kinds.js'; -import { visit } from '../language/visitor.js'; - -/** - * Replaces all fragment argument values with non-fragment-scoped values. - * - * NOTE: fragment arguments are scoped to the fragment they're defined on. - * Therefore, after we apply the passed-in arguments, all remaining variables - * must be either operation defined variables or explicitly unset. - */ -export function substituteFragmentArguments( - def: FragmentDefinitionNode, - fragmentSpread: FragmentSpreadNode, -): SelectionSetNode { - const fragmentDefinitionVariables = def.variableDefinitions; - if ( - fragmentDefinitionVariables == null || - fragmentDefinitionVariables.length === 0 - ) { - return def.selectionSet; - } - - const argumentValues = fragmentArgumentSubstitutions( - fragmentDefinitionVariables, - fragmentSpread.arguments, - ); - - return visit(def.selectionSet, { - Variable(node) { - return argumentValues.get(node.name.value); - }, - }); -} - -export function fragmentArgumentSubstitutions( - variableDefinitions: ReadonlyArray, - argumentValues: Maybe>, -): Map { - const substitutions = new Map(); - if (argumentValues) { - for (const argument of argumentValues) { - substitutions.set(argument.name.value, argument.value); - } - } - - for (const variableDefinition of variableDefinitions) { - const variableName = variableDefinition.variable.name.value; - if (substitutions.has(variableName)) { - continue; - } - - const defaultValue = variableDefinition.defaultValue; - if (defaultValue) { - substitutions.set(variableName, defaultValue); - } else { - // We need a way to allow unset arguments without accidentally - // replacing an unset fragment argument with an operation - // variable value. Fragment arguments must always have LOCAL scope. - // - // To remove this hack, we need to either: - // - include fragment argument scope when evaluating fields - // - make unset fragment arguments invalid - // Requiring the spread to pass all non-default-defined arguments is nice, - // but makes field argument default values impossible to use. - substitutions.set(variableName, { - kind: Kind.VARIABLE, - // We set the variable name to something that is "guaranteed" to be unset - // so that we can leverage default field arguments when defined. - name: { kind: Kind.NAME, value: variableName + '__UNSET' }, - }); - } - } - return substitutions; -}