From 29ead57c195a42a1754ce9ec78ffba02524b9916 Mon Sep 17 00:00:00 2001 From: Greg MacWilliam Date: Sun, 29 Nov 2020 15:48:09 -0500 Subject: [PATCH] fix(batch-delegate): proxy batched errors (#2288) Previously, batch-delegate did not reset the path on errors, so an array index would spuriously end up in the path. The new onLocatedError option to delegateToSchema allows batchDelegateToSchema to remove the array index after the array index is used to properly merge the error into the returned object. Previously, any error returned by the batch loader would result in throwing of a new error => now errors returned by the batch loader are caught and returned as is (cf. https://github.com/graphql/graphql-js/blob/cd273ad136d615b3f2f4c830bd8891c7c5590c30/src/execution/execute.js#L693) --- .changeset/seven-dryers-vanish.md | 6 ++ packages/batch-delegate/src/getLoader.ts | 7 +++ packages/delegate/src/delegateToSchema.ts | 2 + packages/delegate/src/mergeFields.ts | 5 +- .../transforms/CheckResultAndHandleErrors.ts | 18 ++++-- packages/delegate/src/types.ts | 2 + packages/stitch/tests/mergeFailures.test.ts | 58 +++++++++++++++++++ 7 files changed, 91 insertions(+), 7 deletions(-) create mode 100644 .changeset/seven-dryers-vanish.md diff --git a/.changeset/seven-dryers-vanish.md b/.changeset/seven-dryers-vanish.md new file mode 100644 index 00000000000..2360b5f7fd5 --- /dev/null +++ b/.changeset/seven-dryers-vanish.md @@ -0,0 +1,6 @@ +--- +'@graphql-tools/batch-delegate': patch +'@graphql-tools/delegate': patch +--- + +fix(batch-delegate): proxy batched errors diff --git a/packages/batch-delegate/src/getLoader.ts b/packages/batch-delegate/src/getLoader.ts index 88db4513cb7..d1536dd91e5 100644 --- a/packages/batch-delegate/src/getLoader.ts +++ b/packages/batch-delegate/src/getLoader.ts @@ -3,6 +3,7 @@ import { getNamedType, GraphQLOutputType, GraphQLList, GraphQLSchema } from 'gra import DataLoader from 'dataloader'; import { delegateToSchema, SubschemaConfig } from '@graphql-tools/delegate'; +import { relocatedError } from '@graphql-tools/utils'; import { BatchDelegateOptions, DataLoaderCache } from './types'; @@ -15,10 +16,16 @@ function createBatchFn(options: BatchDelegateOptions) { return async (keys: ReadonlyArray) => { const results = await delegateToSchema({ returnType: new GraphQLList(getNamedType(options.info.returnType) as GraphQLOutputType), + onLocatedError: originalError => + relocatedError(originalError, originalError.path.slice(0, 0).concat(originalError.path.slice(2))), args: argsFromKeys(keys), ...(lazyOptionsFn == null ? options : lazyOptionsFn(options)), }); + if (results instanceof Error) { + return keys.map(() => results); + } + const values = valuesFromResults == null ? results : valuesFromResults(results, keys); return Array.isArray(values) ? values : keys.map(() => values); diff --git a/packages/delegate/src/delegateToSchema.ts b/packages/delegate/src/delegateToSchema.ts index 91a32a65871..287b650c7bc 100644 --- a/packages/delegate/src/delegateToSchema.ts +++ b/packages/delegate/src/delegateToSchema.ts @@ -92,6 +92,7 @@ export function delegateRequest({ fieldName, args, returnType, + onLocatedError, context, transforms = [], transformedSchema, @@ -134,6 +135,7 @@ export function delegateRequest({ info, returnType: returnType ?? info?.returnType ?? getDelegationReturnType(targetSchema, targetOperation, targetFieldName), + onLocatedError, transforms: allTransforms, transformedSchema: transformedSchema ?? (subschemaConfig as Subschema)?.transformedSchema ?? targetSchema, skipTypeMerging, diff --git a/packages/delegate/src/mergeFields.ts b/packages/delegate/src/mergeFields.ts index 06b00059644..0073433794c 100644 --- a/packages/delegate/src/mergeFields.ts +++ b/packages/delegate/src/mergeFields.ts @@ -180,11 +180,12 @@ export function mergeFields( const resultMap: Map | any, SelectionSetNode> = new Map(); delegationMap.forEach((selectionSet: SelectionSetNode, s: Subschema) => { const resolver = mergedTypeInfo.resolvers.get(s); - const maybePromise = resolver(object, context, info, s, selectionSet); - resultMap.set(maybePromise, selectionSet); + let maybePromise = resolver(object, context, info, s, selectionSet); if (isPromise(maybePromise)) { containsPromises = true; + maybePromise = maybePromise.then(undefined, error => error); } + resultMap.set(maybePromise, selectionSet); }); return containsPromises diff --git a/packages/delegate/src/transforms/CheckResultAndHandleErrors.ts b/packages/delegate/src/transforms/CheckResultAndHandleErrors.ts index c9582dc5962..264541de177 100644 --- a/packages/delegate/src/transforms/CheckResultAndHandleErrors.ts +++ b/packages/delegate/src/transforms/CheckResultAndHandleErrors.ts @@ -27,7 +27,8 @@ export default class CheckResultAndHandleErrors implements Transform { delegationContext.fieldName, delegationContext.subschema, delegationContext.returnType, - delegationContext.skipTypeMerging + delegationContext.skipTypeMerging, + delegationContext.onLocatedError ); } } @@ -39,12 +40,14 @@ export function checkResultAndHandleErrors( responseKey: string = getResponseKeyFromInfo(info), subschema?: GraphQLSchema | SubschemaConfig, returnType: GraphQLOutputType = info.returnType, - skipTypeMerging?: boolean + skipTypeMerging?: boolean, + onLocatedError?: (originalError: GraphQLError) => GraphQLError ): any { const { data, unpathedErrors } = mergeDataAndErrors( result.data == null ? undefined : result.data[responseKey], result.errors == null ? [] : result.errors, - info ? responsePathAsArray(info.path) : undefined + info ? responsePathAsArray(info.path) : undefined, + onLocatedError ); return resolveExternalValue(data, unpathedErrors, subschema, context, info, returnType, skipTypeMerging); @@ -54,6 +57,7 @@ export function mergeDataAndErrors( data: any, errors: ReadonlyArray, path: Array, + onLocatedError: (originalError: GraphQLError) => GraphQLError, index = 1 ): { data: any; unpathedErrors: Array } { if (data == null) { @@ -62,13 +66,16 @@ export function mergeDataAndErrors( } if (errors.length === 1) { - const error = errors[0]; + const error = onLocatedError ? onLocatedError(errors[0]) : errors[0]; const newPath = path === undefined ? error.path : error.path === undefined ? path : path.concat(error.path.slice(1)); + return { data: relocatedError(errors[0], newPath), unpathedErrors: [] }; } - return { data: locatedError(new AggregateError(errors), undefined, path), unpathedErrors: [] }; + const newError = locatedError(new AggregateError(errors), undefined, path); + + return { data: newError, unpathedErrors: [] }; } if (!errors.length) { @@ -98,6 +105,7 @@ export function mergeDataAndErrors( data[pathSegment], errorMap[pathSegment], path, + onLocatedError, index + 1 ); data[pathSegment] = newData; diff --git a/packages/delegate/src/types.ts b/packages/delegate/src/types.ts index 768a2ef2520..ad12fcdd0a1 100644 --- a/packages/delegate/src/types.ts +++ b/packages/delegate/src/types.ts @@ -51,6 +51,7 @@ export interface DelegationContext { context: Record; info: GraphQLResolveInfo; returnType: GraphQLOutputType; + onLocatedError?: (originalError: GraphQLError) => GraphQLError; transforms: Array; transformedSchema: GraphQLSchema; skipTypeMerging: boolean; @@ -64,6 +65,7 @@ export interface IDelegateToSchemaOptions, TArgs operation?: OperationTypeNode; fieldName?: string; returnType?: GraphQLOutputType; + onLocatedError?: (originalError: GraphQLError) => GraphQLError; args?: TArgs; selectionSet?: SelectionSetNode; fieldNodes?: ReadonlyArray; diff --git a/packages/stitch/tests/mergeFailures.test.ts b/packages/stitch/tests/mergeFailures.test.ts index fe1a345c022..78d12648a98 100644 --- a/packages/stitch/tests/mergeFailures.test.ts +++ b/packages/stitch/tests/mergeFailures.test.ts @@ -85,6 +85,64 @@ describe('merge failures', () => { expect(result).toEqual(expectedResult); }); + test('proxies merged error arrays', async () => { + const schema1 = makeExecutableSchema({ + typeDefs: ` + type Thing { + id: ID! + name: String + desc: String + } + type Query { + things(ids: [ID!]!): [Thing]! + } + `, + resolvers: { + Query: { + things: () => [new Error('no thing')], + }, + }, + }); + + const schema2 = makeExecutableSchema({ + typeDefs: ` + type ParentThing { + thing: Thing + } + type Thing { + id: ID! + } + type Query { + parent: ParentThing + } + `, + resolvers: { + Query: { + parent: () => ({ thing: { id: 23 } }), + }, + }, + }); + + const stitchedSchema = stitchSchemas({ + subschemas: [{ + schema: schema1, + merge: { + Thing: { + selectionSet: '{ id }', + fieldName: 'things', + key: ({ id }) => id, + argsFromKeys: (ids) => ({ ids }), + }, + } + }, { + schema: schema2, + }], + }); + + const stitchedResult = await graphql(stitchedSchema, '{ parent { thing { name desc id } } }'); + expect(stitchedResult.errors[0].path).toEqual(['parent', 'thing', 'name']); + }); + it('proxies inappropriate null', async () => { const secondSchema = makeExecutableSchema({ typeDefs: `