From e61eea43eefe7cfc93926bacfc613bed44d4b96b Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Wed, 22 May 2019 15:47:12 +0300 Subject: [PATCH 1/2] Call `didEncounterErrors` from `sendErrorResponse`. This re-implements the triggering of `didEncounterErrors` during `willResolveOperation` errors which occur in the request pipeline, but in a more common spot and where it probably should have been getting called already anyway. --- packages/apollo-server-core/src/requestPipeline.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index b5827492246..30d1ed17630 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -481,6 +481,8 @@ export async function processGraphQLRequest( ? errorOrErrors : [errorOrErrors]; + extensionStack.didEncounterErrors(errors); + return sendResponse({ errors: formatErrors( errors.map(err => From 5b37b361c13e029e1e28287889b9b1cd311784cb Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Wed, 22 May 2019 00:07:16 +0300 Subject: [PATCH 2/2] Call `sendErrorResponse` when errors occur within `didResolveOperation`. This aims to accomplish one of the bits which was included within https://github.com/apollographql/apollo-server/pull/2659 via a technique that scopes the errors more precisely. While I do think we should be passing all errors, the fix in #2659 was specifically aimed at a particular error-case which surfaced and we should consider a more refined approach to capturing those errors in the right places in future API iterations. --- .../apollo-server-core/src/requestPipeline.ts | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index 30d1ed17630..0249da325d8 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -61,6 +61,7 @@ export { }; import createSHA from './utils/createSHA'; +import { HttpQueryError } from './runHttpQuery'; export const APQ_CACHE_PREFIX = 'apq:'; @@ -290,13 +291,26 @@ export async function processGraphQLRequest( requestContext.operationName = (operation && operation.name && operation.name.value) || null; - await dispatcher.invokeHookAsync( - 'didResolveOperation', - requestContext as WithRequired< - typeof requestContext, - 'document' | 'source' | 'operation' | 'operationName' | 'metrics' - >, - ); + try { + await dispatcher.invokeHookAsync( + 'didResolveOperation', + requestContext as WithRequired< + typeof requestContext, + 'document' | 'source' | 'operation' | 'operationName' | 'metrics' + >, + ); + } catch (err) { + // XXX: The HttpQueryError is special-cased here because we currently + // depend on `throw`-ing an error from the `didResolveOperation` hook + // we've implemented in `runHttpQuery.ts`'s `checkOperationPlugin`: + // https://git.io/fj427. This could be perceived as a feature, but + // for the time-being this just maintains existing behavior for what + // happens when `throw`-ing an `HttpQueryError` in `didResolveOperation`. + if (err instanceof HttpQueryError) { + throw err; + } + return sendErrorResponse(err); + } // Now that we've gone through the pre-execution phases of the request // pipeline, and given plugins appropriate ability to object (by throwing