Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement onError proposal #4364

Draft
wants to merge 7 commits into
base: 16.x.x
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/error/ErrorBehavior.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export type GraphQLErrorBehavior = 'PROPAGATE' | 'NO_PROPAGATE' | 'ABORT';

export function isErrorBehavior(
onError: unknown,
): onError is GraphQLErrorBehavior {
return (
onError === 'PROPAGATE' || onError === 'NO_PROPAGATE' || onError === 'ABORT'
);
}
1 change: 1 addition & 0 deletions src/error/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ export type {
export { syntaxError } from './syntaxError';

export { locatedError } from './locatedError';
export type { GraphQLErrorBehavior } from './ErrorBehavior';
223 changes: 223 additions & 0 deletions src/execution/__tests__/executor-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ describe('Execute: Handles basic execution tasks', () => {
'rootValue',
'operation',
'variableValues',
'errorBehavior',
);

const operation = document.definitions[0];
Expand All @@ -275,6 +276,7 @@ describe('Execute: Handles basic execution tasks', () => {
schema,
rootValue,
operation,
errorBehavior: 'PROPAGATE',
});

const field = operation.selectionSet.selections[0];
Expand All @@ -285,6 +287,70 @@ describe('Execute: Handles basic execution tasks', () => {
});
});

it('reflects onError:NO_PROPAGATE via errorBehavior', () => {
let resolvedInfo;
const testType = new GraphQLObjectType({
name: 'Test',
fields: {
test: {
type: GraphQLString,
resolve(_val, _args, _ctx, info) {
resolvedInfo = info;
},
},
},
});
const schema = new GraphQLSchema({ query: testType });

const document = parse('query ($var: String) { result: test }');
const rootValue = { root: 'val' };
const variableValues = { var: 'abc' };

executeSync({
schema,
document,
rootValue,
variableValues,
onError: 'NO_PROPAGATE',
});

expect(resolvedInfo).to.include({
errorBehavior: 'NO_PROPAGATE',
});
});

it('reflects onError:ABORT via errorBehavior', () => {
let resolvedInfo;
const testType = new GraphQLObjectType({
name: 'Test',
fields: {
test: {
type: GraphQLString,
resolve(_val, _args, _ctx, info) {
resolvedInfo = info;
},
},
},
});
const schema = new GraphQLSchema({ query: testType });

const document = parse('query ($var: String) { result: test }');
const rootValue = { root: 'val' };
const variableValues = { var: 'abc' };

executeSync({
schema,
document,
rootValue,
variableValues,
onError: 'ABORT',
});

expect(resolvedInfo).to.include({
errorBehavior: 'ABORT',
});
});

it('populates path correctly with complex types', () => {
let path;
const someObject = new GraphQLObjectType({
Expand Down Expand Up @@ -739,6 +805,163 @@ describe('Execute: Handles basic execution tasks', () => {
});
});

it('Full response path is included for non-nullable fields with onError:NO_PROPAGATE', () => {
const A: GraphQLObjectType = new GraphQLObjectType({
name: 'A',
fields: () => ({
nullableA: {
type: A,
resolve: () => ({}),
},
nonNullA: {
type: new GraphQLNonNull(A),
resolve: () => ({}),
},
throws: {
type: new GraphQLNonNull(GraphQLString),
resolve: () => {
throw new Error('Catch me if you can');
},
},
}),
});
const schema = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'query',
fields: () => ({
nullableA: {
type: A,
resolve: () => ({}),
},
}),
}),
});

const document = parse(`
query {
nullableA {
aliasedA: nullableA {
nonNullA {
anotherA: nonNullA {
throws
}
}
}
}
}
`);

const result = executeSync({ schema, document, onError: 'NO_PROPAGATE' });
expectJSON(result).toDeepEqual({
data: {
nullableA: {
aliasedA: {
nonNullA: {
anotherA: {
throws: null,
},
},
},
},
},
errors: [
{
message: 'Catch me if you can',
locations: [{ line: 7, column: 17 }],
path: ['nullableA', 'aliasedA', 'nonNullA', 'anotherA', 'throws'],
},
],
});
});

it('Full response path is included for non-nullable fields with onError:ABORT', () => {
const A: GraphQLObjectType = new GraphQLObjectType({
name: 'A',
fields: () => ({
nullableA: {
type: A,
resolve: () => ({}),
},
nonNullA: {
type: new GraphQLNonNull(A),
resolve: () => ({}),
},
throws: {
type: new GraphQLNonNull(GraphQLString),
resolve: () => {
throw new Error('Catch me if you can');
},
},
}),
});
const schema = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'query',
fields: () => ({
nullableA: {
type: A,
resolve: () => ({}),
},
}),
}),
});

const document = parse(`
query {
nullableA {
aliasedA: nullableA {
nonNullA {
anotherA: nonNullA {
throws
}
}
}
}
}
`);

const result = executeSync({ schema, document, onError: 'ABORT' });
expectJSON(result).toDeepEqual({
data: null,
errors: [
{
message: 'Catch me if you can',
locations: [{ line: 7, column: 17 }],
path: ['nullableA', 'aliasedA', 'nonNullA', 'anotherA', 'throws'],
},
],
});
});

it('raises request error with invalid onError', () => {
const schema = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'query',
fields: () => ({
a: {
type: GraphQLInt,
},
}),
}),
});

const document = parse('{ a }');
const result = executeSync({
schema,
document,
// @ts-expect-error
onError: 'DANCE',
});
expectJSON(result).toDeepEqual({
errors: [
{
message:
'Unsupported `onError` value; supported values are `PROPAGATE`, `NO_PROPAGATE` and `ABORT`.',
},
],
});
});

it('uses the inline operation if no operation name is provided', () => {
const schema = new GraphQLSchema({
query: new GraphQLObjectType({
Expand Down
46 changes: 43 additions & 3 deletions src/execution/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { promiseForObject } from '../jsutils/promiseForObject';
import type { PromiseOrValue } from '../jsutils/PromiseOrValue';
import { promiseReduce } from '../jsutils/promiseReduce';

import type { GraphQLErrorBehavior } from '../error/ErrorBehavior';
import { isErrorBehavior } from '../error/ErrorBehavior';
import type { GraphQLFormattedError } from '../error/GraphQLError';
import { GraphQLError } from '../error/GraphQLError';
import { locatedError } from '../error/locatedError';
Expand Down Expand Up @@ -115,6 +117,7 @@ export interface ExecutionContext {
typeResolver: GraphQLTypeResolver<any, any>;
subscribeFieldResolver: GraphQLFieldResolver<any, any>;
errors: Array<GraphQLError>;
errorBehavior: GraphQLErrorBehavior;
}

/**
Expand All @@ -130,6 +133,7 @@ export interface ExecutionResult<
> {
errors?: ReadonlyArray<GraphQLError>;
data?: TData | null;
onError?: GraphQLErrorBehavior;
extensions?: TExtensions;
}

Expand All @@ -152,6 +156,15 @@ export interface ExecutionArgs {
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>;
typeResolver?: Maybe<GraphQLTypeResolver<any, any>>;
subscribeFieldResolver?: Maybe<GraphQLFieldResolver<any, any>>;
/**
* Experimental. Set to NO_PROPAGATE to prevent error propagation. Set to ABORT to
* abort a request when any error occurs.
*
* Default: PROPAGATE
*
* @experimental
*/
onError?: GraphQLErrorBehavior;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this to the new options?: object that is in here, could maybe facilitate these experimental things better. Alternatively we stick to a flat config

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's specified to be part of the "GraphQL request", so I would expect it top level. I would consider moving some of the non-request properties to be inside an options object though: fieldResolver, typeResolver, subscribeFieldResolver.

}

/**
Expand Down Expand Up @@ -286,8 +299,17 @@ export function buildExecutionContext(
fieldResolver,
typeResolver,
subscribeFieldResolver,
onError,
} = args;

if (onError != null && !isErrorBehavior(onError)) {
return [
new GraphQLError(
'Unsupported `onError` value; supported values are `PROPAGATE`, `NO_PROPAGATE` and `ABORT`.',
),
];
}

let operation: OperationDefinitionNode | undefined;
const fragments: ObjMap<FragmentDefinitionNode> = Object.create(null);
for (const definition of document.definitions) {
Expand Down Expand Up @@ -347,6 +369,7 @@ export function buildExecutionContext(
typeResolver: typeResolver ?? defaultTypeResolver,
subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver,
errors: [],
errorBehavior: onError ?? 'PROPAGATE',
};
}

Expand Down Expand Up @@ -585,6 +608,7 @@ export function buildResolveInfo(
rootValue: exeContext.rootValue,
operation: exeContext.operation,
variableValues: exeContext.variableValues,
errorBehavior: exeContext.errorBehavior,
};
}

Expand All @@ -593,10 +617,26 @@ function handleFieldError(
returnType: GraphQLOutputType,
exeContext: ExecutionContext,
): null {
// 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.errorBehavior === 'PROPAGATE') {
// If the field type is non-nullable, then it is resolved without any
// protection from errors, however it still properly locates the error.
// Note: semantic non-null types are treated as nullable for the purposes
// of error handling.
if (isNonNullType(returnType)) {
throw error;
}
} else if (exeContext.errorBehavior === 'ABORT') {
// In this mode, any error aborts the request
throw error;
} else if (exeContext.errorBehavior === 'NO_PROPAGATE') {
// In this mode, the client takes responsibility for error handling, so we
// treat the field as if it were nullable.
/* c8 ignore next 6 */
} else {
invariant(
false,
'Unexpected errorBehavior setting: ' + inspect(exeContext.errorBehavior),
);
}

// Otherwise, error protection is applied, logging the error and resolving
Expand Down
Loading