From 5b8565a6908557af22eaead1d86c546d8d1e9163 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 30 Jul 2019 17:21:27 -0700 Subject: [PATCH 1/2] apollo-engine-reporting: don't fail if errors have non-array 'path' Specifically, we found that some network errors talking to federated backends could have a string path. --- CHANGELOG.md | 1 + packages/apollo-engine-reporting/src/treeBuilder.ts | 4 +++- packages/apollo-gateway/src/index.ts | 5 +++++ packages/apollo-server-core/src/requestPipeline.ts | 3 +++ 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 689362b1aa5..efcf51b3fef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ The version headers in this history reflect the versions of Apollo Server itself > 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. +- `apollo-engine-reporting`: Fix reporting errors which have non-array `path` fields (eg, non-GraphQLError errors). [PR #3112](https://github.com/apollographql/apollo-server/pull/3112) - `apollo-engine-reporting`: Add missing `apollo-server-caching` dependency. [PR #3054](https://github.com/apollographql/apollo-server/pull/3054) - `apollo-server-hapi`: Revert switch from `accept` and `boom` which took place in v2.8.0. [PR #3089](https://github.com/apollographql/apollo-server/pull/3089) - `@apollo/gateway`: Change the `setInterval` timer, which is used to continuously check for updates to a federated graph from the Apollo Graph Manager, to be an `unref`'d timer. Without this change, the server wouldn't terminate properly once polling had started since the event-loop would continue to have unprocessed events on it. [PR #3105](https://github.com/apollographql/apollo-server/pull/3105) diff --git a/packages/apollo-engine-reporting/src/treeBuilder.ts b/packages/apollo-engine-reporting/src/treeBuilder.ts index 2221231e7cf..155a7a036fa 100644 --- a/packages/apollo-engine-reporting/src/treeBuilder.ts +++ b/packages/apollo-engine-reporting/src/treeBuilder.ts @@ -117,7 +117,9 @@ export class EngineReportingTreeBuilder { // By default, put errors on the root node. let node = this.rootNode; - if (path) { + // If a non-GraphQLError Error sneaks in here somehow with a non-array + // path, don't crash. + if (path && Array.isArray(path)) { const specificNode = this.nodes.get(path.join('.')); if (specificNode) { node = specificNode; diff --git a/packages/apollo-gateway/src/index.ts b/packages/apollo-gateway/src/index.ts index c628f877f36..0846a9749a1 100644 --- a/packages/apollo-gateway/src/index.ts +++ b/packages/apollo-gateway/src/index.ts @@ -258,6 +258,11 @@ export class ApolloGateway implements GraphQLService { }); } + // XXX Nothing guarantees that the only errors thrown or returned in + // result.errors are GraphQLErrors, even though other code (eg + // apollo-engine-reporting) assumes that. In fact, errors talking to backends + // are unlikely to show up as GraphQLErrors. Do we need to use + // formatApolloErrors or something? public executor = async ( requestContext: WithRequired< GraphQLRequestContext, diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index d2d8a399769..96a16a75eff 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -456,6 +456,9 @@ export async function processGraphQLRequest( try { if (config.executor) { + // XXX Nothing guarantees that the only errors thrown or returned + // in result.errors are GraphQLErrors, even though other code + // (eg apollo-engine-reporting) assumes that. return await config.executor(requestContext); } else { return await graphql.execute(executionArgs); From e6ae7b7579c87374b53b909b836d960eac47446f Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Wed, 31 Jul 2019 13:19:10 +0300 Subject: [PATCH 2/2] Just use `Array.isArray`, which returns `false` when passed `undefined`. Nit, of course. --- packages/apollo-engine-reporting/src/treeBuilder.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apollo-engine-reporting/src/treeBuilder.ts b/packages/apollo-engine-reporting/src/treeBuilder.ts index 155a7a036fa..cd4b2341196 100644 --- a/packages/apollo-engine-reporting/src/treeBuilder.ts +++ b/packages/apollo-engine-reporting/src/treeBuilder.ts @@ -119,7 +119,7 @@ export class EngineReportingTreeBuilder { let node = this.rootNode; // If a non-GraphQLError Error sneaks in here somehow with a non-array // path, don't crash. - if (path && Array.isArray(path)) { + if (Array.isArray(path)) { const specificNode = this.nodes.get(path.join('.')); if (specificNode) { node = specificNode;