diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f3f52690d2..be83dbb8c92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The version headers in this history reflect the versions of Apollo Server itself - `apollo-server-core`: Upgrade TS to 3.7.3 [#3618](https://github.com/apollographql/apollo-server/pull/3618) - `apollo-server-cloud-functions`: Transmit CORS headers on `OPTIONS` request. [PR #3557](https://github.com/apollographql/apollo-server/pull/3557) - `apollo-server-caching`: De-compose options interface for `KeyValueCache.prototype.set` to accommodate better TSDoc annotations for its properties (e.g. to specify that `ttl` is defined in _seconds_). [PR #3619](https://github.com/apollographql/apollo-server/pull/3619) +- `apollo-engine-reporting`: Fix regression introduced by [#3614](https://github.com/apollographql/apollo-server/pull/3614) which caused `PersistedQueryNotFoundError`, `PersistedQueryNotSupportedError` and `InvalidGraphQLRequestError` errors to be triggered before the `requestDidStart` handler triggered `treeBuilder`'s `startTiming` method. This fix preserves the existing behavior by special-casing these specific errors. [PR #3638](https://github.com/apollographql/apollo-server/pull/3638) [Issue #3627](https://github.com/apollographql/apollo-server/issues/3627) ### v2.9.14 diff --git a/package-lock.json b/package-lock.json index d7626102b93..dada7c3d869 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4057,6 +4057,7 @@ "apollo-graphql": "^0.3.4", "apollo-server-caching": "file:packages/apollo-server-caching", "apollo-server-env": "file:packages/apollo-server-env", + "apollo-server-errors": "file:packages/apollo-server-errors", "apollo-server-types": "file:packages/apollo-server-types", "async-retry": "^1.2.1", "graphql-extensions": "file:packages/graphql-extensions" diff --git a/packages/apollo-engine-reporting/package.json b/packages/apollo-engine-reporting/package.json index 219cdc39aee..38bc5c023f7 100644 --- a/packages/apollo-engine-reporting/package.json +++ b/packages/apollo-engine-reporting/package.json @@ -15,6 +15,7 @@ "apollo-graphql": "^0.3.4", "apollo-server-caching": "file:../apollo-server-caching", "apollo-server-env": "file:../apollo-server-env", + "apollo-server-errors": "file:../apollo-server-errors", "apollo-server-types": "file:../apollo-server-types", "async-retry": "^1.2.1", "graphql-extensions": "file:../graphql-extensions" diff --git a/packages/apollo-engine-reporting/src/treeBuilder.ts b/packages/apollo-engine-reporting/src/treeBuilder.ts index cd4b2341196..c6c40742c2f 100644 --- a/packages/apollo-engine-reporting/src/treeBuilder.ts +++ b/packages/apollo-engine-reporting/src/treeBuilder.ts @@ -5,6 +5,11 @@ import { responsePathAsArray, } from 'graphql'; import { Trace, google } from 'apollo-engine-reporting-protobuf'; +import { + PersistedQueryNotFoundError, + PersistedQueryNotSupportedError, +} from 'apollo-server-errors'; +import { InvalidGraphQLRequestError } from "apollo-server-types"; function internalError(message: string) { return new Error(`[internal apollo-server error] ${message}`); @@ -77,6 +82,12 @@ export class EngineReportingTreeBuilder { public didEncounterErrors(errors: GraphQLError[]) { errors.forEach(err => { + if (err instanceof PersistedQueryNotFoundError || + err instanceof PersistedQueryNotSupportedError || + err instanceof InvalidGraphQLRequestError) { + return; + } + // This is an error from a federated service. We will already be reporting // it in the nested Trace in the query plan. // diff --git a/packages/apollo-engine-reporting/tsconfig.json b/packages/apollo-engine-reporting/tsconfig.json index 7ee857f33a1..ca382f1094c 100644 --- a/packages/apollo-engine-reporting/tsconfig.json +++ b/packages/apollo-engine-reporting/tsconfig.json @@ -8,6 +8,7 @@ "exclude": ["**/__tests__", "**/__mocks__"], "references": [ { "path": "../graphql-extensions" }, + { "path": "../apollo-server-errors" }, { "path": "../apollo-server-types" }, ] } diff --git a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts index c32e25021f1..d642834e80f 100644 --- a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts +++ b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts @@ -1084,6 +1084,55 @@ export function testApolloServer( ); }); + it("doesn't internal server error on an APQ", async () => { + await setupApolloServerAndFetchPair(); + + const TEST_STRING_QUERY = ` + { onlyForThisApqTest${ + Math.random() + .toString() + .split('.')[1] + }: justAField } + `; + const hash = sha256 + .create() + .update(TEST_STRING_QUERY) + .hex(); + + const result = await apolloFetch({ + // @ts-ignore The `ApolloFetch` types don't allow `extensions` to be + // passed in, in the same way as `variables`, with a request. This + // is a typing omission in `apollo-fetch`, as can be seen here: + // https://git.io/Jeb63 This will all be going away soon (and + // that package is already archived and deprecated. + extensions: { + persistedQuery: { + version: VERSION, + sha256Hash: hash, + } + }, + }); + + // Having a persisted query not found error is fine. + expect(result.errors).toContainEqual( + expect.objectContaining({ + extensions: expect.objectContaining({ + code: "PERSISTED_QUERY_NOT_FOUND", + }) + }) + ); + + // However, having an internal server error is not okay! + expect(result.errors).not.toContainEqual( + expect.objectContaining({ + extensions: expect.objectContaining({ + code: "INTERNAL_SERVER_ERROR", + }) + }) + ); + + }); + describe('error munging', () => { describe('rewriteError', () => { it('new error', async () => {