Skip to content

Commit

Permalink
Add comments about preservation of legacy behaviors in request pipeline.
Browse files Browse the repository at this point in the history
These comments hope to better explain the behavior which was present in the
2.x series of Apollo Server prior to the introduction of
`didEncounterErrors`.

The behavior which this commentary applies to -- in which we `throw` prior
to finishing subsequent errors, has been this way since AS2 was released and
was a pattern which we locked ourselves into for the duration of the AS2
release when we put out the request pipeline plugins.

It should _not_ stay this way in the future, and in AS3 we will resolve this.
  • Loading branch information
abernix committed May 20, 2020
1 parent 96dbccc commit cd4844e
Showing 1 changed file with 32 additions and 1 deletion.
33 changes: 32 additions & 1 deletion packages/apollo-server-core/src/requestPipeline.ts
Expand Up @@ -160,8 +160,16 @@ export async function processGraphQLRequest<TContext>(
// It looks like we've received a persisted query. Check if we
// support them.
if (!config.persistedQueries || !config.persistedQueries.cache) {
// We are returning to `runHttpQuery` to preserve legacy behavior while
// still delivering observability to the `didEncounterErrors` hook.
// This particular error will _not_ trigger `willSendResponse`.
// See comment on `emitErrorAndThrow` for more details.
return await emitErrorAndThrow(new PersistedQueryNotSupportedError());
} else if (extensions.persistedQuery.version !== 1) {
// We are returning to `runHttpQuery` to preserve legacy behavior while
// still delivering observability to the `didEncounterErrors` hook.
// This particular error will _not_ trigger `willSendResponse`.
// See comment on `emitErrorAndThrow` for more details.
return await emitErrorAndThrow(
new InvalidGraphQLRequestError('Unsupported persisted query version'));
}
Expand All @@ -188,12 +196,20 @@ export async function processGraphQLRequest<TContext>(
if (query) {
metrics.persistedQueryHit = true;
} else {
// We are returning to `runHttpQuery` to preserve legacy behavior while
// still delivering observability to the `didEncounterErrors` hook.
// This particular error will _not_ trigger `willSendResponse`.
// See comment on `emitErrorAndThrow` for more details.
return await emitErrorAndThrow(new PersistedQueryNotFoundError());
}
} else {
const computedQueryHash = computeQueryHash(query);

if (queryHash !== computedQueryHash) {
// We are returning to `runHttpQuery` to preserve legacy behavior while
// still delivering observability to the `didEncounterErrors` hook.
// This particular error will _not_ trigger `willSendResponse`.
// See comment on `emitErrorAndThrow` for more details.
return await emitErrorAndThrow(
new InvalidGraphQLRequestError('provided sha does not match query'));
}
Expand All @@ -209,6 +225,10 @@ export async function processGraphQLRequest<TContext>(
// now, but this should be replaced with the new operation ID algorithm.
queryHash = computeQueryHash(query);
} else {
// We are returning to `runHttpQuery` to preserve legacy behavior
// while still delivering observability to the `didEncounterErrors` hook.
// This particular error will _not_ trigger `willSendResponse`.
// See comment on `emitErrorAndThrow` for more details.
return await emitErrorAndThrow(
new InvalidGraphQLRequestError('Must provide query string.'));
}
Expand Down Expand Up @@ -359,6 +379,9 @@ export async function processGraphQLRequest<TContext>(
// an error) and not actually write, we'll write to the cache if it was
// determined earlier in the request pipeline that we should do so.
if (metrics.persistedQueryRegister && persistedQueryCache) {
// While it shouldn't normally be necessary to wrap this `Promise` in a
// `Promise.resolve` invocation, it seems that the underlying cache store
// is returning a non-native `Promise` (e.g. Bluebird, etc.).
Promise.resolve(
persistedQueryCache.set(
queryHash,
Expand Down Expand Up @@ -556,7 +579,15 @@ export async function processGraphQLRequest<TContext>(
}

/**
* Report an error via `didEncounterErrors` and then `throw` it.
* HEREIN LIE LEGACY COMPATIBILITY
*
* DO NOT PERPETUATE THE USE OF THIS METHOD IN NEWLY INTRODUCED CODE.
*
* Report an error via `didEncounterErrors` and then `throw` it again,
* ENTIRELY BYPASSING the rest of the request pipeline and returning
* control to `runHttpQuery.ts`.
*
* Any number of other life-cycle events may not be invoked in this case.
*
* Prior to the introduction of this function, some errors were being thrown
* within the request pipeline and going directly to handling within
Expand Down

0 comments on commit cd4844e

Please sign in to comment.