Don't treat entity interface __typename refinements as useless

.changeset/bright-mails-travel.md

@@ -0,0 +1,14 @@
+---
+"@apollo/query-planner": patch
+"@apollo/subgraph": patch
+"@apollo/gateway": patch
+---
+
+Fix specific case for requesting __typename on interface entity type
+
+In certain cases, when resolving a __typename on an interface entity (due to it actual being requested in the operation), that fetch group could previously be trimmed / treated as useless. At a glance, it appears to be a redundant step, i.e.:
+```
+{ ... on Product { __typename id }} => { ... on Product { __typename} }
+```
+It's actually necessary to preserve this in the case that we're coming from an interface object to an (entity) interface so that we can resolve the concrete __typename correctly.
+

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

@@ -3636,7 +3636,6 @@ describe('executeQueryPlan', () => {
         resolvers: {
           ChildItem: {
             __resolveReference(ref: { id: string, name: string, parentItem: { name: string }}) {
-              console.log(ref);
               return {
                 ...ref,
                 message: `${ref.parentItem.name} | ${ref.name}`,
@@ -5549,6 +5548,184 @@ describe('executeQueryPlan', () => {
         }
       `);
     });
+
+    test('handles resolving concrete `__typename`s of an @interfaceObject via the interface itself (issue #2743)', async () => {
+      const iList = [
+        {
+          id: '1',
+          tField: 'field on a T'
+        },
+      ];
+      const S1 = {
+        name: 'S1',
+        typeDefs: gql`
+          extend schema
+            @link(
+              url: "https://specs.apollo.dev/federation/v2.3"
+              import: ["@key", "@interfaceObject"]
+            )
+
+          interface I @key(fields: "id") {
+            id: ID!
+          }
+
+          type T implements I @key(fields: "id") {
+            id: ID!
+            tField: String
+          }
+        `,
+        resolvers: {
+          I: {
+            __resolveReference: ({ id }: { id: string }) => {
+              return iList.find((e) => e.id === id);
+            },
+            __resolveType: () => {
+              return 'T';
+            },
+          },
+          Query: {
+            allI: () => iList,
+          },
+        },
+      };
+
+      const S2 = {
+        name: 'S2',
+        typeDefs: gql`
+          extend schema
+            @link(
+              url: "https://specs.apollo.dev/federation/v2.3"
+              import: ["@key", "@interfaceObject"]
+            )
+
+          type I @key(fields: "id") @interfaceObject {
+            id: ID!
+            iField: String
+          }
+        `,
+        resolvers: {
+          I: {
+            __resolveReference: ({ id }: { id: string }) =>
+              iList.find((e) => e.id === id),
+            iField: () => "field on I's",
+          },
+        },
+      };
+
+      const S3 = {
+        name: 'S3',
+        typeDefs: gql`
+          extend schema
+            @link(
+              url: "https://specs.apollo.dev/federation/v2.3"
+              import: ["@key", "@interfaceObject"]
+            )
+
+          type I @key(fields: "id") @interfaceObject {
+            id: ID!
+          }
+
+          type Query {
+            searchIs: [I!]!
+          }
+        `,
+        resolvers: {
+          I: {
+            __resolveReference: ({ id }: { id: string }) =>
+              iList.find((e) => e.id === id),
+          },
+          Query: {
+            searchIs: () => iList,
+          },
+        },
+      };
+
+      const { serviceMap, schema, queryPlanner } = getFederatedTestingSchema([
+        S1, S2, S3,
+      ]);
+      const operation = parseOp(
+        `#graphql
+          {
+            searchIs {
+              __typename
+              id
+              iField
+            }
+          }
+        `,
+        schema,
+      );
+
+      const queryPlan = buildPlan(operation, queryPlanner);
+      expect(queryPlan).toMatchInlineSnapshot(`
+        QueryPlan {
+          Sequence {
+            Fetch(service: "S3") {
+              {
+                searchIs {
+                  __typename
+                  id
+                }
+              }
+            },
+            Parallel {
+              Flatten(path: "searchIs.@") {
+                Fetch(service: "S1") {
+                  {
+                    ... on I {
+                      __typename
+                      id
+                    }
+                  } =>
+                  {
+                    ... on I {
+                      __typename
+                    }
+                  }
+                },
+              },
+              Flatten(path: "searchIs.@") {
+                Fetch(service: "S2") {
+                  {
+                    ... on I {
+                      __typename
+                      id
+                    }
+                  } =>
+                  {
+                    ... on I {
+                      iField
+                    }
+                  }
+                },
+              },
+            },
+          },
+        }
+      `);
+
+      const response = await executePlan(
+        queryPlan,
+        operation,
+        undefined,
+        schema,
+        serviceMap,
+      );
+
+      expect(response).toMatchInlineSnapshot(`
+        Object {
+          "data": Object {
+            "searchIs": Array [
+              Object {
+                "__typename": "T",
+                "iField": "field on I's",
+                "id": "1",
+              },
+            ],
+          },
+        }
+      `);
+    });
   });
 
   describe('fields with conflicting types needing aliasing', () => {

query-planner-js/src/buildPlan.ts

@@ -1226,9 +1226,39 @@ class FetchGroup {
       return undefined;
     }
 
+    // This condition is specific to the case where we're resolving the _concrete_
+    // `__typename` field of an interface when coming from an interfaceObject type.
+    // i.e. { ... on Product { __typename id }} => { ... on Product { __typename} }
+    // This is usually useless at a glance, but in this case we need to actually
+    // keep this since this is our only path to resolving the concrete `__typename`.
+    const isInterfaceTypeConditionOnInterfaceObject = (
+      selection: Selection
+    ): boolean => {
+      if (selection.kind === "FragmentSelection") {
+        const parentType = selection.element.typeCondition;
+        if (parentType && isInterfaceType(parentType)) {
+          // Lastly, we just need to check that we're coming from a subgraph
+          // that has the type as an interface object in its schema.
+          return this.parents().some((p) => {
+            const typeInParent = this.dependencyGraph.subgraphSchemas
+              .get(p.group.subgraphName)
+              ?.type(parentType.name);
+            return typeInParent && isInterfaceObjectType(typeInParent);
+          });
+        }
+      }
+      return false;
+    };
+
     const inputSelections = this.inputs.selectionSets().flatMap((s) => s.selections());
     // Checks that every selection is contained in the input selections.
     const isUseless = this.selection.selections().every((selection) => {
+      // If we're coming from an interfaceObject _to_ an interface, we're
+      // "resolving" the concrete type of the interface and don't want to treat
+      // this as useless.
+      if (isInterfaceTypeConditionOnInterfaceObject(selection)) {
+        return false;
+      }
       const conditionInSupergraph = conditionInSupergraphIfInterfaceObject(selection);
       if (!conditionInSupergraph) {
         // We're not in the @interfaceObject case described above. We just check that an input selection contains the

subgraph-js/src/types.ts

@@ -57,26 +57,41 @@ function isPromise<T>(value: PromiseOrValue<T>): value is Promise<T> {
   return typeof (value as {then?: unknown})?.then === 'function';
 }
 
-function addTypeNameToPossibleReturn<T>(
-  maybeObject: null | T,
+async function maybeAddTypeNameToPossibleReturn<T extends { __typename?: string }>(
+  maybeObject: PromiseOrValue<null | T>,
   typename: string,
-): null | (T & { __typename: string }) {
-  if (maybeObject !== null && typeof maybeObject === 'object') {
-    Object.defineProperty(maybeObject, '__typename', {
+): Promise<null | T> {
+  const objectOrNull = await maybeObject;
+  if (
+    objectOrNull !== null
+    && typeof objectOrNull === 'object'
+  ) {
+    // If the object already has a __typename assigned, we're "refining" the
+    // type from an interface to an interfaceObject.
+    if ('__typename' in objectOrNull && objectOrNull.__typename !== typename) {
+      // XXX There's a really interesting nuance here in this condition. At a
+      // first glance, it looks like the code here and below could be simplified
+      // to just:
+      // ```
+      // objectOrNull.__typename = typename;
+      // return objectOrNull;
+      // ```
+      // But in this case, something internal to `graphql-js` depends on the
+      // identity of the object returned here. If we mutate in this case, we end
+      // up with errors from `graphql-js`. This might be worth investigating at
+      // some point, but for now creating a new object solves the problem and
+      // doesn't create any new ones.
+      return {
+        ...objectOrNull,
+        __typename: typename,
+      };
+    }
+
+    Object.defineProperty(objectOrNull, '__typename', {
       value: typename,
     });
   }
-  return maybeObject as null | (T & { __typename: string });
-}
-
-function maybeAddTypeNameToPossibleReturn<T>(
-  maybeObject: PromiseOrValue<null | T>,
-  typename: string,
-): PromiseOrValue<null | (T & { __typename?: string })> {
-  if (isPromise(maybeObject)) {
-    return maybeObject.then((x: any) => addTypeNameToPossibleReturn(x, typename));
-  }
-  return addTypeNameToPossibleReturn(maybeObject, typename);
+  return objectOrNull;
 }
 
 /**