Move gateway post-processing errors into extensions (#2380)

This align with what the router does, and allow to include
post-processing error messages in the response without generating
errors we weren't before and thus avoids backward compatibility
headaches.

Fixes #2374.

.changeset/eleven-guests-battle.md

@@ -0,0 +1,10 @@
+---
+"@apollo/gateway": patch
+---
+
+Move gateway post-processing errors from `errors` into `extensions.valueCompletion` of the response
+
+[https://github.com/apollographql/federation/pull/2335](PR #2335) introduced a breaking change that broke existing usages with respect to nullability and gateway error handling. In response to [https://github.com/apollographql/federation/issues/2374](Issue #2374), we are reverting the breaking portion of this change by continuing to swallow post processing errors as the gateway did prior to v2.3.0. Instead, those errors will now be included on the `extensions.valueCompletion` object in the response object.
+
+Gateway v2.3.0 and v2.3.1 are both affected by this change in behavior.
+

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

@@ -1378,10 +1378,16 @@ describe('executeQueryPlan', () => {
       const response = await executePlan(queryPlan, operation, undefined, schema);
 
       expect(response.data?.vehicle).toEqual(null);
-      expect(response.errors).toMatchInlineSnapshot(`
-         Array [
-           [GraphQLError: Invalid __typename found for object at field Query.vehicle.],
-         ]
+      // This kind of error is only found by the post-processing of the gateway, and post-processing errors are currently not returned
+      // as normal errors. Instead they are return as `extension`. See discussion on #2374 for details.
+      expect(response.errors).toBeUndefined();
+      // This message should not include `Car` in it.
+      expect(response.extensions).toMatchInlineSnapshot(`
+        Object {
+          "valueCompletion": Array [
+            [GraphQLError: Invalid __typename found for object at field Query.vehicle.],
+          ],
+        }
       `);
     });
   });
@@ -5429,4 +5435,112 @@ describe('executeQueryPlan', () => {
       `);
     });
   });
+
+  it(`surface post-processing errors as extensions in the response`, async () => {
+    // This test is such that the first subgraph return some object with a key, but
+    // then the 2nd one is queried for additional field `x`, but the reference resolver
+    // returns `null`, so that response is ignored, and the data internally to the
+    // gateway after the plan execution is the object from the 1st subgraph with
+    // just the key but no value for `x` (so effectively, `x` is `null`).
+    // However, because `x` is non-nullable, the gateway has to propagate that null
+    // to the whole `t` object, and it generates an appropriate message.
+    // But, for backward compatibility reasonse, we don't want that error to surface
+    // as a normal graphQL error: instead, it should be send in the response as
+    // an "extension" (see #2374 for details). This is what is tested here.
+
+    const s1 = {
+      name: 'S1',
+      typeDefs: gql`
+        type Query {
+          t: T
+        }
+
+        type T @key(fields: "id") {
+          id: ID!
+        }
+      `,
+      resolvers: {
+        Query: {
+          t() {
+            return { "__typename": "T", "id": 0 };
+          }
+        }
+      }
+    }
+
+    const s2 = {
+      name: 'S2',
+      typeDefs: gql`
+        type T @key(fields: "id") {
+          id: ID!
+          x: Int!
+        }
+      `,
+      resolvers: {
+        T: {
+          __resolveReference() {
+            return null;
+          }
+        }
+      }
+    }
+
+    const { serviceMap, schema, queryPlanner} = getFederatedTestingSchema([ s1, s2 ]);
+    const operation = parseOp(`
+      {
+       t {
+         id
+         x
+       }
+      }
+      `, schema);
+
+    const queryPlan = buildPlan(operation, queryPlanner);
+    expect(queryPlan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Sequence {
+          Fetch(service: "S1") {
+            {
+              t {
+                __typename
+                id
+              }
+            }
+          },
+          Flatten(path: "t") {
+            Fetch(service: "S2") {
+              {
+                ... on T {
+                  __typename
+                  id
+                }
+              } =>
+              {
+                ... on T {
+                  x
+                }
+              }
+            },
+          },
+        },
+      }
+    `);
+    const response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
+    expect(response.data).toMatchInlineSnapshot(`
+      Object {
+        "t": null,
+      }
+    `);
+
+    // As described above, we should _not_ have a "normal" error ...
+    expect(response.errors).toBeUndefined();
+    // ... but we should still have a trace of the underlying problem in the extensions.
+    expect(response.extensions).toMatchInlineSnapshot(`
+      Object {
+        "valueCompletion": Array [
+          [GraphQLError: Cannot return null for non-nullable field T.x.],
+        ],
+      }
+    `);
+  });
 });

gateway-js/src/executeQueryPlan.ts

@@ -175,21 +175,18 @@ export async function executeQueryPlan(
           // internal data in a state that triggers additional post-processing errors, but that leads to 2 errors recorded
           // for the same problem and that is unexpected by clients. See https://github.com/apollographql/federation/issues/981
           // for additional context.
-          // If we had no errors during query plan execution, then we do ship any post-processing ones as there is little
-          // reason not to and it might genuinely help debugging (note that if subgraphs return no errors and we assume that
-          // subgraph do return graphQL valid responses, then our composition rules should guarantee no post-processing errors,
-          // so getting a post-processing error points to either 1) a bug in our code or in composition or 2) a subgraph not
-          // returning valid graphQL results, both of which are well worth surfacing (see [this comment for instance](https://github.com/apollographql/federation/pull/159#issuecomment-801132906))).
+          // If we had no errors during query plan execution, then we do ship any post-processing ones, but not as "normal"
+          // errors, as "extensions". The reason is that we used to completely ignore those post-processing errors, and as a
+          // result some users have been relying on not getting errors in some nullability related cases that post-processing
+          // cover, and switching to returning errors in those case is problematic. Putting these error messages in `extensions`
+          // is a compromise in that the errors are still part of the reponse, which may help users debug an issue, but are
+          // not "normal graphQL errors", so clients and tooling will mostly ignore them.
           //
-          // That said, note that this is still not perfect in the sense that if someone does get subgraph errors, then
-          // while postProcessingErrors may duplicate those, it may also contain additional unrelated errors (again, something
-          // like a subgraph returning non-grapqlQL valid data unknowingly), and we don't surface those. In a perfect worlds
-          // we've be able to filter the post-proessing errors that duplicate errors from subgraph and still ship anything that
-          // remains, but it's unclear how to do that at all (it migth be that checking the error path helps, but not sure
-          // that's fullproof).
+          // Note that this behavior is also what the router does (and in fact, the exact name of the `extensions` we use,
+          // "valueCompletion", comes from the router and we use it for alignment.
           if (errors.length === 0 && postProcessingErrors.length > 0) {
             span.setStatus({ code:SpanStatusCode.ERROR });
-            return { errors: postProcessingErrors, data };
+            return { extensions: { "valueCompletion":  postProcessingErrors }, data };
           }
         } catch (error) {
           span.setStatus({ code:SpanStatusCode.ERROR });