From d9c350fc56b18cf56acd314ecd17155111c4b8a0 Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Fri, 21 Feb 2025 17:23:30 +0100 Subject: [PATCH 1/7] Add support for @onError --- src/execution/__tests__/onerror-test.ts | 93 +++++++++++++++++++++++++ src/execution/execute.ts | 16 ++++- src/type/directives.ts | 55 ++++++++++++++- 3 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 src/execution/__tests__/onerror-test.ts diff --git a/src/execution/__tests__/onerror-test.ts b/src/execution/__tests__/onerror-test.ts new file mode 100644 index 0000000000..ae1db3cfda --- /dev/null +++ b/src/execution/__tests__/onerror-test.ts @@ -0,0 +1,93 @@ +import { describe, it } from 'mocha'; + +import { expectJSON } from '../../__testUtils__/expectJSON.js'; + +import type { PromiseOrValue } from '../../jsutils/PromiseOrValue.js'; + +import { parse } from '../../language/parser.js'; + +import { buildSchema } from '../../utilities/buildASTSchema.js'; + +import { execute } from '../execute.js'; +import type { ExecutionResult } from '../types.js'; + +const syncError = new Error('bar'); + +const throwingData = { + foo() { + throw syncError; + }, +}; + +const schema = buildSchema(` + type Query { + foo : Int! + } + + enum _ErrorAction { PROPAGATE, NULL } + directive @onError(action: _ErrorAction) on QUERY | MUTATION | SUBSCRIPTION +`); + +function executeQuery( + query: string, + rootValue: unknown, +): PromiseOrValue { + return execute({ schema, document: parse(query), rootValue }); +} + +describe('Execute: handles errors', () => { + it('with `@onError(action: NULL) returns null', async () => { + const query = ` + query getFoo @onError(action: NULL) { + foo + } + `; + const result = await executeQuery(query, throwingData); + expectJSON(result).toDeepEqual({ + data: { foo: null }, + errors: [ + { + message: 'bar', + path: ['foo'], + locations: [{ line: 3, column: 9 }], + }, + ], + }); + }); + it('with `@onError(action: PROPAGATE) propagates the error', async () => { + const query = ` + query getFoo @onError(action: PROPAGATE) { + foo + } + `; + const result = await executeQuery(query, throwingData); + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + { + message: 'bar', + path: ['foo'], + locations: [{ line: 3, column: 9 }], + }, + ], + }); + }); + it('by default propagates the error', async () => { + const query = ` + query getFoo { + foo + } + `; + const result = await executeQuery(query, throwingData); + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + { + message: 'bar', + path: ['foo'], + locations: [{ line: 3, column: 9 }], + }, + ], + }); + }); +}); diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 6fd3fc5b4a..55b6de7ff2 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -44,7 +44,11 @@ import { isNonNullType, isObjectType, } from '../type/definition.js'; -import { GraphQLStreamDirective } from '../type/directives.js'; +import { + ErrorAction, + GraphQLOnErrorDirective, + GraphQLStreamDirective, +} from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; import { assertValidSchema } from '../type/validate.js'; @@ -170,6 +174,7 @@ export interface ExecutionContext { abortSignalListener: AbortSignalListener | undefined; completed: boolean; cancellableStreams: Set | undefined; + propagateErrors: boolean; } interface IncrementalContext { @@ -314,6 +319,12 @@ export function executeQueryOrMutationOrSubscriptionEvent( return ensureSinglePayload(result); } +function propagateErrors(operation: OperationDefinitionNode): boolean { + const value = getDirectiveValues(GraphQLOnErrorDirective, operation); + + return value?.action !== ErrorAction.NULL; +} + export function experimentalExecuteQueryOrMutationOrSubscriptionEvent( validatedExecutionArgs: ValidatedExecutionArgs, ): PromiseOrValue { @@ -326,6 +337,7 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent( : undefined, completed: false, cancellableStreams: undefined, + propagateErrors: propagateErrors(validatedExecutionArgs.operation), }; try { const { @@ -976,7 +988,7 @@ function handleFieldError( // If the field type is non-nullable, then it is resolved without any // protection from errors, however it still properly locates the error. - if (isNonNullType(returnType)) { + if (exeContext.propagateErrors && isNonNullType(returnType)) { throw error; } diff --git a/src/type/directives.ts b/src/type/directives.ts index 6b6f5d4fa2..9c909d41db 100644 --- a/src/type/directives.ts +++ b/src/type/directives.ts @@ -9,6 +9,7 @@ import { toObjMapWithSymbols } from '../jsutils/toObjMap.js'; import type { DirectiveDefinitionNode } from '../language/ast.js'; import { DirectiveLocation } from '../language/directiveLocation.js'; +import { Kind } from '../language/kinds.js'; import { assertName } from './assertName.js'; import type { @@ -16,7 +17,11 @@ import type { GraphQLFieldNormalizedConfigArgumentMap, GraphQLSchemaElement, } from './definition.js'; -import { GraphQLArgument, GraphQLNonNull } from './definition.js'; +import { + GraphQLArgument, + GraphQLEnumType, + GraphQLNonNull, +} from './definition.js'; import { GraphQLBoolean, GraphQLInt, GraphQLString } from './scalars.js'; /** @@ -276,6 +281,54 @@ export const GraphQLOneOfDirective: GraphQLDirective = new GraphQLDirective({ args: {}, }); +/** + * Possible error handling actions. + */ +export const ErrorAction = { + PROPAGATE: 'PROPAGATE' as const, + NULL: 'NULL' as const, +} as const; + +// eslint-disable-next-line @typescript-eslint/no-redeclare +export type ErrorAction = (typeof ErrorAction)[keyof typeof ErrorAction]; + +export const _ErrorAction = new GraphQLEnumType({ + name: '_ErrorAction', + description: 'Possible error handling actions.', + values: { + PROPAGATE: { + value: ErrorAction.PROPAGATE, + description: + 'Non-nullable positions that error cause the error to propagate to the nearest nullable ancestor position. The error is added to the "errors" list.', + }, + NULL: { + value: ErrorAction.NULL, + description: + 'Positions that error are replaced with a `null` and an error is added to the "errors list.', + }, + }, +}); + +/** + * Controls how the executor handles errors. + */ +export const GraphQLOnErrorDirective = new GraphQLDirective({ + name: 'onError', + description: 'Controls how the executor handles errors.', + locations: [ + DirectiveLocation.QUERY, + DirectiveLocation.MUTATION, + DirectiveLocation.SUBSCRIPTION, + ], + args: { + action: { + type: new GraphQLNonNull(_ErrorAction), + description: 'The action to execute when a field error is encountered.', + default: { literal: { kind: Kind.ENUM, value: 'PROPAGATE' } }, + }, + }, +}); + /** * The full list of specified directives. */ From 5d4bec2648ec83b0aa2d6ea7a194962f43c4c724 Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Fri, 21 Feb 2025 17:39:51 +0100 Subject: [PATCH 2/7] typo --- src/type/directives.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/type/directives.ts b/src/type/directives.ts index 9c909d41db..0a03172d69 100644 --- a/src/type/directives.ts +++ b/src/type/directives.ts @@ -304,7 +304,7 @@ export const _ErrorAction = new GraphQLEnumType({ NULL: { value: ErrorAction.NULL, description: - 'Positions that error are replaced with a `null` and an error is added to the "errors list.', + 'Positions that error are replaced with a `null` and an error is added to the "errors" list.', }, }, }); From 6409cf6809156930f3874e0159908525932ec588 Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Sat, 22 Feb 2025 00:12:11 +0100 Subject: [PATCH 3/7] propagateErrors -> errorPropagation --- src/execution/execute.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 55b6de7ff2..9d043512f3 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -174,7 +174,7 @@ export interface ExecutionContext { abortSignalListener: AbortSignalListener | undefined; completed: boolean; cancellableStreams: Set | undefined; - propagateErrors: boolean; + errorPropagation: boolean; } interface IncrementalContext { @@ -319,7 +319,7 @@ export function executeQueryOrMutationOrSubscriptionEvent( return ensureSinglePayload(result); } -function propagateErrors(operation: OperationDefinitionNode): boolean { +function errorPropagation(operation: OperationDefinitionNode): boolean { const value = getDirectiveValues(GraphQLOnErrorDirective, operation); return value?.action !== ErrorAction.NULL; @@ -337,7 +337,7 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent( : undefined, completed: false, cancellableStreams: undefined, - propagateErrors: propagateErrors(validatedExecutionArgs.operation), + errorPropagation: errorPropagation(validatedExecutionArgs.operation), }; try { const { @@ -988,7 +988,7 @@ function handleFieldError( // If the field type is non-nullable, then it is resolved without any // protection from errors, however it still properly locates the error. - if (exeContext.propagateErrors && isNonNullType(returnType)) { + if (exeContext.errorPropagation && isNonNullType(returnType)) { throw error; } From f96492c096c373f2287cffc3328258245a020b82 Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Sat, 22 Feb 2025 00:19:49 +0100 Subject: [PATCH 4/7] Rename to @experimental_disableErrorPropagation --- src/execution/__tests__/onerror-test.ts | 93 ------------------------- src/execution/execute.ts | 10 +-- src/type/directives.ts | 50 ++----------- 3 files changed, 11 insertions(+), 142 deletions(-) delete mode 100644 src/execution/__tests__/onerror-test.ts diff --git a/src/execution/__tests__/onerror-test.ts b/src/execution/__tests__/onerror-test.ts deleted file mode 100644 index ae1db3cfda..0000000000 --- a/src/execution/__tests__/onerror-test.ts +++ /dev/null @@ -1,93 +0,0 @@ -import { describe, it } from 'mocha'; - -import { expectJSON } from '../../__testUtils__/expectJSON.js'; - -import type { PromiseOrValue } from '../../jsutils/PromiseOrValue.js'; - -import { parse } from '../../language/parser.js'; - -import { buildSchema } from '../../utilities/buildASTSchema.js'; - -import { execute } from '../execute.js'; -import type { ExecutionResult } from '../types.js'; - -const syncError = new Error('bar'); - -const throwingData = { - foo() { - throw syncError; - }, -}; - -const schema = buildSchema(` - type Query { - foo : Int! - } - - enum _ErrorAction { PROPAGATE, NULL } - directive @onError(action: _ErrorAction) on QUERY | MUTATION | SUBSCRIPTION -`); - -function executeQuery( - query: string, - rootValue: unknown, -): PromiseOrValue { - return execute({ schema, document: parse(query), rootValue }); -} - -describe('Execute: handles errors', () => { - it('with `@onError(action: NULL) returns null', async () => { - const query = ` - query getFoo @onError(action: NULL) { - foo - } - `; - const result = await executeQuery(query, throwingData); - expectJSON(result).toDeepEqual({ - data: { foo: null }, - errors: [ - { - message: 'bar', - path: ['foo'], - locations: [{ line: 3, column: 9 }], - }, - ], - }); - }); - it('with `@onError(action: PROPAGATE) propagates the error', async () => { - const query = ` - query getFoo @onError(action: PROPAGATE) { - foo - } - `; - const result = await executeQuery(query, throwingData); - expectJSON(result).toDeepEqual({ - data: null, - errors: [ - { - message: 'bar', - path: ['foo'], - locations: [{ line: 3, column: 9 }], - }, - ], - }); - }); - it('by default propagates the error', async () => { - const query = ` - query getFoo { - foo - } - `; - const result = await executeQuery(query, throwingData); - expectJSON(result).toDeepEqual({ - data: null, - errors: [ - { - message: 'bar', - path: ['foo'], - locations: [{ line: 3, column: 9 }], - }, - ], - }); - }); -}); diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 9d043512f3..71cbdb6b42 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -45,8 +45,7 @@ import { isObjectType, } from '../type/definition.js'; import { - ErrorAction, - GraphQLOnErrorDirective, + GraphQLDisableErrorPropagationDirective, GraphQLStreamDirective, } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; @@ -320,9 +319,12 @@ export function executeQueryOrMutationOrSubscriptionEvent( } function errorPropagation(operation: OperationDefinitionNode): boolean { - const value = getDirectiveValues(GraphQLOnErrorDirective, operation); + const directiveNode = operation.directives?.find( + (directive) => + directive.name.value === GraphQLDisableErrorPropagationDirective.name, + ); - return value?.action !== ErrorAction.NULL; + return directiveNode === undefined; } export function experimentalExecuteQueryOrMutationOrSubscriptionEvent( diff --git a/src/type/directives.ts b/src/type/directives.ts index 0a03172d69..96f5b6b65a 100644 --- a/src/type/directives.ts +++ b/src/type/directives.ts @@ -9,7 +9,6 @@ import { toObjMapWithSymbols } from '../jsutils/toObjMap.js'; import type { DirectiveDefinitionNode } from '../language/ast.js'; import { DirectiveLocation } from '../language/directiveLocation.js'; -import { Kind } from '../language/kinds.js'; import { assertName } from './assertName.js'; import type { @@ -17,11 +16,7 @@ import type { GraphQLFieldNormalizedConfigArgumentMap, GraphQLSchemaElement, } from './definition.js'; -import { - GraphQLArgument, - GraphQLEnumType, - GraphQLNonNull, -} from './definition.js'; +import { GraphQLArgument, GraphQLNonNull } from './definition.js'; import { GraphQLBoolean, GraphQLInt, GraphQLString } from './scalars.js'; /** @@ -282,51 +277,16 @@ export const GraphQLOneOfDirective: GraphQLDirective = new GraphQLDirective({ }); /** - * Possible error handling actions. + * Disables error propagation (experimental). */ -export const ErrorAction = { - PROPAGATE: 'PROPAGATE' as const, - NULL: 'NULL' as const, -} as const; - -// eslint-disable-next-line @typescript-eslint/no-redeclare -export type ErrorAction = (typeof ErrorAction)[keyof typeof ErrorAction]; - -export const _ErrorAction = new GraphQLEnumType({ - name: '_ErrorAction', - description: 'Possible error handling actions.', - values: { - PROPAGATE: { - value: ErrorAction.PROPAGATE, - description: - 'Non-nullable positions that error cause the error to propagate to the nearest nullable ancestor position. The error is added to the "errors" list.', - }, - NULL: { - value: ErrorAction.NULL, - description: - 'Positions that error are replaced with a `null` and an error is added to the "errors" list.', - }, - }, -}); - -/** - * Controls how the executor handles errors. - */ -export const GraphQLOnErrorDirective = new GraphQLDirective({ - name: 'onError', - description: 'Controls how the executor handles errors.', +export const GraphQLDisableErrorPropagationDirective = new GraphQLDirective({ + name: 'experimental_disableErrorPropagation', + description: 'Disables error propagation.', locations: [ DirectiveLocation.QUERY, DirectiveLocation.MUTATION, DirectiveLocation.SUBSCRIPTION, ], - args: { - action: { - type: new GraphQLNonNull(_ErrorAction), - description: 'The action to execute when a field error is encountered.', - default: { literal: { kind: Kind.ENUM, value: 'PROPAGATE' } }, - }, - }, }); /** From 26e5375fd064837390d71936df1ed74a8f1d2d2f Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Sat, 22 Feb 2025 00:28:39 +0100 Subject: [PATCH 5/7] Forgot to add file --- .../__tests__/errorPropagation-test.ts | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 src/execution/__tests__/errorPropagation-test.ts diff --git a/src/execution/__tests__/errorPropagation-test.ts b/src/execution/__tests__/errorPropagation-test.ts new file mode 100644 index 0000000000..a935ddced8 --- /dev/null +++ b/src/execution/__tests__/errorPropagation-test.ts @@ -0,0 +1,74 @@ +import { describe, it } from 'mocha'; + +import { expectJSON } from '../../__testUtils__/expectJSON.js'; + +import type { PromiseOrValue } from '../../jsutils/PromiseOrValue.js'; + +import { parse } from '../../language/parser.js'; + +import { buildSchema } from '../../utilities/buildASTSchema.js'; + +import { execute } from '../execute.js'; +import type { ExecutionResult } from '../types.js'; + +const syncError = new Error('bar'); + +const throwingData = { + foo() { + throw syncError; + }, +}; + +const schema = buildSchema(` + type Query { + foo : Int! + } + + directive @experimental_disableErrorPropagation on QUERY | MUTATION | SUBSCRIPTION +`); + +function executeQuery( + query: string, + rootValue: unknown, +): PromiseOrValue { + return execute({ schema, document: parse(query), rootValue }); +} + +describe('Execute: handles errors', () => { + it('with `@experimental_disableErrorPropagation returns null', async () => { + const query = ` + query getFoo @experimental_disableErrorPropagation { + foo + } + `; + const result = await executeQuery(query, throwingData); + expectJSON(result).toDeepEqual({ + data: { foo: null }, + errors: [ + { + message: 'bar', + path: ['foo'], + locations: [{ line: 3, column: 9 }], + }, + ], + }); + }); + it('without `experimental_disableErrorPropagation` propagates the error', async () => { + const query = ` + query getFoo { + foo + } + `; + const result = await executeQuery(query, throwingData); + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + { + message: 'bar', + path: ['foo'], + locations: [{ line: 3, column: 9 }], + }, + ], + }); + }); +}); From 2aaf644c2fdfba1b742c2b85f8babb8e04db221f Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Mon, 24 Feb 2025 11:59:13 +0100 Subject: [PATCH 6/7] Update src/execution/__tests__/errorPropagation-test.ts Co-authored-by: Jovi De Croock --- src/execution/__tests__/errorPropagation-test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/execution/__tests__/errorPropagation-test.ts b/src/execution/__tests__/errorPropagation-test.ts index a935ddced8..32ad40c576 100644 --- a/src/execution/__tests__/errorPropagation-test.ts +++ b/src/execution/__tests__/errorPropagation-test.ts @@ -1,13 +1,9 @@ import { describe, it } from 'mocha'; import { expectJSON } from '../../__testUtils__/expectJSON.js'; - import type { PromiseOrValue } from '../../jsutils/PromiseOrValue.js'; - import { parse } from '../../language/parser.js'; - import { buildSchema } from '../../utilities/buildASTSchema.js'; - import { execute } from '../execute.js'; import type { ExecutionResult } from '../types.js'; From aec37aa057e950871821be917a854eb1cacab639 Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Mon, 24 Feb 2025 12:31:00 +0100 Subject: [PATCH 7/7] Fix lint --- src/execution/__tests__/errorPropagation-test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/execution/__tests__/errorPropagation-test.ts b/src/execution/__tests__/errorPropagation-test.ts index 32ad40c576..a935ddced8 100644 --- a/src/execution/__tests__/errorPropagation-test.ts +++ b/src/execution/__tests__/errorPropagation-test.ts @@ -1,9 +1,13 @@ import { describe, it } from 'mocha'; import { expectJSON } from '../../__testUtils__/expectJSON.js'; + import type { PromiseOrValue } from '../../jsutils/PromiseOrValue.js'; + import { parse } from '../../language/parser.js'; + import { buildSchema } from '../../utilities/buildASTSchema.js'; + import { execute } from '../execute.js'; import type { ExecutionResult } from '../types.js';