From c6d175b2c1de640d2156ba0b2c69bf7e8884d98f Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Thu, 18 Jul 2024 15:35:29 +0300 Subject: [PATCH] fix(federation): merge errors for shared root fields correctly (#6355) * fix(federation): merge errors for shared root fields correctly * Tests * aggregateerror message * Spread AggregateError in errors in ExecutionResult * Fix lint --------- Co-authored-by: enisdenjo --- .changeset/five-buttons-give.md | 8 + packages/executor/src/execution/execute.ts | 45 ++++- packages/federation/src/supergraph.ts | 25 ++- .../federation/test/error-handling.test.ts | 162 ++++++++++++++++++ 4 files changed, 232 insertions(+), 8 deletions(-) create mode 100644 .changeset/five-buttons-give.md create mode 100644 packages/federation/test/error-handling.test.ts diff --git a/.changeset/five-buttons-give.md b/.changeset/five-buttons-give.md new file mode 100644 index 00000000000..22f72249b69 --- /dev/null +++ b/.changeset/five-buttons-give.md @@ -0,0 +1,8 @@ +--- +'@graphql-tools/federation': patch +--- + +Handle errors coming from subgraphs correctly when a root field is shared by different subgraphs + +- If subgraph A returns an error for `Query.foo`, and subgraph B returns the data, ignore the error and keep it for null fields. +- If both subgraphs return errors, return them as `AggregateError` then return them to the gateway result. diff --git a/packages/executor/src/execution/execute.ts b/packages/executor/src/execution/execute.ts index f452a4ec75f..d641bf1e23d 100644 --- a/packages/executor/src/execution/execute.ts +++ b/packages/executor/src/execution/execute.ts @@ -324,7 +324,11 @@ function executeImpl( throw exeContext.signal.reason; } - exeContext.errors.push(error); + if (error.errors) { + exeContext.errors.push(...error.errors); + } else { + exeContext.errors.push(error); + } return buildResponse(null, exeContext.errors); }, ) @@ -691,6 +695,17 @@ function executeField( // Note: we don't rely on a `catch` method, but we do expect "thenable" // to take a second callback for the error case. return completed.then(undefined, rawError => { + if (rawError instanceof AggregateError) { + return new AggregateError( + rawError.errors.map(rawErrorItem => { + rawErrorItem = coerceError(rawErrorItem); + const error = locatedError(rawErrorItem, fieldNodes, pathToArray(path)); + const handledError = handleFieldError(error, returnType, errors); + filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + return handledError; + }), + ); + } rawError = coerceError(rawError); const error = locatedError(rawError, fieldNodes, pathToArray(path)); const handledError = handleFieldError(error, returnType, errors); @@ -700,6 +715,15 @@ function executeField( } return completed; } catch (rawError) { + if (rawError instanceof AggregateError) { + return new AggregateError( + rawError.errors.map(rawErrorItem => { + const coercedError = coerceError(rawErrorItem); + const error = locatedError(coercedError, fieldNodes, pathToArray(path)); + return handleFieldError(error, returnType, errors); + }), + ); + } const coercedError = coerceError(rawError); const error = locatedError(coercedError, fieldNodes, pathToArray(path)); const handledError = handleFieldError(error, returnType, errors); @@ -1615,16 +1639,25 @@ function mapSourceToResponse( async (payload: unknown) => ensureAsyncIterable(await executeImpl(buildPerEventExecutionContext(exeContext, payload))), (error: Error) => { - const wrappedError = createGraphQLError(error.message, { - originalError: error, - nodes: [exeContext.operation], - }); - throw wrappedError; + if (error instanceof AggregateError) { + throw new AggregateError( + error.errors.map(e => wrapError(e, exeContext.operation)), + error.message, + ); + } + throw wrapError(error, exeContext.operation); }, ), ); } +function wrapError(error: Error, operation: OperationDefinitionNode) { + return createGraphQLError(error.message, { + originalError: error, + nodes: [operation], + }); +} + function createSourceEventStreamImpl( exeContext: ExecutionContext, ): MaybePromise | SingularExecutionResult> { diff --git a/packages/federation/src/supergraph.ts b/packages/federation/src/supergraph.ts index 07fcf865e98..87d2ed15be4 100644 --- a/packages/federation/src/supergraph.ts +++ b/packages/federation/src/supergraph.ts @@ -1060,9 +1060,9 @@ export function getStitchingOptionsFromSupergraphSdl( return jobs[0]; } if (hasPromise) { - return Promise.all(jobs).then(results => mergeDeep(results, false, true, true)); + return Promise.all(jobs).then(results => mergeResults(results)); } - return mergeDeep(jobs); + return mergeResults(jobs); }, }; } @@ -1219,3 +1219,24 @@ const entitiesFieldDefinitionNode: FieldDefinitionNode = { }; const specifiedTypeNames = ['ID', 'String', 'Float', 'Int', 'Boolean', '_Any', '_Entity']; + +function mergeResults(results: unknown[]) { + const errors: Error[] = []; + const datas: unknown[] = []; + for (const result of results) { + if (result instanceof AggregateError) { + errors.push(...result.errors); + } else if (result instanceof Error) { + errors.push(result); + } else { + datas.push(result); + } + } + if (datas.length) { + return mergeDeep(datas, false, true, true); + } + if (errors.length) { + return new AggregateError(errors, errors.map(error => error.message).join(', \n')); + } + return null; +} diff --git a/packages/federation/test/error-handling.test.ts b/packages/federation/test/error-handling.test.ts new file mode 100644 index 00000000000..9a7445bf886 --- /dev/null +++ b/packages/federation/test/error-handling.test.ts @@ -0,0 +1,162 @@ +import { GraphQLSchema, parse } from 'graphql'; +import { IntrospectAndCompose, LocalGraphQLDataSource } from '@apollo/gateway'; +import { buildSubgraphSchema } from '@apollo/subgraph'; +import { createDefaultExecutor } from '@graphql-tools/delegate'; +import { normalizedExecutor } from '@graphql-tools/executor'; +import { isAsyncIterable } from '@graphql-tools/utils'; +import { getStitchedSchemaFromSupergraphSdl } from '../src/supergraph'; + +describe('Error handling', () => { + let aResult: any; + let bResult: any; + const subgraphA = buildSubgraphSchema({ + typeDefs: parse(/* GraphQL */ ` + type Query { + foo: Foo + } + + type Foo @key(fields: "id") { + id: ID! + bar: String + } + `), + resolvers: { + Query: { + foo() { + return aResult; + }, + }, + Foo: { + __resolveReference(root) { + return root; + }, + bar() { + return 'Bar'; + }, + }, + }, + }); + const subgraphB = buildSubgraphSchema({ + typeDefs: parse(/* GraphQL */ ` + type Query { + foo: Foo + } + + extend type Foo @key(fields: "id") { + id: ID! + baz: String + } + `), + resolvers: { + Query: { + foo() { + return bResult; + }, + }, + Foo: { + __resolveReference(root) { + return root; + }, + baz() { + return 'Baz'; + }, + }, + }, + }); + let supergraph: GraphQLSchema; + beforeAll(async () => { + const { supergraphSdl } = await new IntrospectAndCompose({ + subgraphs: [ + { + name: 'A', + url: 'http://localhost:4001/graphql', + }, + { + name: 'B', + url: 'http://localhost:4002/graphql', + }, + ], + }).initialize({ + getDataSource({ name }) { + if (name === 'A') { + return new LocalGraphQLDataSource(subgraphA); + } + if (name === 'B') { + return new LocalGraphQLDataSource(subgraphB); + } + throw new Error(`Unknown subgraph: ${name}`); + }, + async healthCheck() {}, + update() {}, + }); + supergraph = getStitchedSchemaFromSupergraphSdl({ + supergraphSdl, + onSubschemaConfig(subschemaConfig) { + if (subschemaConfig.name === 'A') { + subschemaConfig.executor = createDefaultExecutor(subgraphA); + } else if (subschemaConfig.name === 'B') { + subschemaConfig.executor = createDefaultExecutor(subgraphB); + } else { + throw new Error(`Unknown subgraph: ${subschemaConfig.name}`); + } + }, + }); + }); + it('chooses the successful result from shared root fields', async () => { + aResult = new Error('A failed'); + bResult = { id: '1' }; + const result = await normalizedExecutor({ + schema: supergraph, + document: parse(/* GraphQL */ ` + query { + foo { + id + bar + baz + } + } + `), + }); + if (isAsyncIterable(result)) { + throw new Error('Expected result to be an ExecutionResult'); + } + expect(result.errors).toBeUndefined(); + expect(result.data).toEqual({ + foo: { + id: '1', + bar: null, + baz: 'Baz', + }, + }); + }); + it('merges errors from shared root fields', async () => { + aResult = new Error('A failed'); + bResult = new Error('B failed'); + const result = await normalizedExecutor({ + schema: supergraph, + document: parse(/* GraphQL */ ` + query { + foo { + id + bar + baz + } + } + `), + }); + if (isAsyncIterable(result)) { + throw new Error('Expected result to be an ExecutionResult'); + } + expect(result.errors).toHaveLength(2); + expect(result.errors).toContainEqual( + expect.objectContaining({ + message: 'A failed', + }), + ); + expect(result.errors).toContainEqual( + expect.objectContaining({ + message: 'B failed', + }), + ); + }); +});