diff --git a/change/@graphitation-supermassive-b78db284-0803-439d-939a-bdb7591e5872.json b/change/@graphitation-supermassive-b78db284-0803-439d-939a-bdb7591e5872.json new file mode 100644 index 000000000..c263b6c40 --- /dev/null +++ b/change/@graphitation-supermassive-b78db284-0803-439d-939a-bdb7591e5872.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Delay variable coercion until argument use.", + "packageName": "@graphitation/supermassive", + "email": "vrazuvaev@microsoft.com_msteamsmdb", + "dependentChangeType": "patch" +} diff --git a/packages/supermassive/src/__tests__/execute.test.ts b/packages/supermassive/src/__tests__/execute.test.ts index d1edaa8d0..89f49ebc8 100644 --- a/packages/supermassive/src/__tests__/execute.test.ts +++ b/packages/supermassive/src/__tests__/execute.test.ts @@ -3,6 +3,8 @@ import { execute as graphQLExecute, subscribe as graphQLSubscribe, GraphQLSchema, + GraphQLScalarType, + Kind, } from "graphql"; import { makeSchema } from "../benchmarks/swapi-schema"; import models from "../benchmarks/swapi-schema/models"; @@ -677,4 +679,188 @@ describe("executeWithoutSchema - regression tests", () => { const events = await drainExecution(result); expect(events).toMatchSnapshot(); }); + + test("coerces input variables after loading the root field schema fragment", async () => { + expect.assertions(3); + + let resolvedArgs: unknown; + const loaderRequests: string[] = []; + + const result = await executeWithoutSchema({ + document: parse(` + mutation useReorderConversationFoldersMutation( + $input: OneGQL_ReorderConversationFoldersInput! + ) { + reorderConversationFolders(input: $input) + } + `), + variableValues: { + input: { + ids: ["folder-1"], + }, + }, + schemaFragment: { + schemaId: "test", + definitions: { types: {} }, + resolvers: { + Mutation: { + reorderConversationFolders( + _source: unknown, + args: { input: { ids: string[] } }, + ) { + resolvedArgs = args; + return args.input.ids[0]; + }, + }, + }, + }, + schemaFragmentLoader: (currentFragment, _context, req) => { + if (req.kind !== "ReturnType") { + throw new Error(`Unexpected schema fragment request: ${req.kind}`); + } + loaderRequests.push( + `${req.kind}:${req.parentTypeName}.${req.fieldName}`, + ); + currentFragment.definitions = encodeASTSchema( + parse(` + type Mutation { + reorderConversationFolders(input: OneGQL_ReorderConversationFoldersInput!): ID! + } + + input OneGQL_ReorderConversationFoldersInput { + ids: [ID!]! + folderId: ID = "default-folder" + } + `), + )[0]; + return Promise.resolve({ mergedFragment: currentFragment }); + }, + }); + + expect(await drainExecution(result)).toEqual({ + data: { + reorderConversationFolders: "folder-1", + }, + }); + expect(resolvedArgs).toEqual({ + input: { + ids: ["folder-1"], + folderId: "default-folder", + }, + }); + expect(loaderRequests).toEqual([ + "ReturnType:Mutation.reorderConversationFolders", + ]); + }); + + test("does not load schema fragments for unused input variable definitions", async () => { + expect.assertions(2); + + const result = await executeWithoutSchema({ + document: parse(` + mutation WithUnusedInput($input: MissingInput) { + ping + } + `), + variableValues: { + input: { + id: "unused", + }, + }, + schemaFragment: { + schemaId: "test", + definitions: { types: {} }, + resolvers: { + Mutation: { + ping() { + return "pong"; + }, + }, + }, + }, + schemaFragmentLoader: (currentFragment, _context, req) => { + expect(req).toMatchObject({ + kind: "ReturnType", + parentTypeName: "Mutation", + fieldName: "ping", + }); + currentFragment.definitions = encodeASTSchema( + parse(` + type Mutation { + ping: String! + } + `), + )[0]; + return Promise.resolve({ mergedFragment: currentFragment }); + }, + }); + + expect(await drainExecution(result)).toEqual({ data: { ping: "pong" } }); + }); + + test("coerces a used variable only once", async () => { + expect.assertions(3); + + let parseValueCalls = 0; + const argsAtResolver: unknown[] = []; + const CountedScalar = new GraphQLScalarType({ + name: "CountedScalar", + serialize(value) { + return value; + }, + parseValue(value) { + parseValueCalls++; + return `coerced:${value}`; + }, + parseLiteral(node) { + return node.kind === Kind.STRING ? `literal:${node.value}` : undefined; + }, + }); + const definitions = encodeASTSchema( + parse(` + scalar CountedScalar + + type Query { + echo(value: CountedScalar!): String! + } + `), + )[0]; + + const result = await executeWithoutSchema({ + document: parse(` + query UsesVariableTwice($value: CountedScalar!) { + first: echo(value: $value) + second: echo(value: $value) + } + `), + variableValues: { + value: "raw", + }, + schemaFragment: { + schemaId: "test", + definitions, + resolvers: { + CountedScalar, + Query: { + echo(_source: unknown, args: { value: string }) { + argsAtResolver.push(args); + return args.value; + }, + }, + }, + }, + }); + + expect(await drainExecution(result)).toEqual({ + data: { + first: "coerced:raw", + second: "coerced:raw", + }, + }); + expect(argsAtResolver).toEqual([ + { value: "coerced:raw" }, + { value: "coerced:raw" }, + ]); + expect(parseValueCalls).toBe(1); + }); }); diff --git a/packages/supermassive/src/executeWithoutSchema.ts b/packages/supermassive/src/executeWithoutSchema.ts index 340c4523c..89f72582b 100644 --- a/packages/supermassive/src/executeWithoutSchema.ts +++ b/packages/supermassive/src/executeWithoutSchema.ts @@ -7,6 +7,7 @@ import { FragmentDefinitionNode, OperationDefinitionNode, OperationTypeDefinitionNode, + VariableDefinitionNode, } from "graphql"; import { collectFields, @@ -45,8 +46,8 @@ import type { } from "./types"; import { getArgumentValues, - getVariableValues, getDirectiveValues, + getVariableDefinitionMap, } from "./values"; import type { ExecutionHooks } from "./hooks/types"; import { arraysAreEqual } from "./utilities/array"; @@ -115,6 +116,9 @@ export interface ExecutionContext { buildContextValue?: (contextValue?: unknown) => unknown; operation: OperationDefinitionNode; variableValues: { [variable: string]: unknown }; + rawVariableValues: { [variable: string]: unknown }; + variableDefinitions: { [variable: string]: VariableDefinitionNode }; + variableCoercionResults: Map; fieldResolver: FunctionFieldResolver; typeResolver: TypeResolver; subscribeFieldResolver: FunctionFieldResolver; @@ -124,6 +128,11 @@ export interface ExecutionContext { enablePerEventContext: boolean; } +export type VariableCoercionResult = + | { status: "coerced"; value: unknown } + | { status: "missing" } + | { status: "error"; errors: ReadonlyArray }; + /** * Implements the "Executing requests" section of the GraphQL specification. * @@ -232,17 +241,7 @@ function buildExecutionContext( // istanbul ignore next (See: 'https://github.com/graphql/graphql-js/issues/2203') const variableDefinitions = operation.variableDefinitions ?? []; - - const coercedVariableValues = getVariableValues( - schemaFragment, - variableDefinitions, - variableValues ?? {}, - { maxErrors: 50 }, - ); - - if (coercedVariableValues.errors) { - return coercedVariableValues.errors; - } + const variableDefinitionMap = getVariableDefinitionMap(variableDefinitions); return { schemaFragment, @@ -254,7 +253,10 @@ function buildExecutionContext( : contextValue, buildContextValue, operation, - variableValues: coercedVariableValues.coerced, + variableValues: {}, + rawVariableValues: variableValues ?? {}, + variableDefinitions: variableDefinitionMap, + variableCoercionResults: new Map(), fieldResolver: fieldResolver ?? defaultFieldResolver, typeResolver: typeResolver ?? defaultTypeResolver, subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver, diff --git a/packages/supermassive/src/values.ts b/packages/supermassive/src/values.ts index 5a14eb3bc..43ffcf4c6 100644 --- a/packages/supermassive/src/values.ts +++ b/packages/supermassive/src/values.ts @@ -6,10 +6,14 @@ import { DirectiveNode, FieldNode, VariableDefinitionNode, + ValueNode, } from "graphql"; import { inspect } from "./jsutils/inspect"; import { printPathArray } from "./jsutils/printPathArray"; -import { ExecutionContext } from "./executeWithoutSchema"; +import type { + ExecutionContext, + VariableCoercionResult, +} from "./executeWithoutSchema"; import { DirectiveDefinitionTuple, FieldDefinition, @@ -34,6 +38,14 @@ type CoercedVariableValues = | { errors: Array; coerced?: never } | { coerced: { [variable: string]: unknown }; errors?: never }; +type VariableCoercionContext = { + schemaFragment: SchemaFragment; + variableValues: { [variable: string]: unknown }; + rawVariableValues: { [variable: string]: unknown }; + variableDefinitions: { [variable: string]: VariableDefinitionNode }; + variableCoercionResults: Map; +}; + /** * Prepares an object map of variableValues of the correct type based on the * provided variable definitions and arbitrary input. If the input cannot be @@ -53,24 +65,37 @@ export function getVariableValues( ): CoercedVariableValues { const errors: GraphQLError[] = []; const maxErrors = options?.maxErrors; + const variableContext = { + schemaFragment, + variableValues: {}, + rawVariableValues: inputs, + variableDefinitions: getVariableDefinitionMap(varDefNodes), + variableCoercionResults: new Map(), + }; + const onError = (error: GraphQLError) => { + if (maxErrors != null && errors.length >= maxErrors) { + throw locatedError( + "Too many errors processing variables, error limit reached. Execution aborted.", + [], + ); + } + errors.push(error); + }; + try { - const coerced = coerceVariableValues( - schemaFragment, - varDefNodes, - inputs, - (error) => { - if (maxErrors != null && errors.length >= maxErrors) { - throw locatedError( - "Too many errors processing variables, error limit reached. Execution aborted.", - [], - ); + for (const varDefNode of varDefNodes) { + const result = getVariableValue( + variableContext, + varDefNode.variable.name.value, + ); + if (result.status === "error") { + for (const error of result.errors) { + onError(error); } - errors.push(error); - }, - ); - + } + } if (errors.length === 0) { - return { coerced }; + return { coerced: variableContext.variableValues }; } } catch (error) { errors.push(error as GraphQLError); @@ -79,77 +104,152 @@ export function getVariableValues( return { errors: errors }; } -function coerceVariableValues( - schemaFragment: SchemaFragment, +export function getVariableDefinitionMap( varDefNodes: ReadonlyArray, - inputs: { [variable: string]: unknown }, - onError: (error: GraphQLError) => void, -): { [variable: string]: unknown } { - const coercedValues: { [variable: string]: unknown } = {}; +): { [variable: string]: VariableDefinitionNode } { + const variableDefinitionMap: { [variable: string]: VariableDefinitionNode } = + Object.create(null); for (const varDefNode of varDefNodes) { - const varName = varDefNode.variable.name.value; - const varTypeReference = typeReferenceFromNode(varDefNode.type); + variableDefinitionMap[varDefNode.variable.name.value] = varDefNode; + } + return variableDefinitionMap; +} - if (!isInputType(schemaFragment.definitions, varTypeReference)) { - // Must use input types for variables. This should be caught during - // validation, however is checked again here for safety. - const varTypeStr = inspectTypeReference(varTypeReference); - onError( - locatedError( - `Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`, - [varDefNode.type], - ), - ); - continue; - } +function getVariableValue( + exeContext: VariableCoercionContext, + variableName: string, +): VariableCoercionResult { + const existingResult = exeContext.variableCoercionResults.get(variableName); + if (existingResult) { + return existingResult; + } - if (!hasOwnProperty(inputs, varName)) { - if (varDefNode.defaultValue) { - coercedValues[varName] = valueFromAST( - varDefNode.defaultValue, - varTypeReference, - schemaFragment, - ); - } else if (isNonNullType(varTypeReference)) { - const varTypeStr = inspectTypeReference(varTypeReference); - onError( - locatedError( - `Variable "$${varName}" of required type "${varTypeStr}" was not provided.`, - [varDefNode], - ), - ); - } - continue; - } + const varDefNode = exeContext.variableDefinitions[variableName]; + if (!varDefNode) { + const result: VariableCoercionResult = { status: "missing" }; + exeContext.variableCoercionResults.set(variableName, result); + return result; + } + + const result = coerceVariableValue( + exeContext.schemaFragment, + varDefNode, + exeContext.rawVariableValues, + ); + exeContext.variableCoercionResults.set(variableName, result); + + if (result.status === "coerced") { + exeContext.variableValues[variableName] = result.value; + } + + return result; +} - const value = inputs[varName]; - if (value === null && isNonNullType(varTypeReference)) { +function coerceVariableValue( + schemaFragment: SchemaFragment, + varDefNode: VariableDefinitionNode, + inputs: { [variable: string]: unknown }, +): VariableCoercionResult { + const errors: GraphQLError[] = []; + const onError = (error: GraphQLError) => errors.push(error); + const varName = varDefNode.variable.name.value; + const varTypeReference = typeReferenceFromNode(varDefNode.type); + + if (!isInputType(schemaFragment.definitions, varTypeReference)) { + const varTypeStr = inspectTypeReference(varTypeReference); + onError( + locatedError( + `Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`, + [varDefNode.type], + ), + ); + return { status: "error", errors }; + } + + if (!hasOwnProperty(inputs, varName)) { + if (varDefNode.defaultValue) { + const value = valueFromAST( + varDefNode.defaultValue, + varTypeReference, + schemaFragment, + ); + return { status: "coerced", value }; + } + if (isNonNullType(varTypeReference)) { const varTypeStr = inspectTypeReference(varTypeReference); onError( locatedError( - `Variable "$${varName}" of non-null type "${varTypeStr}" must not be null.`, + `Variable "$${varName}" of required type "${varTypeStr}" was not provided.`, [varDefNode], ), ); - continue; + return { status: "error", errors }; } + return { status: "missing" }; + } - coercedValues[varName] = coerceInputValue( - value, - varTypeReference, - schemaFragment, - (path, invalidValue, error) => { - let prefix = - `Variable "$${varName}" got invalid value ` + inspect(invalidValue); - if (path.length > 0) { - prefix += ` at "${varName}${printPathArray(path)}"`; - } - onError(locatedError(prefix + "; " + error.message, [varDefNode])); - }, + const value = inputs[varName]; + if (value === null && isNonNullType(varTypeReference)) { + const varTypeStr = inspectTypeReference(varTypeReference); + onError( + locatedError( + `Variable "$${varName}" of non-null type "${varTypeStr}" must not be null.`, + [varDefNode], + ), ); + return { status: "error", errors }; } - return coercedValues; + const coerced = coerceInputValue( + value, + varTypeReference, + schemaFragment, + (path, invalidValue, error) => { + let prefix = + `Variable "$${varName}" got invalid value ` + inspect(invalidValue); + if (path.length > 0) { + prefix += ` at "${varName}${printPathArray(path)}"`; + } + onError(locatedError(prefix + "; " + error.message, [varDefNode])); + }, + ); + + return errors.length > 0 + ? { status: "error", errors } + : { status: "coerced", value: coerced }; +} + +function ensureVariableValue( + exeContext: ExecutionContext, + variableName: string, +): void { + const result = getVariableValue(exeContext, variableName); + if (result.status === "error") { + throw result.errors[0]; + } +} + +function ensureVariablesInValue( + exeContext: ExecutionContext, + valueNode: ValueNode, +): void { + if (valueNode.kind === Kind.VARIABLE) { + ensureVariableValue(exeContext, valueNode.name.value); + return; + } + + if (valueNode.kind === Kind.LIST) { + for (const itemNode of valueNode.values) { + ensureVariablesInValue(exeContext, itemNode); + } + return; + } + + if (valueNode.kind === Kind.OBJECT) { + for (const fieldNode of valueNode.fields) { + ensureVariablesInValue(exeContext, fieldNode.value); + } + } } function isFieldDefinition( @@ -224,6 +324,7 @@ export function getArgumentValues( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; + ensureVariableValue(exeContext, variableName); if ( exeContext.variableValues == null || !hasOwnProperty(exeContext.variableValues, variableName) @@ -241,6 +342,8 @@ export function getArgumentValues( continue; } isNull = exeContext.variableValues[variableName] == null; + } else { + ensureVariablesInValue(exeContext, valueNode); } if (isNull && isNonNullType(argumentTypeRef)) {