Merge branch 'release-gateway-0.38.1'

gateway-js/CHANGELOG.md

@@ -6,6 +6,10 @@
 
 - Introduce @core v0.2 support with the "for:" directive argument. The "for:" argument allows a @core directive to specify its criticality to the gateway (or any consumer). "for:" is optional - its absence means that the directive requires no additional support from the consumer. Its two available options `EXECUTION` and `SECURITY` both require explicit support from the consumer, else the consumer should fail to start / update to this unsupported schema. [PR #957](https://github.com/apollographql/federation/pull/942)
 
+## v0.38.1
+
+- Reverts [PR #159](https://github.com/apollographql/federation/pull/159) which propogated subgraph execution errors directly to the client.  While desirable in practice, this somewhat recent introduction would seem to beg for a different implementation, given that the pain points of introducing it seem to be currently outweighing the gains.  Happy to revisit this with additional feedback on [the tracking issue](https://github.com/apollographql/federation/issues/981) that has been opened to re-visit this.  In the interim, we are offering a release that reverts this change. [Issue #974](https://github.com/apollographql/federation/issues/974) [Apollo Server Issue #5550](https://github.com/apollographql/apollo-server/issues/5550) [PR #982](https://github.com/apollographql/federation/pull/982)
+
 ## v0.37.0
 
 - OpenTelemetry will now include the GraphQL `operationName` in span attributes, following up on the initial implementation introduced in v0.31.0 via [#836](https://github.com/apollographql/federation/pull/836) [PR #942](https://github.com/apollographql/federation/pull/942)

gateway-js/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@apollo/gateway",
-  "version": "0.38.0",
+  "version": "0.38.1",
   "description": "Apollo Gateway",
   "author": "Apollo <opensource@apollographql.com>",
   "main": "dist/index.js",

gateway-js/src/__tests__/executeQueryPlan.test.ts

@@ -1322,6 +1322,23 @@ describe('executeQueryPlan', () => {
       `);
     });
 
+    // THIS TEST SHOULD BE MODIFIED AFTER THE ISSUE OUTLINED IN
+    // https://github.com/apollographql/federation/issues/981 HAS BEEN RESOLVED.
+    // IT IS BEING LEFT HERE AS A TEST THAT WILL INTENTIONALLY FAIL WHEN
+    // IT IS RESOLVED IF IT'S NOT ADDRESSED.
+    //
+    // This test became relevant after a combination of two things:
+    //   1. when the gateway started surfacing errors from subgraphs happened in
+    //      https://github.com/apollographql/federation/pull/159
+    //   2. the idea of field redaction became necessary after
+    //      https://github.com/apollographql/federation/pull/893,
+    //      which introduced the notion of inaccessible fields.
+    //      The redaction started in
+    //      https://github.com/apollographql/federation/issues/974, which added
+    //      the following test.
+    //
+    // However, the error surfacing (first, above) needed to be reverted, thus
+    // de-necessitating this redaction logic which is no longer tested.
     it(`doesn't leak @inaccessible typenames in error messages`, async () => {
       const operationString = `#graphql
         query {
@@ -1353,11 +1370,13 @@ describe('executeQueryPlan', () => {
       );
 
       expect(response.data?.vehicle).toEqual(null);
-      expect(response.errors).toMatchInlineSnapshot(`
-        Array [
-          [GraphQLError: Abstract type "Vehicle" was resolve to a type [inaccessible type] that does not exist inside schema.],
-        ]
-      `);
+      expect(response.errors).toBeUndefined();
+      // SEE COMMENT ABOVE THIS TEST.  SHOULD BE RE-ENABLED AFTER #981 IS FIXED!
+      // expect(response.errors).toMatchInlineSnapshot(`
+      //   Array [
+      //     [GraphQLError: Abstract type "Vehicle" was resolve to a type [inaccessible type] that does not exist inside schema.],
+      //   ]
+      // `);
     });
   });
 });

gateway-js/src/executeQueryPlan.ts

@@ -31,7 +31,6 @@ import { deepMerge } from './utilities/deepMerge';
 import { isNotNullOrUndefined } from './utilities/array';
 import { SpanStatusCode } from "@opentelemetry/api";
 import { OpenTelemetrySpanNames, tracer } from "./utilities/opentelemetry";
-import { cleanErrorOfInaccessibleNames } from './utilities/cleanErrorOfInaccessibleNames';
 
 export type ServiceMap = {
   [serviceName: string]: GraphQLDataSource;
@@ -53,6 +52,9 @@ export async function executeQueryPlan<TContext>(
   requestContext: GraphQLRequestContext<TContext>,
   operationContext: OperationContext,
 ): Promise<GraphQLExecutionResult> {
+
+  const logger = requestContext.logger || console;
+
   return tracer.startActiveSpan(OpenTelemetrySpanNames.EXECUTE, async span => {
     try {
       const errors: GraphQLError[] = [];
@@ -92,7 +94,7 @@ export async function executeQueryPlan<TContext>(
         // It is also used to allow execution of introspection queries though.
         try {
           const schema = toAPISchema(operationContext.schema);
-          const executionResult = await execute({
+          ({ data } = await execute({
             schema,
             document: {
               kind: Kind.DOCUMENT,
@@ -105,17 +107,38 @@ export async function executeQueryPlan<TContext>(
             variableValues: requestContext.request.variables,
             // See also `wrapSchemaWithAliasResolver` in `gateway-js/src/index.ts`.
             fieldResolver: defaultFieldResolverWithAliasSupport,
-          });
-          data = executionResult.data;
-          if (executionResult.errors?.length) {
-            const cleanedErrors = executionResult.errors.map((error) =>
-              cleanErrorOfInaccessibleNames(schema, error),
-            );
-            errors.push(...cleanedErrors);
-          }
+          }));
         } catch (error) {
           span.setStatus({ code:SpanStatusCode.ERROR });
-          return { errors: [error] };
+          if (error instanceof GraphQLError) {
+            return { errors: [error] };
+          } else if (error instanceof Error) {
+            return {
+              errors: [
+                new GraphQLError(
+                  error.message,
+                  undefined,
+                  undefined,
+                  undefined,
+                  undefined,
+                  error as Error,
+                )
+              ]
+            };
+          } else {
+            // The above cases should cover the known cases, but if we received
+            // something else in the `catch` — like an object or something, we
+            // may not want to merely return this to the client.
+            logger.error(
+              "Unexpected error during query plan execution: " + error);
+            return {
+              errors: [
+                new GraphQLError(
+                  "Unexpected error during query plan execution",
+                )
+              ]
+            };
+          }
         }
         finally {
           span.end()