From e5744fc79ee560eaa55796faeef8dd5261dd912f Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 20 Aug 2024 03:33:01 +0300 Subject: [PATCH] parse fragment signatures when building execution context and reuse getArgumentValues to completely reuse getArgumentValues without greater changes requires omiited arguments to be present as undefined needs further investigation --- src/execution/__tests__/executor-test.ts | 2 +- src/execution/__tests__/variables-test.ts | 2 +- src/execution/collectFields.ts | 41 +++++++--- src/execution/execute.ts | 40 +++++++-- src/execution/values.ts | 82 ++----------------- .../rules/SingleFieldSubscriptionsRule.ts | 12 ++- 6 files changed, 81 insertions(+), 98 deletions(-) diff --git a/src/execution/__tests__/executor-test.ts b/src/execution/__tests__/executor-test.ts index de33f8c91b..3b54591d4a 100644 --- a/src/execution/__tests__/executor-test.ts +++ b/src/execution/__tests__/executor-test.ts @@ -1172,7 +1172,7 @@ describe('Execute: Handles basic execution tasks', () => { const result = executeSync({ schema, document }); expect(result).to.deep.equal({ data: { - field: '{ a: true, c: false, e: 0 }', + field: '{ a: true, b: undefined, c: false, d: undefined, e: 0 }', }, }); }); diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index c35f076a46..76587211c9 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -579,7 +579,7 @@ describe('Execute: Handles inputs', () => { expect(result).to.deep.equal({ data: { - fieldWithNullableStringInput: null, + fieldWithNullableStringInput: 'undefined', }, }); }); diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 42bf344273..47e7dc6d24 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -13,7 +13,10 @@ import type { import { OperationTypeNode } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; -import type { GraphQLObjectType } from '../type/definition.js'; +import type { + GraphQLInputType, + GraphQLObjectType, +} from '../type/definition.js'; import { isAbstractType } from '../type/definition.js'; import { GraphQLDeferDirective, @@ -24,7 +27,7 @@ import type { GraphQLSchema } from '../type/schema.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; -import { getArgumentValuesFromSpread, getDirectiveValues } from './values.js'; +import { getArgumentValues, getDirectiveValues } from './values.js'; export interface DeferUsage { label: string | undefined; @@ -41,9 +44,18 @@ export type FieldGroup = ReadonlyArray; export type GroupedFieldSet = ReadonlyMap; +export interface GraphQLFragmentSignature { + name: string; + type: GraphQLInputType; + defaultValue: unknown; +} + interface CollectFieldsContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap<{ + definition: FragmentDefinitionNode; + signatures: ReadonlyArray; + }>; operation: OperationDefinitionNode; runtimeType: GraphQLObjectType; visitedFragmentNames: Set; @@ -62,7 +74,10 @@ interface CollectFieldsContext { */ export function collectFields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap<{ + definition: FragmentDefinitionNode; + signatures: ReadonlyArray; + }>, variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, operation: OperationDefinitionNode, @@ -104,7 +119,10 @@ export function collectFields( // eslint-disable-next-line max-params export function collectSubfields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap<{ + definition: FragmentDefinitionNode; + signatures: ReadonlyArray; + }>, variableValues: { [variable: string]: unknown }, operation: OperationDefinitionNode, returnType: GraphQLObjectType, @@ -232,7 +250,7 @@ function collectFieldsImpl( const fragment = fragments[fragmentName]; if ( fragment == null || - !doesFragmentConditionMatch(schema, fragment, runtimeType) + !doesFragmentConditionMatch(schema, fragment.definition, runtimeType) ) { continue; } @@ -246,11 +264,10 @@ function collectFieldsImpl( // 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( + context.localVariableValues = fragment.definition.variableDefinitions + ? getArgumentValues( selection, - schema, - fragment.variableDefinitions, + fragment.signatures, variableValues, context.localVariableValues, ) @@ -260,7 +277,7 @@ function collectFieldsImpl( visitedFragmentNames.add(fragmentName); collectFieldsImpl( context, - fragment.selectionSet, + fragment.definition.selectionSet, groupedFieldSet, newDeferUsages, deferUsage, @@ -269,7 +286,7 @@ function collectFieldsImpl( newDeferUsages.push(newDeferUsage); collectFieldsImpl( context, - fragment.selectionSet, + fragment.definition.selectionSet, groupedFieldSet, newDeferUsages, newDeferUsage, diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 47d8bcfb0a..981fb110ec 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'; @@ -39,6 +40,7 @@ import type { } from '../type/definition.js'; import { isAbstractType, + isInputType, isLeafType, isListType, isNonNullType, @@ -48,11 +50,15 @@ import { GraphQLStreamDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; import { assertValidSchema } from '../type/validate.js'; +import { typeFromAST } from '../utilities/typeFromAST.js'; +import { valueFromAST } from '../utilities/valueFromAST.js'; + import type { DeferUsageSet, ExecutionPlan } from './buildExecutionPlan.js'; import { buildExecutionPlan } from './buildExecutionPlan.js'; import type { DeferUsage, FieldGroup, + GraphQLFragmentSignature, GroupedFieldSet, } from './collectFields.js'; import { @@ -132,7 +138,10 @@ const collectSubfields = memoize3( */ export interface ExecutionContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap<{ + definition: FragmentDefinitionNode; + signatures: ReadonlyArray; + }>; rootValue: unknown; contextValue: unknown; operation: OperationDefinitionNode; @@ -445,7 +454,10 @@ export function buildExecutionContext( assertValidSchema(schema); let operation: OperationDefinitionNode | undefined; - const fragments: ObjMap = Object.create(null); + const fragments: ObjMap<{ + definition: FragmentDefinitionNode; + signatures: ReadonlyArray; + }> = Object.create(null); for (const definition of document.definitions) { switch (definition.kind) { case Kind.OPERATION_DEFINITION: @@ -462,9 +474,24 @@ export function buildExecutionContext( operation = definition; } break; - case Kind.FRAGMENT_DEFINITION: - fragments[definition.name.value] = definition; + case Kind.FRAGMENT_DEFINITION: { + const signatures: Array = []; + if (definition.variableDefinitions) { + for (const varDef of definition.variableDefinitions) { + const varType = typeFromAST(schema, varDef.type); + if (isInputType(varType)) { + const signature: GraphQLFragmentSignature = { + name: varDef.variable.name.value, + type: varType, + defaultValue: valueFromAST(varDef.defaultValue, varType), + }; + signatures.push(signature); + } + } + } + fragments[definition.name.value] = { definition, signatures }; break; + } default: // ignore non-executable definitions } @@ -807,7 +834,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, diff --git a/src/execution/values.ts b/src/execution/values.ts index f219310721..e4c697c835 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -22,7 +22,8 @@ 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'; + +import type { GraphQLFragmentSignature } from './collectFields.js'; type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } @@ -159,8 +160,8 @@ function coerceVariableValues( * Object prototype. */ export function getArgumentValues( - node: FieldNode | DirectiveNode, - argDefs: ReadonlyArray, + node: FieldNode | DirectiveNode | FragmentSpreadNode, + argDefs: ReadonlyArray, variableValues: Maybe>, fragmentArgValues?: Maybe>, ): { [argument: string]: unknown } { @@ -183,6 +184,8 @@ export function getArgumentValues( 'was not provided.', { nodes: node }, ); + } else { + coercedValues[name] = undefined; } continue; } @@ -248,79 +251,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 diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 700bc0bda7..86a60b91b1 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -10,7 +10,10 @@ import type { import { Kind } from '../../language/kinds.js'; import type { ASTVisitor } from '../../language/visitor.js'; -import type { FieldGroup } from '../../execution/collectFields.js'; +import type { + FieldGroup, + GraphQLFragmentSignature, +} from '../../execution/collectFields.js'; import { collectFields } from '../../execution/collectFields.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -41,10 +44,13 @@ export function SingleFieldSubscriptionsRule( [variable: string]: any; } = Object.create(null); const document = context.getDocument(); - const fragments: ObjMap = Object.create(null); + const fragments: ObjMap<{ + definition: FragmentDefinitionNode; + signatures: ReadonlyArray; + }> = 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, signatures: [] }; } } const { groupedFieldSet } = collectFields(