From 9f60aa8dfad86d931e90424557c8ad34ff9ac2dd Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Sun, 25 Aug 2024 12:28:46 +0300 Subject: [PATCH] add directive test (#9) * add directive test * add failing test add additional nested fragment test (#8) Correct test and lint stuff suggestions for execution (#11) * introduce internal getVariableSignature utility now extracted also to graphql-js PR, see https://github.com/graphql/graphql-js/pull/4175 * execution suggestions fixes execution to always use fragment variable when has the same name as an operation variable previously, we were allowing an operation variable to be used if the fragment variable was not provided, and the field had no default. Now, we still use the fragment variable, and so the value is null. this now correct logic allows us to significantly reduce the diff from main adds additional test --- src/execution/__tests__/variables-test.ts | 104 +++++++++- src/execution/collectFields.ts | 113 ++++++----- src/execution/execute.ts | 43 ++-- src/execution/values.ts | 186 +++++------------- src/utilities/getVariableSignature.ts | 44 +++++ src/utilities/valueFromAST.ts | 39 +++- .../rules/SingleFieldSubscriptionsRule.ts | 15 +- 7 files changed, 333 insertions(+), 211 deletions(-) create mode 100644 src/utilities/getVariableSignature.ts diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index c35f076a46..ac810c4eeb 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -7,6 +7,7 @@ import { inspect } from '../../jsutils/inspect.js'; import { GraphQLError } from '../../error/GraphQLError.js'; +import { DirectiveLocation } from '../../language/directiveLocation.js'; import { Kind } from '../../language/kinds.js'; import { parse } from '../../language/parser.js'; @@ -22,10 +23,14 @@ import { GraphQLObjectType, GraphQLScalarType, } from '../../type/definition.js'; -import { GraphQLString } from '../../type/scalars.js'; +import { + GraphQLDirective, + GraphQLIncludeDirective, +} from '../../type/directives.js'; +import { GraphQLBoolean, GraphQLString } from '../../type/scalars.js'; import { GraphQLSchema } from '../../type/schema.js'; -import { executeSync } from '../execute.js'; +import { executeSync, experimentalExecuteIncrementally } from '../execute.js'; import { getVariableValues } from '../values.js'; const TestFaultyScalarGraphQLError = new GraphQLError( @@ -154,7 +159,30 @@ const TestType = new GraphQLObjectType({ }, }); -const schema = new GraphQLSchema({ query: TestType }); +const schema = new GraphQLSchema({ + query: TestType, + directives: [ + new GraphQLDirective({ + name: 'skip', + description: + 'Directs the executor to skip this field or fragment when the `if` argument is true.', + locations: [ + DirectiveLocation.FIELD, + DirectiveLocation.FRAGMENT_SPREAD, + DirectiveLocation.INLINE_FRAGMENT, + ], + args: { + if: { + type: new GraphQLNonNull(GraphQLBoolean), + description: 'Skipped when true.', + // default values will override operation variables in the setting of defined fragment variables that are not provided + defaultValue: true, + }, + }, + }), + GraphQLIncludeDirective, + ], +}); function executeQuery( query: string, @@ -1307,6 +1335,22 @@ describe('Execute: Handles inputs', () => { }); }); + it('when a nullable argument without a field default is not provided and shadowed by an operation variable', () => { + const result = executeQueryWithFragmentArguments(` + query($x: String = "A") { + ...a + } + fragment a($x: String) on TestType { + fieldWithNullableStringInput(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: null, + }, + }); + }); + it('when a nullable argument with a field default is not provided and shadowed by an operation variable', () => { const result = executeQueryWithFragmentArguments(` query($x: String = "A") { @@ -1412,6 +1456,27 @@ describe('Execute: Handles inputs', () => { }); }); + it('when argument variables with the same name are used directly and recursively', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: "A") + } + fragment a($value: String!) on TestType { + ...b(value: "B") + fieldInFragmentA: fieldWithNonNullableStringInput(input: $value) + } + fragment b($value: String!) on TestType { + fieldInFragmentB: fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldInFragmentA: '"A"', + fieldInFragmentB: '"B"', + }, + }); + }); + it('when argument passed in as list', () => { const result = executeQueryWithFragmentArguments(` query Q($opValue: String = "op") { @@ -1434,5 +1499,38 @@ describe('Execute: Handles inputs', () => { }, }); }); + + it('when argument passed to a directive', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: true) + } + fragment a($value: Boolean!) on TestType { + fieldWithNonNullableStringInput @skip(if: $value) + } + `); + expect(result).to.deep.equal({ + data: {}, + }); + }); + + it('when a nullable argument to a directive with a field default is not provided and shadowed by an operation variable', () => { + // this test uses the @defer directive and incremental delivery because the `if` argument for skip/include have no field defaults + const document = parse( + ` + query($shouldDefer: Boolean = false) { + ...a + } + fragment a($shouldDefer: Boolean) on TestType { + ... @defer(if: $shouldDefer) { + fieldWithDefaultArgumentValue + } + } + `, + { experimentalFragmentArguments: true }, + ); + const result = experimentalExecuteIncrementally({ schema, document }); + expect(result).to.include.keys('initialResult', 'subsequentResults'); + }); }); }); diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 25ec6ef938..68261c7194 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -22,33 +22,44 @@ import { } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; +import type { GraphQLVariableSignature } from '../utilities/getVariableSignature.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; -import { getArgumentValuesFromSpread, getDirectiveValues } from './values.js'; +import { experimentalGetArgumentValues, getDirectiveValues } 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; - fragmentVariableValues?: ObjMap | undefined; + deferUsage?: DeferUsage | undefined; + fragmentVariables?: FragmentVariables | undefined; } export type FieldGroup = ReadonlyArray; export type GroupedFieldSet = ReadonlyMap; +export interface FragmentDetails { + definition: FragmentDefinitionNode; + variableSignatures?: ObjMap | undefined; +} + interface CollectFieldsContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap; + variableValues: { [variable: string]: unknown }; + fragmentVariableValues?: FragmentVariables; operation: OperationDefinitionNode; runtimeType: GraphQLObjectType; visitedFragmentNames: Set; - localVariableValues: { [variable: string]: unknown } | undefined; - variableValues: { [variable: string]: unknown }; } /** @@ -62,7 +73,7 @@ interface CollectFieldsContext { */ export function collectFields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, operation: OperationDefinitionNode, @@ -75,10 +86,9 @@ export function collectFields( const context: CollectFieldsContext = { schema, fragments, - runtimeType, variableValues, + runtimeType, operation, - localVariableValues: undefined, visitedFragmentNames: new Set(), }; @@ -104,7 +114,7 @@ export function collectFields( // eslint-disable-next-line max-params export function collectSubfields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, operation: OperationDefinitionNode, returnType: GraphQLObjectType, @@ -116,9 +126,8 @@ export function collectSubfields( const context: CollectFieldsContext = { schema, fragments, - runtimeType: returnType, - localVariableValues: undefined, variableValues, + runtimeType: returnType, operation, visitedFragmentNames: new Set(), }; @@ -144,19 +153,20 @@ export function collectSubfields( }; } +// eslint-disable-next-line max-params function collectFieldsImpl( context: CollectFieldsContext, selectionSet: SelectionSetNode, groupedFieldSet: AccumulatorMap, newDeferUsages: Array, deferUsage?: DeferUsage, + fragmentVariables?: FragmentVariables, ): void { const { schema, fragments, - runtimeType, variableValues, - localVariableValues, + runtimeType, operation, visitedFragmentNames, } = context; @@ -164,20 +174,19 @@ function collectFieldsImpl( for (const selection of selectionSet.selections) { switch (selection.kind) { case Kind.FIELD: { - const vars = localVariableValues ?? variableValues; - if (!shouldIncludeNode(vars, selection)) { + if (!shouldIncludeNode(selection, variableValues, fragmentVariables)) { continue; } groupedFieldSet.add(getFieldEntryKey(selection), { node: selection, deferUsage, - fragmentVariableValues: localVariableValues ?? undefined, + fragmentVariables, }); break; } case Kind.INLINE_FRAGMENT: { if ( - !shouldIncludeNode(variableValues, selection) || + !shouldIncludeNode(selection, variableValues, fragmentVariables) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { continue; @@ -186,6 +195,7 @@ function collectFieldsImpl( const newDeferUsage = getDeferUsage( operation, variableValues, + fragmentVariables, selection, deferUsage, ); @@ -197,6 +207,7 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, deferUsage, + fragmentVariables, ); } else { newDeferUsages.push(newDeferUsage); @@ -206,73 +217,72 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, newDeferUsage, + fragmentVariables, ); } break; } case Kind.FRAGMENT_SPREAD: { - const fragmentName = selection.name.value; + const fragName = selection.name.value; const newDeferUsage = getDeferUsage( operation, variableValues, + fragmentVariables, selection, deferUsage, ); if ( !newDeferUsage && - (visitedFragmentNames.has(fragmentName) || - !shouldIncludeNode(variableValues, selection)) + (visitedFragmentNames.has(fragName) || + !shouldIncludeNode(selection, variableValues, fragmentVariables)) ) { continue; } - const fragment = fragments[fragmentName]; + const fragment = fragments[fragName]; if ( fragment == null || - !doesFragmentConditionMatch(schema, fragment, runtimeType) + !doesFragmentConditionMatch(schema, fragment.definition, runtimeType) ) { continue; } - // 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. - context.localVariableValues = fragment.variableDefinitions - ? getArgumentValuesFromSpread( + const fragmentVariableSignatures = fragment.variableSignatures; + let newFragmentVariables: FragmentVariables | undefined; + if (fragmentVariableSignatures) { + newFragmentVariables = { + signatures: fragmentVariableSignatures, + values: experimentalGetArgumentValues( selection, - schema, - fragment.variableDefinitions, + Object.values(fragmentVariableSignatures), variableValues, - context.localVariableValues, - ) - : undefined; + fragmentVariables, + ), + }; + } if (!newDeferUsage) { - visitedFragmentNames.add(fragmentName); + visitedFragmentNames.add(fragName); collectFieldsImpl( context, - fragment.selectionSet, + fragment.definition.selectionSet, groupedFieldSet, newDeferUsages, deferUsage, + newFragmentVariables, ); } else { newDeferUsages.push(newDeferUsage); collectFieldsImpl( context, - fragment.selectionSet, + fragment.definition.selectionSet, groupedFieldSet, newDeferUsages, newDeferUsage, + newFragmentVariables, ); } break; @@ -289,10 +299,16 @@ function collectFieldsImpl( function getDeferUsage( operation: OperationDefinitionNode, variableValues: { [variable: string]: unknown }, + fragmentVariables: FragmentVariables | undefined, node: FragmentSpreadNode | InlineFragmentNode, parentDeferUsage: DeferUsage | undefined, ): DeferUsage | undefined { - const defer = getDirectiveValues(GraphQLDeferDirective, node, variableValues); + const defer = getDirectiveValues( + GraphQLDeferDirective, + node, + variableValues, + fragmentVariables, + ); if (!defer) { return; @@ -318,10 +334,16 @@ function getDeferUsage( * directives, where `@skip` has higher precedence than `@include`. */ function shouldIncludeNode( - variableValues: { [variable: string]: unknown }, node: FragmentSpreadNode | FieldNode | InlineFragmentNode, + variableValues: { [variable: string]: unknown }, + fragmentVariables: FragmentVariables | undefined, ): boolean { - const skip = getDirectiveValues(GraphQLSkipDirective, node, variableValues); + const skip = getDirectiveValues( + GraphQLSkipDirective, + node, + variableValues, + fragmentVariables, + ); if (skip?.if === true) { return false; } @@ -330,6 +352,7 @@ function shouldIncludeNode( GraphQLIncludeDirective, node, variableValues, + fragmentVariables, ); if (include?.if === false) { return false; diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 47d8bcfb0a..f394ac2bae 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -5,6 +5,7 @@ import { isAsyncIterable } from '../jsutils/isAsyncIterable.js'; import { isIterableObject } from '../jsutils/isIterableObject.js'; import { isObjectLike } from '../jsutils/isObjectLike.js'; import { isPromise } from '../jsutils/isPromise.js'; +import { mapValue } from '../jsutils/mapValue.js'; import type { Maybe } from '../jsutils/Maybe.js'; import { memoize3 } from '../jsutils/memoize3.js'; import type { ObjMap } from '../jsutils/ObjMap.js'; @@ -20,7 +21,6 @@ import { locatedError } from '../error/locatedError.js'; import type { DocumentNode, FieldNode, - FragmentDefinitionNode, OperationDefinitionNode, } from '../language/ast.js'; import { OperationTypeNode } from '../language/ast.js'; @@ -48,11 +48,14 @@ import { GraphQLStreamDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; import { assertValidSchema } from '../type/validate.js'; +import { getVariableSignature } from '../utilities/getVariableSignature.js'; + import type { DeferUsageSet, ExecutionPlan } from './buildExecutionPlan.js'; import { buildExecutionPlan } from './buildExecutionPlan.js'; import type { DeferUsage, FieldGroup, + FragmentDetails, GroupedFieldSet, } from './collectFields.js'; import { @@ -74,6 +77,7 @@ import type { } from './types.js'; import { DeferredFragmentRecord } from './types.js'; import { + experimentalGetArgumentValues, getArgumentValues, getDirectiveValues, getVariableValues, @@ -132,7 +136,7 @@ const collectSubfields = memoize3( */ export interface ExecutionContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap; rootValue: unknown; contextValue: unknown; operation: OperationDefinitionNode; @@ -445,7 +449,7 @@ export function buildExecutionContext( assertValidSchema(schema); let operation: OperationDefinitionNode | undefined; - const fragments: ObjMap = Object.create(null); + const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) { switch (definition.kind) { case Kind.OPERATION_DEFINITION: @@ -462,9 +466,18 @@ export function buildExecutionContext( operation = definition; } break; - case Kind.FRAGMENT_DEFINITION: - fragments[definition.name.value] = definition; + case Kind.FRAGMENT_DEFINITION: { + let variableSignatures; + if (definition.variableDefinitions) { + variableSignatures = Object.create(null); + for (const varDef of definition.variableDefinitions) { + const signature = getVariableSignature(schema, varDef); + variableSignatures[signature.name] = signature; + } + } + fragments[definition.name.value] = { definition, variableSignatures }; break; + } default: // ignore non-executable definitions } @@ -720,11 +733,11 @@ function executeField( // Build a JS object of arguments from the field.arguments AST, using the // 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( + const args = experimentalGetArgumentValues( fieldGroup[0].node, fieldDef.args, exeContext.variableValues, - fieldGroup[0].fragmentVariableValues, + fieldGroup[0].fragmentVariables, ); // The resolve function's optional third argument is a context value that @@ -807,7 +820,10 @@ export function buildResolveInfo( parentType, path, schema: exeContext.schema, - fragments: exeContext.fragments, + fragments: mapValue( + exeContext.fragments, + (fragment) => fragment.definition, + ), rootValue: exeContext.rootValue, operation: exeContext.operation, variableValues: exeContext.variableValues, @@ -1029,7 +1045,8 @@ function getStreamUsage( const stream = getDirectiveValues( GraphQLStreamDirective, fieldGroup[0].node, - fieldGroup[0].fragmentVariableValues ?? exeContext.variableValues, + exeContext.variableValues, + fieldGroup[0].fragmentVariables, ); if (!stream) { @@ -1058,6 +1075,7 @@ function getStreamUsage( const streamedFieldGroup: FieldGroup = fieldGroup.map((fieldDetails) => ({ node: fieldDetails.node, deferUsage: undefined, + fragmentVariables: fieldDetails.fragmentVariables, })); const streamUsage = { @@ -2052,12 +2070,7 @@ 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( - fieldNodes[0], - fieldDef.args, - variableValues, - undefined, - ); + const args = getArgumentValues(fieldDef, fieldNodes[0], variableValues); // 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 f219310721..a9420df718 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -14,15 +14,17 @@ import type { import { Kind } from '../language/kinds.js'; import { print } from '../language/printer.js'; -import type { GraphQLArgument } from '../type/definition.js'; -import { isInputType, isNonNullType } from '../type/definition.js'; +import type { GraphQLArgument, GraphQLField } from '../type/definition.js'; +import { isNonNullType } from '../type/definition.js'; import type { GraphQLDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; import { coerceInputValue } from '../utilities/coerceInputValue.js'; -import { typeFromAST } from '../utilities/typeFromAST.js'; +import type { GraphQLVariableSignature } from '../utilities/getVariableSignature.js'; +import { getVariableSignature } from '../utilities/getVariableSignature.js'; import { valueFromAST } from '../utilities/valueFromAST.js'; -import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped.js'; + +import type { FragmentVariables } from './collectFields.js'; type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } @@ -70,14 +72,6 @@ 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, @@ -86,24 +80,16 @@ function coerceVariableValues( ): { [variable: string]: unknown } { const coercedValues: { [variable: string]: unknown } = {}; for (const varDefNode of varDefNodes) { - const varName = varDefNode.variable.name.value; - const varType = typeFromAST(schema, varDefNode.type); - if (!isInputType(varType)) { - // Must use input types for variables. This should be caught during - // validation, however is checked again here for safety. - const varTypeStr = print(varDefNode.type); - onError( - new GraphQLError( - `Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`, - { nodes: varDefNode.type }, - ), - ); + const varSignature = getVariableSignature(schema, varDefNode); + if (varSignature instanceof GraphQLError) { + onError(varSignature); continue; } + const { name: varName, type: varType } = varSignature; if (!Object.hasOwn(inputs, varName)) { if (varDefNode.defaultValue) { - coercedValues[varName] = valueFromAST(varDefNode.defaultValue, varType); + coercedValues[varName] = varSignature.defaultValue; } else if (isNonNullType(varType)) { const varTypeStr = inspect(varType); onError( @@ -159,15 +145,25 @@ function coerceVariableValues( * Object prototype. */ export function getArgumentValues( + def: GraphQLField | GraphQLDirective, node: FieldNode | DirectiveNode, - argDefs: ReadonlyArray, + variableValues?: Maybe>, +): { [argument: string]: unknown } { + return experimentalGetArgumentValues(node, def.args, variableValues); +} + +export function experimentalGetArgumentValues( + node: FieldNode | DirectiveNode | FragmentSpreadNode, + argDefs: ReadonlyArray, variableValues: Maybe>, - fragmentArgValues?: Maybe>, + fragmentVariables?: 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 argDefs) { const name = argDef.name; @@ -188,39 +184,32 @@ 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; + const scopedVariableValues = fragmentVariables?.signatures[variableName] + ? fragmentVariables.values + : variableValues; if ( - fragmentArgValues != null && - Object.hasOwn(fragmentArgValues, variableName) + scopedVariableValues == null || + !Object.hasOwn(scopedVariableValues, variableName) ) { - hasValue = fragmentArgValues[variableName] != null; - if (!hasValue && argDef.defaultValue !== undefined) { + 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 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; } + isNull = scopedVariableValues[variableName] == null; } - if (!hasValue && isNonNullType(argType)) { + if (isNull && isNonNullType(argType)) { throw new GraphQLError( `Argument "${name}" of non-null type "${inspect(argType)}" ` + 'must not be null.', @@ -228,12 +217,12 @@ export function getArgumentValues( ); } - // TODO: Make this follow the spec more closely - const coercedValue = valueFromAST(valueNode, argType, { - ...variableValues, - ...fragmentArgValues, - }); - + const coercedValue = valueFromAST( + valueNode, + argType, + variableValues, + fragmentVariables?.values, + ); if (coercedValue === undefined) { // Note: ValuesOfCorrectTypeRule validation should catch this before // execution. This is a runtime check to ensure execution does not @@ -248,79 +237,6 @@ export function getArgumentValues( 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; - } else if ( - variableValues != null && - Object.hasOwn(variableValues, variableName) - ) { - hasValue = variableValues[variableName] != null; - } - } - - 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 - let coercedValue; - if (argType && isInputType(argType)) { - coercedValue = valueFromAST(valueNode, argType, { - ...variableValues, - ...fragmentArgValues, - }); - } - - coercedValues[name] = coercedValue; - } - return coercedValues; -} - /** * Prepares an object map of argument values given a directive definition * and a AST node which may contain directives. Optionally also accepts a map @@ -336,12 +252,18 @@ export function getDirectiveValues( directiveDef: GraphQLDirective, node: { readonly directives?: ReadonlyArray | undefined }, variableValues?: Maybe>, + fragmentVariables?: Maybe, ): undefined | { [argument: string]: unknown } { const directiveNode = node.directives?.find( (directive) => directive.name.value === directiveDef.name, ); if (directiveNode) { - return getArgumentValues(directiveNode, directiveDef.args, variableValues); + return experimentalGetArgumentValues( + directiveNode, + directiveDef.args, + variableValues, + fragmentVariables, + ); } } diff --git a/src/utilities/getVariableSignature.ts b/src/utilities/getVariableSignature.ts new file mode 100644 index 0000000000..4ff7e977e4 --- /dev/null +++ b/src/utilities/getVariableSignature.ts @@ -0,0 +1,44 @@ +import { GraphQLError } from '../error/GraphQLError.js'; + +import { print } from '../language/printer.js'; + +import type { GraphQLInputType, GraphQLSchema } from '../type/index.js'; +import { isInputType } from '../type/index.js'; + +import type { VariableDefinitionNode } from '../index.js'; + +import { typeFromAST } from './typeFromAST.js'; +import { valueFromAST } from './valueFromAST.js'; + +/** + * A GraphQLVariableSignature is required to coerce a variable value. + * */ +export interface GraphQLVariableSignature { + name: string; + type: GraphQLInputType; + defaultValue: unknown; +} + +export function getVariableSignature( + schema: GraphQLSchema, + varDefNode: VariableDefinitionNode, +): GraphQLVariableSignature | GraphQLError { + const varName = varDefNode.variable.name.value; + const varType = typeFromAST(schema, varDefNode.type); + + if (!isInputType(varType)) { + // Must use input types for variables. This should be caught during + // validation, however is checked again here for safety. + const varTypeStr = print(varDefNode.type); + return new GraphQLError( + `Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`, + { nodes: varDefNode.type }, + ); + } + + return { + name: varName, + type: varType, + defaultValue: valueFromAST(varDefNode.defaultValue, varType), + }; +} diff --git a/src/utilities/valueFromAST.ts b/src/utilities/valueFromAST.ts index 5e0d3c517e..3aec3f272f 100644 --- a/src/utilities/valueFromAST.ts +++ b/src/utilities/valueFromAST.ts @@ -38,6 +38,7 @@ export function valueFromAST( valueNode: Maybe, type: GraphQLInputType, variables?: Maybe>, + fragmentVariables?: Maybe>, ): unknown { if (!valueNode) { // When there is no node, then there is also no value. @@ -47,11 +48,12 @@ export function valueFromAST( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; - if (variables == null || variables[variableName] === undefined) { + const variableValue = + fragmentVariables?.[variableName] ?? variables?.[variableName]; + if (variableValue === undefined) { // No valid return value. return; } - const variableValue = variables[variableName]; if (variableValue === null && isNonNullType(type)) { return; // Invalid: intentionally return no value. } @@ -65,7 +67,7 @@ export function valueFromAST( if (valueNode.kind === Kind.NULL) { return; // Invalid: intentionally return no value. } - return valueFromAST(valueNode, type.ofType, variables); + return valueFromAST(valueNode, type.ofType, variables, fragmentVariables); } if (valueNode.kind === Kind.NULL) { @@ -78,7 +80,7 @@ export function valueFromAST( if (valueNode.kind === Kind.LIST) { const coercedValues = []; for (const itemNode of valueNode.values) { - if (isMissingVariable(itemNode, variables)) { + if (isMissingVariable(itemNode, variables, fragmentVariables)) { // If an array contains a missing variable, it is either coerced to // null or if the item type is non-null, it considered invalid. if (isNonNullType(itemType)) { @@ -86,7 +88,12 @@ export function valueFromAST( } coercedValues.push(null); } else { - const itemValue = valueFromAST(itemNode, itemType, variables); + const itemValue = valueFromAST( + itemNode, + itemType, + variables, + fragmentVariables, + ); if (itemValue === undefined) { return; // Invalid: intentionally return no value. } @@ -95,7 +102,12 @@ export function valueFromAST( } return coercedValues; } - const coercedValue = valueFromAST(valueNode, itemType, variables); + const coercedValue = valueFromAST( + valueNode, + itemType, + variables, + fragmentVariables, + ); if (coercedValue === undefined) { return; // Invalid: intentionally return no value. } @@ -112,7 +124,10 @@ export function valueFromAST( ); for (const field of Object.values(type.getFields())) { const fieldNode = fieldNodes.get(field.name); - if (fieldNode == null || isMissingVariable(fieldNode.value, variables)) { + if ( + fieldNode == null || + isMissingVariable(fieldNode.value, variables, fragmentVariables) + ) { if (field.defaultValue !== undefined) { coercedObj[field.name] = field.defaultValue; } else if (isNonNullType(field.type)) { @@ -120,7 +135,12 @@ export function valueFromAST( } continue; } - const fieldValue = valueFromAST(fieldNode.value, field.type, variables); + const fieldValue = valueFromAST( + fieldNode.value, + field.type, + variables, + fragmentVariables, + ); if (fieldValue === undefined) { return; // Invalid: intentionally return no value. } @@ -166,9 +186,12 @@ export function valueFromAST( function isMissingVariable( valueNode: ValueNode, variables: Maybe>, + fragmentVariables: Maybe>, ): boolean { return ( valueNode.kind === Kind.VARIABLE && + (fragmentVariables == null || + fragmentVariables[valueNode.name.value] === undefined) && (variables == null || variables[valueNode.name.value] === undefined) ); } diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 700bc0bda7..dfefc6cdd8 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -2,15 +2,14 @@ import type { ObjMap } from '../../jsutils/ObjMap.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { - FieldNode, - FragmentDefinitionNode, - OperationDefinitionNode, -} from '../../language/ast.js'; +import type { FieldNode, OperationDefinitionNode } from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import type { ASTVisitor } from '../../language/visitor.js'; -import type { FieldGroup } from '../../execution/collectFields.js'; +import type { + FieldGroup, + FragmentDetails, +} from '../../execution/collectFields.js'; import { collectFields } from '../../execution/collectFields.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -41,10 +40,10 @@ export function SingleFieldSubscriptionsRule( [variable: string]: any; } = Object.create(null); const document = context.getDocument(); - const fragments: ObjMap = Object.create(null); + const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) { if (definition.kind === Kind.FRAGMENT_DEFINITION) { - fragments[definition.name.value] = definition; + fragments[definition.name.value] = { definition }; } } const { groupedFieldSet } = collectFields(