Fix potential bug when an @interfaceObject has a @requires (#2524)

When an `@interfaceObject` type has a field with a `@requires` and the
query requests that field only for some specific implementations of the
corresponding interface, then the generated query plan was sometimes
invalid and could result in an invalid query to a subgraph (against a
subgraph that rely on `@apollo/subgraph`, this lead the subgraph to
produce an error message looking like `"The _entities resolver tried to
load an entity for type X, but no object or interface type of that name
was found in the schema"`).

The underlying reason is that the plan was mistakenly requesting the
required fields for any object of the interface (corresponding to the
`@interfaceObject` in question), instead of only requesting them only
for only the implementation types it needed to. Not only is that
inefficient in principle, but this lead to invalid fetches because the
"rewrite" logic used to fixup the `__typename` for `@interfaceObject`
under the hood was only rewriting the types of the implementation types
in question (only expecting those to need rewrite, which technically was
correct) but was mistakenly getting values for other implementation
types.

Author: Sylvain Lebresne

.changeset/spotty-monkeys-clean.md

@@ -0,0 +1,13 @@
+---
+"@apollo/query-planner": patch
+"@apollo/federation-internals": patch
+"@apollo/gateway": patch
+---
+
+Fix potential bug when an `@interfaceObject` type has a `@requires`. When an `@interfaceObject` type has a field with a
+`@requires` and the query requests that field only for some specific implementations of the corresponding interface,
+then the generated query plan was sometimes invalid and could result in an invalid query to a subgraph (against a
+subgraph that rely on `@apollo/subgraph`, this lead the subgraph to produce an error message looking like `"The
+_entities resolver tried to load an entity for type X, but no object or interface type of that name was found in the
+schema"`).
+  

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

@@ -4850,6 +4850,251 @@ describe('executeQueryPlan', () => {
         }
       `);
     });
+
+    test('handles @requires on @interfaceObject that applies to only one of the queried implementation', async () => {
+      // The case this test is that where the @interfaceObject in s2 has a @requires, but the query we send requests the field on which
+      // there is this @require only for one of the implementation type, which it request another field with no require for another implementation.
+      // And we're making sure the requirements only get queried for T1, the first type.
+      const s1 = {
+        name: 's1',
+        typeDefs: gql`
+          extend schema
+            @link(url: "https://specs.apollo.dev/federation/v2.3", import: ["@key"])
+
+          type Query {
+            is: [I!]!
+          }
+
+          interface I @key(fields: "id") {
+            id: ID!
+            name: String
+            req: Req
+          }
+
+          type T1 implements I @key(fields: "id") {
+            id: ID!
+            name: String
+            req: Req
+          }
+
+          type T2 implements I @key(fields: "id") {
+            id: ID!
+            name: String
+            req: Req
+          }
+
+          type Req {
+            id: ID!
+          }
+        `,
+        resolvers: {
+          Query: {
+            is: () => [
+              { __typename: 'T1', id: '2', name: 'e2', req: { id: 'r1'} },
+              { __typename: 'T2', id: '4', name: 'e4', req: { id: 'r2'} },
+              { __typename: 'T1', id: '1', name: 'e1', req: { id: 'r3'} }
+            ]
+          },
+        }
+      }
+
+      const s2 = {
+        name: 's2',
+        typeDefs: gql`
+          extend schema
+            @link(url: "https://specs.apollo.dev/federation/v2.3", import: ["@key", "@interfaceObject", "@external", "@requires"])
+
+          type I @key(fields: "id") @interfaceObject {
+            id: ID!
+            req: Req @external
+            v: String! @requires(fields: "req { id }")
+          }
+
+          type Req {
+            id: ID! @external
+          }
+        `,
+        resolvers: {
+          I: {
+            __resolveReference(ref: any) {
+              return {
+                ...ref,
+                v: `req=${ref.req.id}`
+              };
+            },
+          }
+        }
+      }
+
+      const { serviceMap, schema, queryPlanner} = getFederatedTestingSchema([ s1, s2 ]);
+
+      let operation = parseOp(`
+        {
+          is {
+            ... on T1 {
+              v
+            }
+            ... on T2 {
+              name
+            }
+          }
+        }
+        `, schema);
+
+      let queryPlan = buildPlan(operation, queryPlanner);
+      expect(queryPlan).toMatchInlineSnapshot(`
+        QueryPlan {
+          Sequence {
+            Fetch(service: "s1") {
+              {
+                is {
+                  __typename
+                  ... on T1 {
+                    __typename
+                    id
+                    req {
+                      id
+                    }
+                  }
+                  ... on T2 {
+                    name
+                  }
+                }
+              }
+            },
+            Flatten(path: "is.@") {
+              Fetch(service: "s2") {
+                {
+                  ... on T1 {
+                    __typename
+                    id
+                  }
+                  ... on I {
+                    __typename
+                    req {
+                      id
+                    }
+                  }
+                } =>
+                {
+                  ... on I {
+                    v
+                  }
+                }
+              },
+            },
+          },
+        }
+      `);
+
+      let response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
+      expect(response.errors).toBeUndefined();
+      expect(response.data).toMatchInlineSnapshot(`
+        Object {
+          "is": Array [
+            Object {
+              "v": "req=r1",
+            },
+            Object {
+              "name": "e4",
+            },
+            Object {
+              "v": "req=r3",
+            },
+          ],
+        }
+      `);
+
+      // Sanity checking that if we ask for `v` (the field with @requires), then everything still works.
+      operation = parseOp(`
+        {
+          is {
+            ... on T1 {
+              v
+            }
+            ... on T2 {
+              v
+              name
+            }
+          }
+        }
+        `, schema);
+
+      global.console = require('console');
+      queryPlan = buildPlan(operation, queryPlanner);
+      expect(queryPlan).toMatchInlineSnapshot(`
+        QueryPlan {
+          Sequence {
+            Fetch(service: "s1") {
+              {
+                is {
+                  __typename
+                  ... on T1 {
+                    __typename
+                    id
+                    req {
+                      id
+                    }
+                  }
+                  ... on T2 {
+                    __typename
+                    id
+                    req {
+                      id
+                    }
+                    name
+                  }
+                }
+              }
+            },
+            Flatten(path: "is.@") {
+              Fetch(service: "s2") {
+                {
+                  ... on T1 {
+                    __typename
+                    id
+                  }
+                  ... on I {
+                    __typename
+                    req {
+                      id
+                    }
+                  }
+                  ... on T2 {
+                    __typename
+                    id
+                  }
+                } =>
+                {
+                  ... on I {
+                    v
+                  }
+                }
+              },
+            },
+          },
+        }
+      `);
+
+      response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
+      expect(response.errors).toBeUndefined();
+      expect(response.data).toMatchInlineSnapshot(`
+        Object {
+          "is": Array [
+            Object {
+              "v": "req=r1",
+            },
+            Object {
+              "name": "e4",
+              "v": "req=r2",
+            },
+            Object {
+              "v": "req=r3",
+            },
+          ],
+        }
+      `);
+    });
   });
 
   describe('fields with conflicting types needing aliasing', () => {

internals-js/src/operations.ts

@@ -49,7 +49,7 @@ import {
 } from "./definitions";
 import { isInterfaceObjectType } from "./federation";
 import { ERRORS } from "./error";
-import { isDirectSubtype, sameType } from "./types";
+import { isDirectSubtype, isSubtype, sameType } from "./types";
 import { assert, mapEntries, MapWithCachedArrays, MultiMap, SetMultiMap } from "./utils";
 import { argumentsEquals, argumentsFromAST, isValidValue, valueToAST, valueToString } from "./values";
 
@@ -568,12 +568,12 @@ function isUselessFollowupElement(first: OperationElement, followup: OperationEl
     : first.typeCondition;
 
   // The followup is useless if it's a fragment (with no directives we would want to preserve) whose type
-  // is already that of the first element.
+  // is already that of the first element (or a supertype).
   return !!typeOfFirst
     && followup.kind === 'FragmentElement'
     && !!followup.typeCondition
     && (followup.appliedDirectives.length === 0 || isDirectiveApplicationsSubset(conditionals, followup.appliedDirectives))
-    && sameType(typeOfFirst, followup.typeCondition);
+    && isSubtype(followup.typeCondition, typeOfFirst);
 }
 
 export type RootOperationPath = {