From 4d5cdde952f2cfee711a1925c6f8eec3791ad84b Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 12:20:35 +0100 Subject: [PATCH 01/21] Hook up onError/extensions to inputs --- grafast/grafast/src/grafastGraphql.ts | 5 +++ grafast/grafast/src/index.ts | 6 ++++ grafast/grafast/src/interfaces.ts | 15 +++++++++ grafast/grafast/src/prepare.ts | 10 ++++++ grafast/grafast/src/utils.ts | 7 ++++ grafast/grafserv/src/interfaces.ts | 15 ++++++++- grafast/grafserv/src/middleware/graphql.ts | 32 ++++++++++++++++--- grafast/grafserv/src/utils.ts | 13 ++++++-- .../website/postgraphile/usage-schema.md | 12 +++++-- 9 files changed, 105 insertions(+), 10 deletions(-) diff --git a/grafast/grafast/src/grafastGraphql.ts b/grafast/grafast/src/grafastGraphql.ts index 34552e72d5..064d64b9ac 100644 --- a/grafast/grafast/src/grafastGraphql.ts +++ b/grafast/grafast/src/grafastGraphql.ts @@ -13,6 +13,7 @@ import { SafeError } from "./error.js"; import { execute } from "./execute.js"; import { hookArgs } from "./index.js"; import type { + ErrorBehavior, GrafastArgs, GrafastExecutionArgs, ParseAndValidateEvent, @@ -160,6 +161,8 @@ export function grafast( contextValue, variableValues, operationName, + onError, + extensions, fieldResolver, typeResolver, resolvedPreset, @@ -207,6 +210,8 @@ export function grafast( contextValue, variableValues, operationName, + onError, + extensions, fieldResolver, typeResolver, middleware, diff --git a/grafast/grafast/src/index.ts b/grafast/grafast/src/index.ts index 09fce5087c..9a0a62ca81 100644 --- a/grafast/grafast/src/index.ts +++ b/grafast/grafast/src/index.ts @@ -77,6 +77,7 @@ import type { CacheByOperationEntry, DataFromStep, EnumValueApplyResolver, + ErrorBehavior, EstablishOperationPlanEvent, EventCallback, EventMapKey, @@ -267,6 +268,7 @@ import { GrafastInputFieldConfigMap, GrafastInputObjectType, GrafastObjectType, + GraphQLSpecifiedErrorBehaviors, inputObjectFieldSpec, InputObjectTypeSpec, isPromiseLike, @@ -359,6 +361,7 @@ export { EnumValueConfig, EnumValueInput, error, + ErrorBehavior, ErrorStep, EventCallback, EventMapKey, @@ -410,6 +413,7 @@ export { GrafastValuesList, graphqlResolver, GraphQLResolverStep, + GraphQLSpecifiedErrorBehaviors, groupBy, GroupByPlanMemo, inhibitOnNull, @@ -704,6 +708,8 @@ declare global { | "rootValue" | "variableValues" | "operationName" + | "onError" + | "extensions" | "resolvedPreset" | "middleware" | "requestContext" diff --git a/grafast/grafast/src/interfaces.ts b/grafast/grafast/src/interfaces.ts index 20a293dd32..e99c668bfd 100644 --- a/grafast/grafast/src/interfaces.ts +++ b/grafast/grafast/src/interfaces.ts @@ -735,6 +735,11 @@ export type StreamMoreableArray = Array & { }; export interface GrafastArgs extends GraphQLArgs { + // This should ultimately come from graphql + onError?: ErrorBehavior; + + // These are ours + extensions?: Record; resolvedPreset?: GraphileConfig.ResolvedPreset; requestContext?: Partial; middleware?: Middleware | null; @@ -774,6 +779,11 @@ export type DataFromStep = TStep extends Step ? TData : never; export interface GrafastExecutionArgs extends ExecutionArgs { + // This should ultimately come from graphql + onError?: ErrorBehavior; + + // These are ours + extensions?: Record; resolvedPreset?: GraphileConfig.ResolvedPreset; middleware?: Middleware | null; requestContext?: Partial; @@ -847,3 +857,8 @@ export interface AbstractTypePlanner { } export type Thunk = T | (() => T); + +/** + * GraphQL error behavior, as per https://github.com/graphql/graphql-spec/pull/1163 + */ +export type ErrorBehavior = "PROPAGATE" | "NULL" | "HALT"; diff --git a/grafast/grafast/src/prepare.ts b/grafast/grafast/src/prepare.ts index 8bdb9424a6..4fa4b55196 100644 --- a/grafast/grafast/src/prepare.ts +++ b/grafast/grafast/src/prepare.ts @@ -571,6 +571,13 @@ function establishOperationPlanFromEvent(event: EstablishOperationPlanEvent) { ); } +// TODO: upstream this +declare module "graphql/execution/execute.js" { + interface ExecutionContext { + onError: "PROPAGATE" | "NULL" | "HALT"; + } +} + /** * @internal */ @@ -597,6 +604,9 @@ export function grafastPrepare( extensions: rootValue[$$extensions], }); } + if (!exeContext.onError) { + exeContext.onError = args.onError ?? "PROPAGATE"; + } const { operation, fragments, variableValues } = exeContext; let operationPlan!: OperationPlan; diff --git a/grafast/grafast/src/utils.ts b/grafast/grafast/src/utils.ts index a472a6b829..d9135b61ae 100644 --- a/grafast/grafast/src/utils.ts +++ b/grafast/grafast/src/utils.ts @@ -33,6 +33,7 @@ import { SafeError } from "./error.js"; import { inspect } from "./inspect.js"; import type { BaseGraphQLArguments, + ErrorBehavior, ExecutionEntryFlags, GrafastFieldConfig, GrafastInputFieldConfig, @@ -1512,3 +1513,9 @@ export function terminateIterable( iterable.return(); } } + +export const GraphQLSpecifiedErrorBehaviors = Object.freeze([ + "PROPAGATE", + "NULL", + "HALT", +]) as readonly ErrorBehavior[]; diff --git a/grafast/grafserv/src/interfaces.ts b/grafast/grafserv/src/interfaces.ts index 8237bd522f..bf77e388c7 100644 --- a/grafast/grafserv/src/interfaces.ts +++ b/grafast/grafserv/src/interfaces.ts @@ -1,6 +1,12 @@ import "graphile-config"; -import type { execute, PromiseOrDirect, SafeError, subscribe } from "grafast"; +import type { + ErrorBehavior, + execute, + PromiseOrDirect, + SafeError, + subscribe, +} from "grafast"; import type { AsyncExecutionResult, FormattedExecutionResult, @@ -41,6 +47,12 @@ export interface ParsedGraphQLBody { query: unknown; operationName: unknown; variableValues: unknown; + /** + * For customizing the error behavior as per https://github.com/graphql/graphql-spec/pull/1163 + * + * @experimental + */ + onError: unknown; extensions: unknown; } @@ -52,6 +64,7 @@ export interface ValidatedGraphQLBody { query: string; operationName: string | undefined; variableValues: Record | undefined; + onError: ErrorBehavior | undefined; extensions: Record | undefined; } diff --git a/grafast/grafserv/src/middleware/graphql.ts b/grafast/grafserv/src/middleware/graphql.ts index bf149d4098..9207361aaf 100644 --- a/grafast/grafserv/src/middleware/graphql.ts +++ b/grafast/grafserv/src/middleware/graphql.ts @@ -2,8 +2,18 @@ import { parse as parseGraphQLQueryString } from "node:querystring"; import { LRU } from "@graphile/lru"; import { createHash } from "crypto"; -import type { GrafastExecutionArgs, PromiseOrDirect } from "grafast"; -import { $$extensions, hookArgs, isAsyncIterable, SafeError } from "grafast"; +import type { + ErrorBehavior, + GrafastExecutionArgs, + PromiseOrDirect, +} from "grafast"; +import { + $$extensions, + GraphQLSpecifiedErrorBehaviors, + hookArgs, + isAsyncIterable, + SafeError, +} from "grafast"; import type { DocumentNode, GraphQLSchema } from "grafast/graphql"; import * as graphql from "grafast/graphql"; @@ -98,6 +108,7 @@ function parseGraphQLQueryParams( typeof variablesString === "string" ? JSON.parse(variablesString) : undefined; + const onError = params.onError ?? undefined; const extensionsString = params.extensions ?? undefined; const extensions = typeof extensionsString === "string" @@ -109,6 +120,7 @@ function parseGraphQLQueryParams( query, operationName, variableValues, + onError, extensions, }; } @@ -207,6 +219,7 @@ function parseGraphQLBody( query: body.text, operationName: undefined, variableValues: undefined, + onError: undefined, extensions: undefined, }; } @@ -217,6 +230,7 @@ function parseGraphQLBody( query: body.buffer.toString("utf8"), operationName: undefined, variableValues: undefined, + onError: undefined, extensions: undefined, }; } @@ -260,7 +274,7 @@ const graphqlOrHTMLAcceptMatcher = makeAcceptMatcher([ export function validateGraphQLBody( parsed: ParsedGraphQLBody, ): ValidatedGraphQLBody { - const { query, operationName, variableValues, extensions } = parsed; + const { query, operationName, variableValues, onError, extensions } = parsed; if (typeof query !== "string") { throw httpError(400, "query must be a string"); @@ -274,6 +288,15 @@ export function validateGraphQLBody( ) { throw httpError(400, "Invalid variables; expected JSON-encoded object"); } + if ( + onError != null && + !GraphQLSpecifiedErrorBehaviors.includes(onError as ErrorBehavior) + ) { + throw httpError( + 400, + `Invalid onError; supported error behaviors are: ${GraphQLSpecifiedErrorBehaviors.join(", ")}`, + ); + } if ( extensions != null && (typeof extensions !== "object" || Array.isArray(extensions)) @@ -414,7 +437,7 @@ const _makeGraphQLHandlerInternal = (instance: GrafservBase) => { const outputDataAsString = dynamicOptions.outputDataAsString; const { maskIterator, maskPayload, maskError } = dynamicOptions; - const { query, operationName, variableValues } = body; + const { query, operationName, variableValues, onError } = body; const { errors, document } = parseAndValidate(query); if (errors !== undefined) { @@ -458,6 +481,7 @@ const _makeGraphQLHandlerInternal = (instance: GrafservBase) => { contextValue, variableValues, operationName, + onError, resolvedPreset, requestContext: grafastCtx, middleware: grafastMiddleware, diff --git a/grafast/grafserv/src/utils.ts b/grafast/grafserv/src/utils.ts index 472a2c056b..9c9d936c63 100644 --- a/grafast/grafserv/src/utils.ts +++ b/grafast/grafserv/src/utils.ts @@ -1,6 +1,11 @@ import type { Readable } from "node:stream"; -import type { execute, GrafastExecutionArgs, subscribe } from "grafast"; +import type { + ErrorBehavior, + execute, + GrafastExecutionArgs, + subscribe, +} from "grafast"; import { hookArgs, SafeError, stripAnsi } from "grafast"; import type { ExecutionArgs, @@ -281,7 +286,7 @@ export function makeGraphQLWSConfig(instance: GrafservBase): ServerOptions { ); } - const { query, operationName, variableValues } = + const { query, operationName, variableValues, onError, extensions } = validateGraphQLBody(parsedBody); const { errors, document } = parseAndValidate(query); if (errors !== undefined) { @@ -296,6 +301,8 @@ export function makeGraphQLWSConfig(instance: GrafservBase): ServerOptions { contextValue, variableValues, operationName, + onError, + extensions, resolvedPreset, requestContext: grafastCtx, middleware: grafastMiddleware, @@ -361,6 +368,7 @@ export function parseGraphQLJSONBody( const query = params.query; const operationName = params.operationName ?? undefined; const variableValues = params.variables ?? undefined; + const onError = (params as any).onError ?? undefined; const extensions = params.extensions ?? undefined; return { id, @@ -368,6 +376,7 @@ export function parseGraphQLJSONBody( query, operationName, variableValues, + onError, extensions, }; } diff --git a/postgraphile/website/postgraphile/usage-schema.md b/postgraphile/website/postgraphile/usage-schema.md index f8fb73c8f4..25ab1d5e4a 100644 --- a/postgraphile/website/postgraphile/usage-schema.md +++ b/postgraphile/website/postgraphile/usage-schema.md @@ -103,7 +103,7 @@ Here's a full example: ```ts import { postgraphile } from "postgraphile"; -import { grafast } from "postgraphile/grafast"; +import { grafast, type ErrorBehavior } from "postgraphile/grafast"; import preset from "./graphile.config.js"; // Make a new PostGraphile instance: @@ -121,6 +121,7 @@ export async function executeQuery( source: string, variableValues?: Record | null, operationName?: string, + onError?: ErrorBehavior, ) { const { schema, resolvedPreset } = await pgl.getSchemaResult(); return await grafast({ @@ -128,6 +129,7 @@ export async function executeQuery( source, variableValues, operationName, + onError, resolvedPreset, requestContext, }); @@ -158,7 +160,7 @@ Here's an example using hookArgs: ```ts import { postgraphile } from "postgraphile"; import { parse, validate } from "postgraphile/graphql"; -import { execute, hookArgs } from "postgraphile/grafast"; +import { execute, hookArgs, type ErrorBehavior } from "postgraphile/grafast"; import preset from "./graphile.config.js"; // Build a `pgl` instance, with helpers and schema based on our preset. @@ -174,6 +176,7 @@ export async function executeQuery( source: string, variableValues?: Record | null, operationName?: string, + onError?: ErrorBehavior, ) { // We might get a newer schema in "watch" mode const { schema, resolvedPreset } = await pgl.getSchemaResult(); @@ -193,6 +196,7 @@ export async function executeQuery( document, variableValues, operationName, + onError, resolvedPreset, requestContext, }); @@ -218,7 +222,7 @@ import type { ResultOf, } from "@graphql-typed-document-node/core"; import { postgraphile } from "postgraphile"; -import { execute, hookArgs } from "postgraphile/grafast"; +import { execute, hookArgs, type ErrorBehavior } from "postgraphile/grafast"; import { validate } from "postgraphile/graphql"; import preset from "./graphile.config.js"; @@ -232,6 +236,7 @@ export async function executeDocument< document: TDoc, variableValues?: VariablesOf, operationName?: string, + onError?: ErrorBehavior, ): Promise, TExtensions>> { const { schema, resolvedPreset } = await pgl.getSchemaResult(); @@ -247,6 +252,7 @@ export async function executeDocument< document, variableValues, operationName, + onError, resolvedPreset, requestContext, } as any); From c699ae513a312840a93db6cfacbec38d2b03cecd Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 12:41:52 +0100 Subject: [PATCH 02/21] onError ties into planning, and start reflecting in OutputPlan --- grafast/grafast/src/bucket.ts | 6 +++++- grafast/grafast/src/engine/OperationPlan.ts | 7 ++++++- grafast/grafast/src/engine/OutputPlan.ts | 18 +++++++++++++++++- .../grafast/src/engine/executeOutputPlan.ts | 4 +++- grafast/grafast/src/establishOperationPlan.ts | 11 +++++++++-- grafast/grafast/src/interfaces.ts | 2 ++ grafast/grafast/src/prepare.ts | 10 +++++++++- 7 files changed, 51 insertions(+), 7 deletions(-) diff --git a/grafast/grafast/src/bucket.ts b/grafast/grafast/src/bucket.ts index 05ce30238f..15aae9cb8d 100644 --- a/grafast/grafast/src/bucket.ts +++ b/grafast/grafast/src/bucket.ts @@ -1,6 +1,8 @@ // import type { GraphQLScalarType } from "graphql"; -import type { GrafastExecutionArgs, Step } from "."; +import { ExecutionContext } from "graphql/execution/execute.js"; + +import type { ErrorBehavior, GrafastExecutionArgs, Step } from "."; import type { LayerPlan } from "./engine/LayerPlan"; import type { MetaByMetaKey } from "./engine/OperationPlan"; import type { @@ -15,6 +17,8 @@ import type { export interface RequestTools { /** @internal */ args: GrafastExecutionArgs; + /** @internal */ + onError: ErrorBehavior; /** The `timeSource.now()` at which the request started executing */ startTime: number; /** The `timeSource.now()` at which the request should stop executing (if a timeout was configured) */ diff --git a/grafast/grafast/src/engine/OperationPlan.ts b/grafast/grafast/src/engine/OperationPlan.ts index 400d58be9f..64d026aaf5 100644 --- a/grafast/grafast/src/engine/OperationPlan.ts +++ b/grafast/grafast/src/engine/OperationPlan.ts @@ -31,7 +31,11 @@ import { newSelectionSetDigest, } from "../graphqlCollectFields.js"; import { fieldSelectionsForType } from "../graphqlMergeSelectionSets.js"; -import type { GrafastPlanJSON, StepStreamOptions } from "../index.js"; +import type { + ErrorBehavior, + GrafastPlanJSON, + StepStreamOptions, +} from "../index.js"; import { __FlagStep, __ItemStep, @@ -371,6 +375,7 @@ export class OperationPlan { public readonly context: { [key: string]: any }, rootValueConstraints: Constraint[], public readonly rootValue: any, + public readonly errorBehavior: ErrorBehavior, options: GrafastOperationOptions, ) { this.planningTimeout = options?.timeouts?.planning ?? null; diff --git a/grafast/grafast/src/engine/OutputPlan.ts b/grafast/grafast/src/engine/OutputPlan.ts index f8ec3ca644..32250e5b0a 100644 --- a/grafast/grafast/src/engine/OutputPlan.ts +++ b/grafast/grafast/src/engine/OutputPlan.ts @@ -1039,7 +1039,23 @@ function executeChildPlan( childOutputPlan.rootStep === that.rootStep ? bucketRootFlags : undefined, ); if (fieldResult == (asString ? "null" : null)) { - throw nonNullError(locationDetails, mutablePath.slice(1)); + const error = nonNullError(locationDetails, mutablePath.slice(1)); + if (root.errorBehavior === "NULL") { + // Just set to null + const streamCount = root.streams.length; + const queueCount = root.queue.length; + commonErrorHandler( + error, + locationDetails, + mutablePath, + mutablePathIndex, + root, + streamCount, + queueCount, + ); + } else { + throw error; + } } return fieldResult; } else { diff --git a/grafast/grafast/src/engine/executeOutputPlan.ts b/grafast/grafast/src/engine/executeOutputPlan.ts index 5125907888..b64521ab56 100644 --- a/grafast/grafast/src/engine/executeOutputPlan.ts +++ b/grafast/grafast/src/engine/executeOutputPlan.ts @@ -4,7 +4,7 @@ import type { GraphQLError } from "graphql"; import * as assert from "../assert.js"; import type { Bucket, RequestTools } from "../bucket.js"; import { isDev } from "../dev.js"; -import type { JSONValue } from "../interfaces.js"; +import type { ErrorBehavior, JSONValue } from "../interfaces.js"; import type { OutputPlan } from "./OutputPlan.js"; const debug = debugFactory("grafast:OutputPlan"); @@ -30,6 +30,8 @@ export interface OutputStream { * @internal */ export interface PayloadRoot { + errorBehavior: ErrorBehavior; + /** * Serialization works differently if we're running inside GraphQL. (Namely: * we don't serialize - that's GraphQL's job.) diff --git a/grafast/grafast/src/establishOperationPlan.ts b/grafast/grafast/src/establishOperationPlan.ts index 550e0c0d81..b34e83cd44 100644 --- a/grafast/grafast/src/establishOperationPlan.ts +++ b/grafast/grafast/src/establishOperationPlan.ts @@ -10,6 +10,7 @@ import { OperationPlan, SafeError } from "./index.js"; import type { BaseGraphQLRootValue, BaseGraphQLVariables, + ErrorBehavior, EstablishOperationPlanResult, Fragments, LinkedList, @@ -75,16 +76,18 @@ function isOperationPlanResultCompatible< TContext extends Grafast.Context = Grafast.Context, TRootValue extends BaseGraphQLRootValue = BaseGraphQLRootValue, >( - operationPlan: EstablishOperationPlanResult, + operationPlanResult: EstablishOperationPlanResult, variableValues: TVariables, context: TContext, rootValue: TRootValue, + errorBehavior: ErrorBehavior, ): boolean { + if (operationPlanResult.errorBehavior !== errorBehavior) return false; const { variableValuesConstraints, contextConstraints, rootValueConstraints, - } = operationPlan; + } = operationPlanResult; if (!matchesConstraints(variableValuesConstraints, variableValues)) { return false; } @@ -115,6 +118,7 @@ export function establishOperationPlan< variableValues: TVariables, context: TContext, rootValue: TRootValue, + onError: ErrorBehavior, options: GrafastOperationOptions, ): OperationPlan { const planningTimeout = options.timeouts?.planning; @@ -141,6 +145,7 @@ export function establishOperationPlan< variableValues, context, rootValue, + onError, ) ) { const { error, operationPlan } = value; @@ -214,6 +219,7 @@ export function establishOperationPlan< context, rootValueConstraints, rootValue, + onError, options, ); } catch (e) { @@ -234,6 +240,7 @@ export function establishOperationPlan< variableValuesConstraints, contextConstraints, rootValueConstraints, + errorBehavior: onError, ...(operationPlan ? { operationPlan } : { error: error! }), }; if (!cache) { diff --git a/grafast/grafast/src/interfaces.ts b/grafast/grafast/src/interfaces.ts index e99c668bfd..6aea9223ec 100644 --- a/grafast/grafast/src/interfaces.ts +++ b/grafast/grafast/src/interfaces.ts @@ -88,6 +88,7 @@ export interface IEstablishOperationPlanResult { variableValuesConstraints: Constraint[]; contextConstraints: Constraint[]; rootValueConstraints: Constraint[]; + errorBehavior: ErrorBehavior; } export interface EstablishOperationPlanResultSuccess extends IEstablishOperationPlanResult { @@ -815,6 +816,7 @@ export interface EstablishOperationPlanEvent { variableValues: Record; context: any; rootValue: any; + onError: ErrorBehavior; args: GrafastExecutionArgs; options: GrafastOperationOptions; } diff --git a/grafast/grafast/src/prepare.ts b/grafast/grafast/src/prepare.ts index 4fa4b55196..93c59f68b7 100644 --- a/grafast/grafast/src/prepare.ts +++ b/grafast/grafast/src/prepare.ts @@ -42,6 +42,7 @@ import { } from "./engine/OutputPlan.js"; import { establishOperationPlan } from "./establishOperationPlan.js"; import type { + ErrorBehavior, EstablishOperationPlanEvent, GrafastExecutionArgs, GrafastTimeouts, @@ -265,6 +266,7 @@ function outputBucket( >*/ { const operationPlan = rootBucket.layerPlan.operationPlan; const root: PayloadRoot = { + errorBehavior: requestContext.args.onError ?? "PROPAGATE", insideGraphQL: false, errors: [], queue: [], @@ -324,6 +326,7 @@ function executePreemptive( variableValues: any, context: any, rootValue: any, + onError: ErrorBehavior, outputDataAsString: boolean, executionTimeout: number | null, abortSignal: AbortSignal, @@ -358,6 +361,7 @@ function executePreemptive( executionTimeout !== null ? startTime + executionTimeout : null; const requestContext: RequestTools = { args, + onError, startTime, stopTime, // toSerialize: [], @@ -567,6 +571,7 @@ function establishOperationPlanFromEvent(event: EstablishOperationPlanEvent) { event.variableValues, event.context as any, event.rootValue, + event.onError, event.options, ); } @@ -608,7 +613,7 @@ export function grafastPrepare( exeContext.onError = args.onError ?? "PROPAGATE"; } - const { operation, fragments, variableValues } = exeContext; + const { operation, fragments, variableValues, onError } = exeContext; let operationPlan!: OperationPlan; try { if (middleware != null) { @@ -621,6 +626,7 @@ export function grafastPrepare( variableValues, context: context as any, rootValue, + onError, args, options, }, @@ -634,6 +640,7 @@ export function grafastPrepare( variableValues, context as any, rootValue, + onError, options, ); } @@ -677,6 +684,7 @@ export function grafastPrepare( variableValues, context, rootValue, + onError, options.outputDataAsString ?? false, executionTimeout, abortController.signal, From 603422023bb61ee2c29ace8e3e8de8ac8ad7c11c Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 13:20:26 +0100 Subject: [PATCH 03/21] Handle HALT error behavior --- grafast/grafast/src/engine/OutputPlan.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/grafast/grafast/src/engine/OutputPlan.ts b/grafast/grafast/src/engine/OutputPlan.ts index 32250e5b0a..29cd44d514 100644 --- a/grafast/grafast/src/engine/OutputPlan.ts +++ b/grafast/grafast/src/engine/OutputPlan.ts @@ -1018,13 +1018,29 @@ function executeChildPlan( } } // This is the code that changes based on if the field is nullable or not - if (isNonNull) { + if (isNonNull || root.errorBehavior === "HALT") { // No need to catch error if (childBucket === bucket) { //noop } else { if (childBucket == null) { - throw nonNullError(locationDetails, mutablePath.slice(1)); + const error = nonNullError(locationDetails, mutablePath.slice(1)); + if (root.errorBehavior === "NULL") { + const streamCount = root.streams.length; + const queueCount = root.queue.length; + commonErrorHandler( + error, + locationDetails, + mutablePath, + mutablePathIndex, + root, + streamCount, + queueCount, + ); + return asString ? "null" : null; + } else { + throw error; + } } } const fieldResult = childOutputPlan[asString ? "executeString" : "execute"]( @@ -1038,7 +1054,7 @@ function executeChildPlan( : undefined, childOutputPlan.rootStep === that.rootStep ? bucketRootFlags : undefined, ); - if (fieldResult == (asString ? "null" : null)) { + if (isNonNull && fieldResult == (asString ? "null" : null)) { const error = nonNullError(locationDetails, mutablePath.slice(1)); if (root.errorBehavior === "NULL") { // Just set to null From dfb45392fee38138510d4c7dd45cbcc04f90f667 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 13:42:38 +0100 Subject: [PATCH 04/21] Overhaul error handling --- grafast/grafast/src/engine/OutputPlan.ts | 142 ++++++++--------------- 1 file changed, 47 insertions(+), 95 deletions(-) diff --git a/grafast/grafast/src/engine/OutputPlan.ts b/grafast/grafast/src/engine/OutputPlan.ts index 29cd44d514..9a478f9524 100644 --- a/grafast/grafast/src/engine/OutputPlan.ts +++ b/grafast/grafast/src/engine/OutputPlan.ts @@ -998,116 +998,63 @@ function executeChildPlan( locationDetails, mutablePath.slice(1), ); - if (isNonNull) { - throw e; - } else { - const streamCount = root.streams.length; - const queueCount = root.queue.length; - commonErrorHandler( - e, - locationDetails, - mutablePath, - mutablePathIndex, - root, - streamCount, - queueCount, - ); - return asString ? "null" : null; - } - } - } - } - // This is the code that changes based on if the field is nullable or not - if (isNonNull || root.errorBehavior === "HALT") { - // No need to catch error - if (childBucket === bucket) { - //noop - } else { - if (childBucket == null) { - const error = nonNullError(locationDetails, mutablePath.slice(1)); - if (root.errorBehavior === "NULL") { - const streamCount = root.streams.length; - const queueCount = root.queue.length; - commonErrorHandler( - error, - locationDetails, - mutablePath, - mutablePathIndex, - root, - streamCount, - queueCount, - ); - return asString ? "null" : null; - } else { - throw error; - } - } - } - const fieldResult = childOutputPlan[asString ? "executeString" : "execute"]( - root, - mutablePath, - childBucket, - childBucketIndex!, - // NOTE: the previous code may have had a bug here, it referenced childBucket.rootStep - childOutputPlan.rootStep === that.rootStep - ? rawBucketRootValue - : undefined, - childOutputPlan.rootStep === that.rootStep ? bucketRootFlags : undefined, - ); - if (isNonNull && fieldResult == (asString ? "null" : null)) { - const error = nonNullError(locationDetails, mutablePath.slice(1)); - if (root.errorBehavior === "NULL") { - // Just set to null const streamCount = root.streams.length; const queueCount = root.queue.length; - commonErrorHandler( - error, + return commonErrorHandler( + e, locationDetails, mutablePath, mutablePathIndex, root, streamCount, queueCount, + asString, + isNonNull, ); - } else { - throw error; } } - return fieldResult; - } else { - // Need to catch error and set null - const streamCount = root.streams.length; - const queueCount = root.queue.length; - try { - if (childBucket !== bucket && childBucket == null) { - return asString ? "null" : null; - } else { - return childOutputPlan[asString ? "executeString" : "execute"]( - root, - mutablePath, - childBucket, - childBucketIndex!, - // NOTE: the previous code may have had a bug here, it referenced childBucket.rootStep - childOutputPlan.rootStep === that.rootStep - ? rawBucketRootValue - : undefined, - childOutputPlan.rootStep === that.rootStep - ? bucketRootFlags - : undefined, - ); + } + const streamCount = root.streams.length; + const queueCount = root.queue.length; + try { + if (childBucket !== bucket && childBucket == null) { + if (isNonNull) { + throw nonNullError(locationDetails, mutablePath.slice(1)); } - } catch (e) { - commonErrorHandler( - e, - locationDetails, - mutablePath, - mutablePathIndex, + return asString ? "null" : null; + } else { + const fieldResult = childOutputPlan[ + asString ? "executeString" : "execute" + ]( root, - streamCount, - queueCount, + mutablePath, + childBucket, + childBucketIndex!, + // NOTE: the previous code may have had a bug here, it referenced childBucket.rootStep + childOutputPlan.rootStep === that.rootStep + ? rawBucketRootValue + : undefined, + childOutputPlan.rootStep === that.rootStep + ? bucketRootFlags + : undefined, ); - return asString ? "null" : null; + if (isNonNull && fieldResult == (asString ? "null" : null)) { + throw nonNullError(locationDetails, mutablePath.slice(1)); + } + return fieldResult; } + } catch (e) { + return commonErrorHandler( + e, + locationDetails, + mutablePath, + mutablePathIndex, + root, + streamCount, + queueCount, + asString, + isNonNull, + ); } } @@ -1119,6 +1066,8 @@ function commonErrorHandler( root: PayloadRoot, streamCount: number, queueCount: number, + asString: boolean, + isNonNull: boolean, ) { if (root.streams.length > streamCount) { root.streams.splice(streamCount); @@ -1127,12 +1076,15 @@ function commonErrorHandler( root.queue.splice(queueCount); } const error = coerceError(e, locationDetails, mutablePath.slice(1)); + if (root.errorBehavior === "HALT") throw error; + if (isNonNull && root.errorBehavior !== "NULL") throw error; const pathLengthTarget = mutablePathIndex + 1; const overSize = mutablePath.length - pathLengthTarget; if (overSize > 0) { (mutablePath as Array).splice(pathLengthTarget, overSize); } root.errors.push(error); + return asString ? "null" : null; } const nullExecutor = makeExecutor({ From ec7170f1d95e149ef007416f40176edefad2e175 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 13:44:36 +0100 Subject: [PATCH 05/21] Even more common handling --- grafast/grafast/src/engine/OutputPlan.ts | 132 +++++++++-------------- 1 file changed, 50 insertions(+), 82 deletions(-) diff --git a/grafast/grafast/src/engine/OutputPlan.ts b/grafast/grafast/src/engine/OutputPlan.ts index 9a478f9524..449784c1b9 100644 --- a/grafast/grafast/src/engine/OutputPlan.ts +++ b/grafast/grafast/src/engine/OutputPlan.ts @@ -972,51 +972,40 @@ function executeChildPlan( rawBucketRootValue: any, bucketRootFlags: ExecutionEntryFlags, ) { - const $sideEffect = childOutputPlan.layerPlan.parentSideEffectStep; - if ($sideEffect !== null) { - // Check if there's an error - const sideEffectBucketDetails = getChildBucketAndIndex( - $sideEffect.layerPlan, - null, - bucket, - bucketIndex, - ); - if (sideEffectBucketDetails) { - const [sideEffectBucket, sideEffectBucketIndex] = sideEffectBucketDetails; - const ev = sideEffectBucket.store.get($sideEffect.id); - if (!ev) { - throw new Error( - `GrafastInternalError: ${stripAnsi( - String($sideEffect), - )} has no entry in ${bucket}`, - ); - } - const flags = ev._flagsAt(sideEffectBucketIndex); - if (flags & FLAG_ERROR) { - const e = coerceError( - ev.at(sideEffectBucketIndex), - locationDetails, - mutablePath.slice(1), - ); - const streamCount = root.streams.length; - const queueCount = root.queue.length; - return commonErrorHandler( - e, - locationDetails, - mutablePath, - mutablePathIndex, - root, - streamCount, - queueCount, - asString, - isNonNull, - ); - } - } - } const streamCount = root.streams.length; const queueCount = root.queue.length; try { + const $sideEffect = childOutputPlan.layerPlan.parentSideEffectStep; + if ($sideEffect !== null) { + // Check if there's an error + const sideEffectBucketDetails = getChildBucketAndIndex( + $sideEffect.layerPlan, + null, + bucket, + bucketIndex, + ); + if (sideEffectBucketDetails) { + const [sideEffectBucket, sideEffectBucketIndex] = + sideEffectBucketDetails; + const ev = sideEffectBucket.store.get($sideEffect.id); + if (!ev) { + throw new Error( + `GrafastInternalError: ${stripAnsi( + String($sideEffect), + )} has no entry in ${bucket}`, + ); + } + const flags = ev._flagsAt(sideEffectBucketIndex); + if (flags & FLAG_ERROR) { + const e = coerceError( + ev.at(sideEffectBucketIndex), + locationDetails, + mutablePath.slice(1), + ); + throw e; + } + } + } if (childBucket !== bucket && childBucket == null) { if (isNonNull) { throw nonNullError(locationDetails, mutablePath.slice(1)); @@ -1044,47 +1033,26 @@ function executeChildPlan( return fieldResult; } } catch (e) { - return commonErrorHandler( - e, - locationDetails, - mutablePath, - mutablePathIndex, - root, - streamCount, - queueCount, - asString, - isNonNull, - ); - } -} - -function commonErrorHandler( - e: Error, - locationDetails: LocationDetails, - mutablePath: ReadonlyArray, - mutablePathIndex: number, - root: PayloadRoot, - streamCount: number, - queueCount: number, - asString: boolean, - isNonNull: boolean, -) { - if (root.streams.length > streamCount) { - root.streams.splice(streamCount); - } - if (root.queue.length > queueCount) { - root.queue.splice(queueCount); - } - const error = coerceError(e, locationDetails, mutablePath.slice(1)); - if (root.errorBehavior === "HALT") throw error; - if (isNonNull && root.errorBehavior !== "NULL") throw error; - const pathLengthTarget = mutablePathIndex + 1; - const overSize = mutablePath.length - pathLengthTarget; - if (overSize > 0) { - (mutablePath as Array).splice(pathLengthTarget, overSize); + if (root.streams.length > streamCount) { + root.streams.splice(streamCount); + } + if (root.queue.length > queueCount) { + root.queue.splice(queueCount); + } + const error = coerceError(e, locationDetails, mutablePath.slice(1)); + if (root.errorBehavior === "HALT") throw error; + if (isNonNull && root.errorBehavior !== "NULL") throw error; + const pathLengthTarget = mutablePathIndex + 1; + const overSize = mutablePath.length - pathLengthTarget; + if (overSize > 0) { + (mutablePath as Array).splice( + pathLengthTarget, + overSize, + ); + } + root.errors.push(error); + return asString ? "null" : null; } - root.errors.push(error); - return asString ? "null" : null; } const nullExecutor = makeExecutor({ From bea720f1b57cc8a8b566ec947fc8ad785f6bee8c Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 13:48:15 +0100 Subject: [PATCH 06/21] Do splicing later --- grafast/grafast/src/engine/OutputPlan.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/grafast/grafast/src/engine/OutputPlan.ts b/grafast/grafast/src/engine/OutputPlan.ts index 449784c1b9..df62dea65a 100644 --- a/grafast/grafast/src/engine/OutputPlan.ts +++ b/grafast/grafast/src/engine/OutputPlan.ts @@ -1033,15 +1033,15 @@ function executeChildPlan( return fieldResult; } } catch (e) { + const error = coerceError(e, locationDetails, mutablePath.slice(1)); + if (root.errorBehavior === "HALT") throw error; + if (isNonNull && root.errorBehavior !== "NULL") throw error; if (root.streams.length > streamCount) { root.streams.splice(streamCount); } if (root.queue.length > queueCount) { root.queue.splice(queueCount); } - const error = coerceError(e, locationDetails, mutablePath.slice(1)); - if (root.errorBehavior === "HALT") throw error; - if (isNonNull && root.errorBehavior !== "NULL") throw error; const pathLengthTarget = mutablePathIndex + 1; const overSize = mutablePath.length - pathLengthTarget; if (overSize > 0) { From 930b89fdc9a374924040083926e4d2ed9959a85c Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 14:03:22 +0100 Subject: [PATCH 07/21] Tidy --- grafast/grafast/src/engine/OutputPlan.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/grafast/grafast/src/engine/OutputPlan.ts b/grafast/grafast/src/engine/OutputPlan.ts index df62dea65a..9cea0fc751 100644 --- a/grafast/grafast/src/engine/OutputPlan.ts +++ b/grafast/grafast/src/engine/OutputPlan.ts @@ -997,12 +997,11 @@ function executeChildPlan( } const flags = ev._flagsAt(sideEffectBucketIndex); if (flags & FLAG_ERROR) { - const e = coerceError( + throw coerceError( ev.at(sideEffectBucketIndex), locationDetails, mutablePath.slice(1), ); - throw e; } } } From b84f778a9f9ff8e9349280c09329050944bf3bff Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 14:03:32 +0100 Subject: [PATCH 08/21] Tests --- grafast/grafast/__tests__/onError-test.ts | 389 ++++++++++++++++++++++ 1 file changed, 389 insertions(+) create mode 100644 grafast/grafast/__tests__/onError-test.ts diff --git a/grafast/grafast/__tests__/onError-test.ts b/grafast/grafast/__tests__/onError-test.ts new file mode 100644 index 0000000000..11df39fa04 --- /dev/null +++ b/grafast/grafast/__tests__/onError-test.ts @@ -0,0 +1,389 @@ +/* eslint-disable graphile-export/exhaustive-deps, graphile-export/export-methods, graphile-export/export-instances, graphile-export/export-subclasses, graphile-export/no-nested */ +import { expect } from "chai"; +import { resolvePreset } from "graphile-config"; +import { type ExecutionResult } from "graphql"; +import { it } from "mocha"; + +import { + context, + get, + grafast, + GraphQLSpecifiedErrorBehaviors, + lambda, + makeGrafastSchema, + Step, +} from "../dist/index.js"; + +const resolvedPreset = resolvePreset({}); +const requestContext = {}; + +declare global { + namespace Grafast { + interface Context { + me?: { + id: number; + name: string; + nonNullable: unknown; + nullable: unknown; + }; + } + } +} + +const schema = makeGrafastSchema({ + typeDefs: /* GraphQL */ ` + type Query { + me: User + } + type User { + id: Int! + name: String! + nonNullable: Int! + nullable: Int + throwNonNullable: Int! + throwNullable: Int + } + `, + objects: { + Query: { + plans: { + me() { + return get(context(), "me"); + }, + }, + }, + User: { + plans: { + throwNonNullable($user: Step) { + return lambda($user, () => { + throw new Error("Threw in non-nullable field"); + }); + }, + throwNullable($user: Step) { + return lambda($user, () => { + throw new Error("Threw in nullable field"); + }); + }, + }, + }, + }, + enableDeferStream: true, +}); + +function throwOnUnhandledRejections(callback: () => Promise) { + return async () => { + let failed: Error | undefined; + function fail(e: Error) { + console.error(`UNHANDLED PROMISE REJECTION: ${e}`); + failed = e; + } + process.on("unhandledRejection", fail); + try { + return await callback(); + } finally { + process.off("unhandledRejection", fail); + if (failed) { + failed = undefined; + // eslint-disable-next-line no-unsafe-finally + throw new Error(`Unhandled promise rejection occurred`); + } + } + }; +} + +GraphQLSpecifiedErrorBehaviors.forEach((onError) => { + describe(`onError=${onError}`, () => { + it( + "handles null in non-nullable position", + throwOnUnhandledRejections(async () => { + const source = /* GraphQL */ ` + { + me { + id + nonNullable + nullable + name + } + } + `; + const result = (await grafast({ + schema, + source, + onError, + contextValue: { + me: { + id: 42, + nonNullable: null, + nullable: null, + name: "Benjie", + }, + }, + requestContext, + resolvedPreset, + })) as ExecutionResult; + + // All error modes expect the same errors: + expect(result.errors).to.have.length(1); + expect(result.errors![0]).to.deep.include({ + message: + "Cannot return null for non-nullable field User.nonNullable.", + path: ["me", "nonNullable"], + }); + + switch (onError) { + case "PROPAGATE": { + expect(result.data).to.deep.equal({ + me: null, + }); + break; + } + case "NULL": { + expect(result.data).to.deep.equal({ + me: { + id: 42, + nonNullable: null, + nullable: null, + name: "Benjie", + }, + }); + break; + } + case "HALT": { + expect(result.data).to.equal(null); + break; + } + default: { + const never: never = onError; + throw new Error(`Unexpected onError: ${never}`); + } + } + }), + ); + + it( + "handles null in nullable position", + throwOnUnhandledRejections(async () => { + const source = /* GraphQL */ ` + { + me { + id + nonNullable + nullable + name + } + } + `; + const result = (await grafast({ + schema, + source, + onError, + contextValue: { + me: { + id: 42, + nonNullable: 27, + nullable: null, + name: "Benjie", + }, + }, + requestContext, + resolvedPreset, + })) as ExecutionResult; + + expect(result.errors).to.be.undefined; + expect(result.data).to.deep.equal({ + me: { + id: 42, + nonNullable: 27, + nullable: null, + name: "Benjie", + }, + }); + }), + ); + + it( + "handles coercion failure in non-nullable position", + throwOnUnhandledRejections(async () => { + const source = /* GraphQL */ ` + { + me { + id + nonNullable + nullable + name + } + } + `; + const result = (await grafast({ + schema, + source, + onError, + contextValue: { + me: { + id: 42, + nonNullable: "NOT A NUMBER", + nullable: null, + name: "Benjie", + }, + }, + requestContext, + resolvedPreset, + })) as ExecutionResult; + + // All error modes expect the same errors: + expect(result.errors).to.have.length(1); + expect(result.errors![0]).to.deep.include({ + message: `Int cannot represent non-integer value: "NOT A NUMBER"`, + path: ["me", "nonNullable"], + }); + + switch (onError) { + case "PROPAGATE": { + expect(result.data).to.deep.equal({ + me: null, + }); + break; + } + case "NULL": { + expect(result.data).to.deep.equal({ + me: { + id: 42, + nonNullable: null, + nullable: null, + name: "Benjie", + }, + }); + break; + } + case "HALT": { + expect(result.data).to.equal(null); + break; + } + default: { + const never: never = onError; + throw new Error(`Unexpected onError: ${never}`); + } + } + }), + ); + + it( + "handles thrown error in nullable position", + throwOnUnhandledRejections(async () => { + const source = /* GraphQL */ ` + { + me { + id + throwNullable + name + } + } + `; + const result = (await grafast({ + schema, + source, + onError, + contextValue: { + me: { + id: 42, + name: "Benjie", + }, + }, + requestContext, + resolvedPreset, + })) as ExecutionResult; + + // All error modes expect the same errors: + expect(result.errors).to.have.length(1); + expect(result.errors![0]).to.deep.include({ + message: `Threw in nullable field`, + path: ["me", "throwNullable"], + }); + + switch (onError) { + case "NULL": + case "PROPAGATE": { + expect(result.data).to.deep.equal({ + me: { + id: 42, + throwNullable: null, + name: "Benjie", + }, + }); + break; + } + case "HALT": { + expect(result.data).to.equal(null); + break; + } + default: { + const never: never = onError; + throw new Error(`Unexpected onError: ${never}`); + } + } + }), + ); + + it( + "handles thrown error in non-nullable position", + throwOnUnhandledRejections(async () => { + const source = /* GraphQL */ ` + { + me { + id + throwNonNullable + name + } + } + `; + const result = (await grafast({ + schema, + source, + onError, + contextValue: { + me: { + id: 42, + name: "Benjie", + }, + }, + requestContext, + resolvedPreset, + })) as ExecutionResult; + + // All error modes expect the same errors: + expect(result.errors).to.have.length(1); + expect(result.errors![0]).to.deep.include({ + message: `Threw in non-nullable field`, + path: ["me", "throwNonNullable"], + }); + + switch (onError) { + case "NULL": { + expect(result.data).to.deep.equal({ + me: { + id: 42, + throwNonNullable: null, + name: "Benjie", + }, + }); + break; + } + case "PROPAGATE": { + expect(result.data).to.deep.equal({ + me: null, + }); + break; + } + case "HALT": { + expect(result.data).to.equal(null); + break; + } + default: { + const never: never = onError; + throw new Error(`Unexpected onError: ${never}`); + } + } + }), + ); + }); +}); From b5fcf9d764a25a9a4d4d5e98e78891180d7e2719 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 14:50:07 +0100 Subject: [PATCH 09/21] Check within lists --- grafast/grafast/__tests__/onError-test.ts | 71 +++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/grafast/grafast/__tests__/onError-test.ts b/grafast/grafast/__tests__/onError-test.ts index 11df39fa04..8a9abaa601 100644 --- a/grafast/grafast/__tests__/onError-test.ts +++ b/grafast/grafast/__tests__/onError-test.ts @@ -25,6 +25,8 @@ declare global { name: string; nonNullable: unknown; nullable: unknown; + nonNullableList: unknown; + nullableList: unknown; }; } } @@ -40,6 +42,8 @@ const schema = makeGrafastSchema({ name: String! nonNullable: Int! nullable: Int + nonNullableList: [Int!]! + nullableList: [Int] throwNonNullable: Int! throwNullable: Int } @@ -385,5 +389,72 @@ GraphQLSpecifiedErrorBehaviors.forEach((onError) => { } }), ); + + it( + "handles null in non-nullable list position", + throwOnUnhandledRejections(async () => { + const source = /* GraphQL */ ` + { + me { + id + nonNullableList + nullableList + name + } + } + `; + const result = (await grafast({ + schema, + source, + onError, + contextValue: { + me: { + id: 42, + nonNullableList: [1, 1, 2, null, 5], + nullableList: null, + name: "Benjie", + }, + }, + requestContext, + resolvedPreset, + })) as ExecutionResult; + + // All error modes expect the same errors: + expect(result.errors).to.have.length(1); + expect(result.errors![0]).to.deep.include({ + message: + "Cannot return null for non-nullable field User.nonNullableList.", + path: ["me", "nonNullableList", 3], + }); + + switch (onError) { + case "PROPAGATE": { + expect(result.data).to.deep.equal({ + me: null, + }); + break; + } + case "NULL": { + expect(result.data).to.deep.equal({ + me: { + id: 42, + nonNullableList: [1, 1, 2, null, 5], + nullableList: null, + name: "Benjie", + }, + }); + break; + } + case "HALT": { + expect(result.data).to.equal(null); + break; + } + default: { + const never: never = onError; + throw new Error(`Unexpected onError: ${never}`); + } + } + }), + ); }); }); From 228198f7bf927d24695f7ab08c070938a6581a74 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 15:56:22 +0100 Subject: [PATCH 10/21] Test nested --- grafast/grafast/__tests__/onError-test.ts | 101 ++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/grafast/grafast/__tests__/onError-test.ts b/grafast/grafast/__tests__/onError-test.ts index 8a9abaa601..9db423181e 100644 --- a/grafast/grafast/__tests__/onError-test.ts +++ b/grafast/grafast/__tests__/onError-test.ts @@ -5,11 +5,13 @@ import { type ExecutionResult } from "graphql"; import { it } from "mocha"; import { + constant, context, get, grafast, GraphQLSpecifiedErrorBehaviors, lambda, + list, makeGrafastSchema, Step, } from "../dist/index.js"; @@ -46,6 +48,7 @@ const schema = makeGrafastSchema({ nullableList: [Int] throwNonNullable: Int! throwNullable: Int + friends: [User!] } `, objects: { @@ -58,6 +61,17 @@ const schema = makeGrafastSchema({ }, User: { plans: { + friends($user) { + return list([ + constant({ + id: 99, + name: "Test", + nonNullable: 99, + nonNullableList: [], + }), + $user, + ]); + }, throwNonNullable($user: Step) { return lambda($user, () => { throw new Error("Threw in non-nullable field"); @@ -456,5 +470,92 @@ GraphQLSpecifiedErrorBehaviors.forEach((onError) => { } }), ); + + it( + "handles null in non-nullable position inside a non-nullable list position", + throwOnUnhandledRejections(async () => { + const source = /* GraphQL */ ` + { + me { + id + name + friends { + id + name + nonNullableList + nullableList + } + } + } + `; + const result = (await grafast({ + schema, + source, + onError, + contextValue: { + me: { + id: 42, + nonNullableList: [1, 1, 2, null, 5], + nullableList: null, + name: "Benjie", + }, + }, + requestContext, + resolvedPreset, + })) as ExecutionResult; + + // All error modes expect the same errors: + expect(result.errors).to.have.length(1); + expect(result.errors![0]).to.deep.include({ + message: + "Cannot return null for non-nullable field User.nonNullableList.", + path: ["me", "friends", 1, "nonNullableList", 3], + }); + + switch (onError) { + case "PROPAGATE": { + expect(result.data).to.deep.equal({ + me: { + id: 42, + name: "Benjie", + friends: null, + }, + }); + break; + } + case "NULL": { + expect(result.data).to.deep.equal({ + me: { + id: 42, + name: "Benjie", + friends: [ + { + id: 99, + name: "Test", + nonNullableList: [], + nullableList: null, + }, + { + id: 42, + name: "Benjie", + nonNullableList: [1, 1, 2, null, 5], + nullableList: null, + }, + ], + }, + }); + break; + } + case "HALT": { + expect(result.data).to.equal(null); + break; + } + default: { + const never: never = onError; + throw new Error(`Unexpected onError: ${never}`); + } + } + }), + ); }); }); From 4ca53812e96dd9e97f70ade15b7af45ee05479ee Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:02:16 +0100 Subject: [PATCH 11/21] Create error tests object for testing --- postgraphile/postgraphile/graphile.config.ts | 36 ++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/postgraphile/postgraphile/graphile.config.ts b/postgraphile/postgraphile/graphile.config.ts index 55e9b1b0ba..859a0f8f10 100644 --- a/postgraphile/postgraphile/graphile.config.ts +++ b/postgraphile/postgraphile/graphile.config.ts @@ -521,6 +521,42 @@ const preset: GraphileConfig.Preset = { }, }, }), + // Testing errors + extendSchema({ + typeDefs: /* GraphQL */ ` + extend type Query { + errorTests: ErrorTests + } + type ErrorTests { + fourtyTwo: Int! + nonNullableReturningNull: Int! + coercionFailure: Int + } + `, + objects: { + Query: { + plans: { + errorTests: EXPORTABLE( + (constant) => () => constant({}), + [constant], + ), + }, + }, + ErrorTests: { + plans: { + fourtyTwo: EXPORTABLE((constant) => () => constant(42), [constant]), + nonNullableReturningNull: EXPORTABLE( + (constant) => () => constant(null), + [constant], + ), + coercionFailure: EXPORTABLE( + (constant) => () => constant("NOT A NUMBER"), + [constant], + ), + }, + }, + }, + }), // PrimaryKeyMutationsOnlyPlugin, PersistedPlugin, ruruTitle(""), From 21e4ea9e87fdd27ba87195bbc7fd096037132487 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:11:48 +0100 Subject: [PATCH 12/21] Expose onError configuration --- .../ruru-components/src/hooks/useStorage.ts | 16 ++++++++--- grafast/ruru-components/src/ruru.tsx | 27 +++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/grafast/ruru-components/src/hooks/useStorage.ts b/grafast/ruru-components/src/hooks/useStorage.ts index 15c708b4aa..12beab5517 100644 --- a/grafast/ruru-components/src/hooks/useStorage.ts +++ b/grafast/ruru-components/src/hooks/useStorage.ts @@ -8,8 +8,13 @@ export interface StoredKeys { explainSize: string; verbose: "true" | ""; condensed: "true" | ""; + onError: "PROPAGATE" | "NULL" | "HALT"; } +const storageDefault: { [key in keyof StoredKeys]?: StoredKeys[key] } = { + onError: "PROPAGATE", +}; + const KEYS: { [key in keyof StoredKeys]: string } = { explain: "Ruru:explain", explainSize: "Ruru:explainSize", @@ -18,14 +23,19 @@ const KEYS: { [key in keyof StoredKeys]: string } = { explorerIsOpen: "graphiql:explorerIsOpen", verbose: "Ruru:verbose", condensed: "Ruru:condensed", + onError: "Ruru:onError", }; +type ToggleableKey = { + [K in keyof StoredKeys]: StoredKeys[K] extends "true" | "" ? K : never; +}[keyof StoredKeys]; + const up = (v: number) => v + 1; export interface RuruStorage { get(key: TKey): StoredKeys[TKey] | null; set(key: TKey, value: StoredKeys[TKey]): void; - toggle(key: TKey): void; + toggle(key: ToggleableKey): void; } export const useStorage = (): RuruStorage => { @@ -39,7 +49,7 @@ export const useStorage = (): RuruStorage => { return { _revision: revision, get(key) { - return cache[key] ?? null; + return cache[key] ?? storageDefault[key] ?? null; }, set(key, value) { cache[key] = value; @@ -57,7 +67,7 @@ export const useStorage = (): RuruStorage => { storage.removeItem(KEYS[key]); return null; } - return val ?? null; + return val ?? storageDefault[key] ?? null; }, set(key, value) { storage.setItem(KEYS[key], value); diff --git a/grafast/ruru-components/src/ruru.tsx b/grafast/ruru-components/src/ruru.tsx index af5995f8b7..9987a06da8 100644 --- a/grafast/ruru-components/src/ruru.tsx +++ b/grafast/ruru-components/src/ruru.tsx @@ -221,6 +221,33 @@ export const RuruInner: FC<{ Condensed + storage.set("onError", "PROPAGATE")} + > + + {storage.get("onError") === "PROPAGATE" ? check : nocheck} + onError: PROPAGATE + + + storage.set("onError", "NULL")} + > + + {storage.get("onError") === "NULL" ? check : nocheck} + onError: NULL + + + storage.set("onError", "HALT")} + > + + {storage.get("onError") === "HALT" ? check : nocheck} + onError: HALT + + From 593a944a6de73fb8a852f782f1a601a7ed569d67 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:23:08 +0100 Subject: [PATCH 13/21] Give Ruru support for the different onError modes --- grafast/ruru-components/src/hooks/useFetcher.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/grafast/ruru-components/src/hooks/useFetcher.ts b/grafast/ruru-components/src/hooks/useFetcher.ts index fd89159dd0..d497be9af9 100644 --- a/grafast/ruru-components/src/hooks/useFetcher.ts +++ b/grafast/ruru-components/src/hooks/useFetcher.ts @@ -15,6 +15,7 @@ import { getOperationAST, parse } from "graphql"; import { useEffect, useMemo, useState } from "react"; import type { RuruProps } from "../interfaces.js"; +import { useStorage } from "./useStorage.js"; export interface IExplainedOperation { type: string; @@ -102,6 +103,7 @@ export const useFetcher = ( props: RuruProps, options: { explain?: boolean; verbose?: boolean } = {}, ) => { + const storage = useStorage(); const [streamEndpoint, setStreamEndpoint] = useState(null); const endpoint = props.endpoint ?? "/graphql"; const url = endpoint.startsWith("/") @@ -235,8 +237,15 @@ export const useFetcher = ( return result; }; return async function ( - ...args: Parameters + ...inArgs: Parameters ): Promise> { + const [params, ...rest] = inArgs; + const onError = storage.get("onError"); + const args = [ + onError !== "PROPAGATE" ? { ...params, onError } : params, + ...rest, + ] as const; + const result = await fetcher(...args); // Short circuit the introspection query so as to not confuse people @@ -275,7 +284,7 @@ export const useFetcher = ( return processPayload(result) as any; } }; - }, [fetcher, verbose]); + }, [fetcher, storage, verbose]); return { fetcher: wrappedFetcher, explainResults, streamEndpoint }; }; From fe589ebe649715474a691826149798f517286d0f Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:25:50 +0100 Subject: [PATCH 14/21] Don't assume the default onError is PROPAGATE --- grafast/ruru-components/src/hooks/useFetcher.ts | 2 +- grafast/ruru-components/src/hooks/useStorage.ts | 4 ++-- grafast/ruru-components/src/ruru.tsx | 8 +++++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/grafast/ruru-components/src/hooks/useFetcher.ts b/grafast/ruru-components/src/hooks/useFetcher.ts index d497be9af9..edce71613f 100644 --- a/grafast/ruru-components/src/hooks/useFetcher.ts +++ b/grafast/ruru-components/src/hooks/useFetcher.ts @@ -242,7 +242,7 @@ export const useFetcher = ( const [params, ...rest] = inArgs; const onError = storage.get("onError"); const args = [ - onError !== "PROPAGATE" ? { ...params, onError } : params, + onError ? { ...params, onError } : params, ...rest, ] as const; diff --git a/grafast/ruru-components/src/hooks/useStorage.ts b/grafast/ruru-components/src/hooks/useStorage.ts index 12beab5517..71c606337e 100644 --- a/grafast/ruru-components/src/hooks/useStorage.ts +++ b/grafast/ruru-components/src/hooks/useStorage.ts @@ -8,11 +8,11 @@ export interface StoredKeys { explainSize: string; verbose: "true" | ""; condensed: "true" | ""; - onError: "PROPAGATE" | "NULL" | "HALT"; + onError: "PROPAGATE" | "NULL" | "HALT" | ""; } const storageDefault: { [key in keyof StoredKeys]?: StoredKeys[key] } = { - onError: "PROPAGATE", + onError: "", }; const KEYS: { [key in keyof StoredKeys]: string } = { diff --git a/grafast/ruru-components/src/ruru.tsx b/grafast/ruru-components/src/ruru.tsx index 9987a06da8..37a4791894 100644 --- a/grafast/ruru-components/src/ruru.tsx +++ b/grafast/ruru-components/src/ruru.tsx @@ -223,7 +223,13 @@ export const RuruInner: FC<{ storage.set("onError", "PROPAGATE")} + onSelect={() => { + if (storage.get("onError") === "PROPAGATE") { + storage.set("onError", ""); + } else { + storage.set("onError", "PROPAGATE"); + } + }} > {storage.get("onError") === "PROPAGATE" ? check : nocheck} From 13513ddaea15ad9498a77de7c4e92679498f99ca Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:26:51 +0100 Subject: [PATCH 15/21] docs(changeset): Add support for `onError` RFC with `PROPAGATE`, `NULL` and `HALT` behaviors implemented. --- .changeset/ninety-animals-ring.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/ninety-animals-ring.md diff --git a/.changeset/ninety-animals-ring.md b/.changeset/ninety-animals-ring.md new file mode 100644 index 0000000000..2915d08b37 --- /dev/null +++ b/.changeset/ninety-animals-ring.md @@ -0,0 +1,9 @@ +--- +"postgraphile": patch +"ruru-components": patch +"grafserv": patch +"grafast": patch +--- + +Add support for `onError` RFC with `PROPAGATE`, `NULL` and `HALT` behaviors +implemented. From 52e4479ef7d4e0cc3240a70a6efa4f3c79a3aa11 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:33:36 +0100 Subject: [PATCH 16/21] Test schema for subscriptions --- postgraphile/postgraphile/graphile.config.ts | 38 ++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/postgraphile/postgraphile/graphile.config.ts b/postgraphile/postgraphile/graphile.config.ts index 859a0f8f10..70a6e40e6a 100644 --- a/postgraphile/postgraphile/graphile.config.ts +++ b/postgraphile/postgraphile/graphile.config.ts @@ -527,7 +527,12 @@ const preset: GraphileConfig.Preset = { extend type Query { errorTests: ErrorTests } + extend type Subscription { + errorTests: ErrorTests + } type ErrorTests { + index: Int + errorOnOddNumbers: Int! fourtyTwo: Int! nonNullableReturningNull: Int! coercionFailure: Int @@ -542,8 +547,41 @@ const preset: GraphileConfig.Preset = { ), }, }, + Subscription: { + plans: { + errorTests: { + subscribePlan: EXPORTABLE( + (lambda, sleep) => () => { + return lambda(null, () => { + return (async function* () { + for (let index = 0; ; index++) { + yield { index }; + await sleep(1000); + } + })(); + }); + }, + [lambda, sleep], + ), + plan: EXPORTABLE(() => function plan($event) { + return $event; + }, []), + }, + }, + }, ErrorTests: { plans: { + errorOnOddNumbers: EXPORTABLE( + (lambda) => ($payload) => + lambda($payload, (p: any) => { + if (p.index % 2 === 1) { + throw new Error(`ODD! ${p.index}`); + } else { + return p.index; + } + }), + [lambda], + ), fourtyTwo: EXPORTABLE((constant) => () => constant(42), [constant]), nonNullableReturningNull: EXPORTABLE( (constant) => () => constant(null), From cb02bb52a0669addfc5c9d89fbaf275b1233b23c Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:34:12 +0100 Subject: [PATCH 17/21] Ruru too --- .changeset/ninety-animals-ring.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.changeset/ninety-animals-ring.md b/.changeset/ninety-animals-ring.md index 2915d08b37..d5a77e7633 100644 --- a/.changeset/ninety-animals-ring.md +++ b/.changeset/ninety-animals-ring.md @@ -1,6 +1,7 @@ --- "postgraphile": patch "ruru-components": patch +"ruru": patch "grafserv": patch "grafast": patch --- From b89e2f958303545698f7a6f87b8170b25e5adecf Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:47:52 +0100 Subject: [PATCH 18/21] No need to mod into GraphQL.js types --- grafast/grafast/src/prepare.ts | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/grafast/grafast/src/prepare.ts b/grafast/grafast/src/prepare.ts index 93c59f68b7..2bdd61b79a 100644 --- a/grafast/grafast/src/prepare.ts +++ b/grafast/grafast/src/prepare.ts @@ -576,13 +576,6 @@ function establishOperationPlanFromEvent(event: EstablishOperationPlanEvent) { ); } -// TODO: upstream this -declare module "graphql/execution/execute.js" { - interface ExecutionContext { - onError: "PROPAGATE" | "NULL" | "HALT"; - } -} - /** * @internal */ @@ -609,11 +602,11 @@ export function grafastPrepare( extensions: rootValue[$$extensions], }); } - if (!exeContext.onError) { - exeContext.onError = args.onError ?? "PROPAGATE"; - } - const { operation, fragments, variableValues, onError } = exeContext; + const { operation, fragments, variableValues } = exeContext; + // TODO: update this when GraphQL.js gets support for onError + const onError = args.onError ?? "PROPAGATE"; + let operationPlan!: OperationPlan; try { if (middleware != null) { From 27b542f4a3c8005ace4ca89a748562178dfae932 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:49:09 +0100 Subject: [PATCH 19/21] Change type assertion --- grafast/grafast/src/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grafast/grafast/src/utils.ts b/grafast/grafast/src/utils.ts index d9135b61ae..70e086fccb 100644 --- a/grafast/grafast/src/utils.ts +++ b/grafast/grafast/src/utils.ts @@ -1518,4 +1518,4 @@ export const GraphQLSpecifiedErrorBehaviors = Object.freeze([ "PROPAGATE", "NULL", "HALT", -]) as readonly ErrorBehavior[]; +] as const); From e73372cb4dacc271a1bbdf398459f3a4aa381b7f Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 16:55:20 +0100 Subject: [PATCH 20/21] Lint --- grafast/grafast/src/bucket.ts | 2 -- grafast/grafast/src/grafastGraphql.ts | 1 - grafast/grafast/src/utils.ts | 1 - grafast/grafserv/src/utils.ts | 7 +------ 4 files changed, 1 insertion(+), 10 deletions(-) diff --git a/grafast/grafast/src/bucket.ts b/grafast/grafast/src/bucket.ts index 15aae9cb8d..dc7fd267e4 100644 --- a/grafast/grafast/src/bucket.ts +++ b/grafast/grafast/src/bucket.ts @@ -1,7 +1,5 @@ // import type { GraphQLScalarType } from "graphql"; -import { ExecutionContext } from "graphql/execution/execute.js"; - import type { ErrorBehavior, GrafastExecutionArgs, Step } from "."; import type { LayerPlan } from "./engine/LayerPlan"; import type { MetaByMetaKey } from "./engine/OperationPlan"; diff --git a/grafast/grafast/src/grafastGraphql.ts b/grafast/grafast/src/grafastGraphql.ts index 064d64b9ac..0d73c2fbc3 100644 --- a/grafast/grafast/src/grafastGraphql.ts +++ b/grafast/grafast/src/grafastGraphql.ts @@ -13,7 +13,6 @@ import { SafeError } from "./error.js"; import { execute } from "./execute.js"; import { hookArgs } from "./index.js"; import type { - ErrorBehavior, GrafastArgs, GrafastExecutionArgs, ParseAndValidateEvent, diff --git a/grafast/grafast/src/utils.ts b/grafast/grafast/src/utils.ts index 70e086fccb..b08f2ed962 100644 --- a/grafast/grafast/src/utils.ts +++ b/grafast/grafast/src/utils.ts @@ -33,7 +33,6 @@ import { SafeError } from "./error.js"; import { inspect } from "./inspect.js"; import type { BaseGraphQLArguments, - ErrorBehavior, ExecutionEntryFlags, GrafastFieldConfig, GrafastInputFieldConfig, diff --git a/grafast/grafserv/src/utils.ts b/grafast/grafserv/src/utils.ts index 9c9d936c63..653ef3fa57 100644 --- a/grafast/grafserv/src/utils.ts +++ b/grafast/grafserv/src/utils.ts @@ -1,11 +1,6 @@ import type { Readable } from "node:stream"; -import type { - ErrorBehavior, - execute, - GrafastExecutionArgs, - subscribe, -} from "grafast"; +import type { execute, GrafastExecutionArgs, subscribe } from "grafast"; import { hookArgs, SafeError, stripAnsi } from "grafast"; import type { ExecutionArgs, From abdbad94b6d326217491320f0d993cb62f83c92a Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 30 Aug 2025 17:07:02 +0100 Subject: [PATCH 21/21] Format --- postgraphile/postgraphile/graphile.config.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/postgraphile/postgraphile/graphile.config.ts b/postgraphile/postgraphile/graphile.config.ts index 70a6e40e6a..1ad3b10391 100644 --- a/postgraphile/postgraphile/graphile.config.ts +++ b/postgraphile/postgraphile/graphile.config.ts @@ -563,9 +563,13 @@ const preset: GraphileConfig.Preset = { }, [lambda, sleep], ), - plan: EXPORTABLE(() => function plan($event) { - return $event; - }, []), + plan: EXPORTABLE( + () => + function plan($event) { + return $event; + }, + [], + ), }, }, },