Skip to content

Commit dcbbc34

Browse files
abernixabernix
authored
Invoke didEncounterErrors for known errors during pre-parse. (#3614)
* tests: Assert `didEncounterErrors` is invoked for execution errors. Previously a missing test. * tests: Assert error conditions for existing APQ logic (prior to improvements). Previously, a number of APQ errors did not have tests for errors which could arise at runtime. These include: - When the APQ `version` is not a version that we support. (Currently, the only supported `version` is `1`. - When the APQ `version` is entirely omitted. (It must be `1`.) - When the provided APQ `sha256Hash` doesn't match the provided `query` during APQ registration. I'm adding these tests which assert the existing logic without making any changes to the logic itself prior to making a number of changes to said logic which will ensure that these (again, existing) errors are _also_ propagated to the `didEncounterErrors` hook (They currently are NOT!). * Invoke `didEncounterErrors` for errors during pre-parse phases. Roughly, these "pre-parse" errors come in two forms: When the query was entirely omitted, or APQ (automated persisted query) errors. Arguably, there is a third form of pre-parse errors, but it would be unexpected and potentially out of scope of the `didEncounterErrors` life-cycle hook. Such an error would still be caught by `runHttpQuery.ts`, but we wouldn't have a guarantee that our plugins have been initialized. This is unlikely to improve until we re-work the entirety of the request pipeline in Apollo Server 3.x. While these errors were in-fact causing errors at runtime that were being delivered to the client, those errors were not being delivered to the plugin's `didEncounterErrors` hooks, causing reduced visibility by reporting tools which utilize that life-cycle hook. This commit changes the parent class of `InvaidGraphQLRequestError` from `Error` to `GraphQLError` in order to allow such an error to be received by `didEncounterErrors`, which requires `GraphQLError`s. The `GraphQLError` itself is still an `instanceof Error`, so any existing code that leverages such a condition should still work as expected, and I suspect that most other conditions that could be checked on either an `Error` or `GraphQLError` should also still remain the same. Despite any uncertain here, I'd call this a net-win for reporting errors more reliably to the hook on which they're expected to be received. * Add CHANGELOG.md for #3614.
1 parent 4c41e80 commit dcbbc34

5 files changed

Lines changed: 252 additions & 32 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ The version headers in this history reflect the versions of Apollo Server itself
66

77
> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the the appropriate changes within that release will be moved into the new section.
88
9+
- `apollo-server-core`: Ensure that plugin's `didEncounterErrors` hooks are invoked for known automated persisted query (APQ) errors. [#3614](https://github.com/apollographql/apollo-server/pull/3614)
910
- `apollo-server-plugin-base`: Move `TContext` generic from `requestDidStart` method to `ApolloServerPlugin` Interface. [#3525](https://github.com/apollographql/apollo-server/pull/3525)
1011

1112
### v2.9.13

‎packages/apollo-server-core/src/__tests__/runQuery.test.ts‎

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -514,19 +514,19 @@ describe('runQuery', () => {
514514

515515
describe('didEncounterErrors', () => {
516516
const didEncounterErrors = jest.fn();
517+
const plugins: ApolloServerPlugin[] = [
518+
{
519+
requestDidStart() {
520+
return { didEncounterErrors };
521+
},
522+
},
523+
];
524+
517525
it('called when an error occurs', async () => {
518526
await runQuery({
519527
schema,
520528
queryString: '{ testStringWithParseError: }',
521-
plugins: [
522-
{
523-
requestDidStart() {
524-
return {
525-
didEncounterErrors,
526-
};
527-
},
528-
},
529-
],
529+
plugins,
530530
request: new MockReq(),
531531
});
532532

@@ -537,19 +537,32 @@ describe('runQuery', () => {
537537
);
538538
});
539539

540+
it('called when an error occurs in execution', async () => {
541+
const response = await runQuery({
542+
schema,
543+
queryString: '{ testError }',
544+
plugins,
545+
request: new MockReq(),
546+
});
547+
548+
expect(response).toHaveProperty(
549+
'errors.0.message','Secret error message');
550+
expect(response).toHaveProperty('data.testError', null);
551+
552+
expect(didEncounterErrors).toBeCalledWith(
553+
expect.objectContaining({
554+
errors: expect.arrayContaining([expect.objectContaining({
555+
message: 'Secret error message',
556+
})]),
557+
}),
558+
);
559+
});
560+
540561
it('not called when an error does not occur', async () => {
541562
await runQuery({
542563
schema,
543564
queryString: '{ testString }',
544-
plugins: [
545-
{
546-
requestDidStart() {
547-
return {
548-
didEncounterErrors,
549-
};
550-
},
551-
},
552-
],
565+
plugins,
553566
request: new MockReq(),
554567
});
555568

‎packages/apollo-server-core/src/requestPipeline.ts‎

Lines changed: 36 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -135,11 +135,10 @@ export async function processGraphQLRequest<TContext>(
135135
// It looks like we've received a persisted query. Check if we
136136
// support them.
137137
if (!config.persistedQueries || !config.persistedQueries.cache) {
138-
throw new PersistedQueryNotSupportedError();
138+
return await emitErrorAndThrow(new PersistedQueryNotSupportedError());
139139
} else if (extensions.persistedQuery.version !== 1) {
140-
throw new InvalidGraphQLRequestError(
141-
'Unsupported persisted query version',
142-
);
140+
return await emitErrorAndThrow(
141+
new InvalidGraphQLRequestError('Unsupported persisted query version'));
143142
}
144143

145144
// We'll store a reference to the persisted query cache so we can actually
@@ -164,15 +163,14 @@ export async function processGraphQLRequest<TContext>(
164163
if (query) {
165164
metrics.persistedQueryHit = true;
166165
} else {
167-
throw new PersistedQueryNotFoundError();
166+
return await emitErrorAndThrow(new PersistedQueryNotFoundError());
168167
}
169168
} else {
170169
const computedQueryHash = computeQueryHash(query);
171170

172171
if (queryHash !== computedQueryHash) {
173-
throw new InvalidGraphQLRequestError(
174-
'provided sha does not match query',
175-
);
172+
return await emitErrorAndThrow(
173+
new InvalidGraphQLRequestError('provided sha does not match query'));
176174
}
177175

178176
// We won't write to the persisted query cache until later.
@@ -186,7 +184,8 @@ export async function processGraphQLRequest<TContext>(
186184
// now, but this should be replaced with the new operation ID algorithm.
187185
queryHash = computeQueryHash(query);
188186
} else {
189-
throw new InvalidGraphQLRequestError('Must provide query string.');
187+
return await emitErrorAndThrow(
188+
new InvalidGraphQLRequestError('Must provide query string.'));
190189
}
191190

192191
requestContext.queryHash = queryHash;
@@ -313,6 +312,14 @@ export async function processGraphQLRequest<TContext>(
313312
// for the time-being this just maintains existing behavior for what
314313
// happens when `throw`-ing an `HttpQueryError` in `didResolveOperation`.
315314
if (err instanceof HttpQueryError) {
315+
// In order to report this error reliably to the request pipeline, we'll
316+
// have to regenerate it with the original error message and stack for
317+
// the purposes of the `didEncounterErrors` life-cycle hook (which
318+
// expects `GraphQLError`s), but still throw the `HttpQueryError`, so
319+
// the appropriate status code is enforced by `runHttpQuery.ts`.
320+
const graphqlError = new GraphQLError(err.message);
321+
graphqlError.stack = err.stack;
322+
await didEncounterErrors([graphqlError]);
316323
throw err;
317324
}
318325
return await sendErrorResponse(err);
@@ -493,6 +500,26 @@ export async function processGraphQLRequest<TContext>(
493500
return requestContext.response!;
494501
}
495502

503+
/**
504+
* Report an error via `didEncounterErrors` and then `throw` it.
505+
*
506+
* Prior to the introduction of this function, some errors were being thrown
507+
* within the request pipeline and going directly to handling within
508+
* the `runHttpQuery.ts` module, rather than first being reported to the
509+
* plugin API's `didEncounterErrors` life-cycle hook (where they are to be
510+
* expected!).
511+
*
512+
* @param error The error to report to the request pipeline plugins prior
513+
* to being thrown.
514+
*
515+
* @throws
516+
*
517+
*/
518+
async function emitErrorAndThrow(error: GraphQLError): Promise<never> {
519+
await didEncounterErrors([error]);
520+
throw error;
521+
}
522+
496523
async function didEncounterErrors(errors: ReadonlyArray<GraphQLError>) {
497524
requestContext.errors = errors;
498525
extensionStack.didEncounterErrors(errors);

0 commit comments

Comments
 (0)