Fix removeInputsFromSelection to only remove inputs

.changeset/pink-ducks-fail.md

@@ -0,0 +1,6 @@
+---
+"@apollo/query-planner": patch
+---
+
+Fix query planning bug where keys or required fields can sometimes reach subgraphs with null values. ([#2805](https://github.com/apollographql/federation/issues/2805))
+

query-planner-js/src/__tests__/buildPlan.test.ts

@@ -3810,7 +3810,7 @@ describe('Fed1 supergraph handling', () => {
 });
 
 describe('Named fragments preservation', () => {
-  it('works with nested fragments', () => {
+  it('works with nested fragments 1', () => {
     const subgraph1 = {
       name: 'Subgraph1',
       typeDefs: gql`
@@ -7821,3 +7821,155 @@ test('avoid considering indirect paths from the root when a more direct one exis
     4,
   );
 });
+
+describe('@requires references external field indirectly', () => {
+  it('key where @external is not at top level of selection of requires', () => {
+    // Field issue where we were seeing a FetchGroup created where the fields used by the key to jump subgraphs
+    // were not properly fetched. In the below test, this test will ensure that 'k2' is properly collected
+    // before it is used
+    const subgraph1 = {
+      name: 'A',
+      typeDefs: gql`
+        type Query {
+          u: U!
+        }
+
+        type U @key(fields: "k1 { id }") {
+          k1: K
+        }
+
+        type K @key(fields: "id") {
+          id: ID!
+        }
+      `,
+    };
+    const subgraph2 = {
+      name: 'B',
+      typeDefs: gql`
+        type U @key(fields: "k1 { id }") @key(fields: "k2") {
+          k1: K!
+          k2: ID!
+          v: V! @external
+          f: ID! @requires(fields: "v { v }")
+          f2: Int!
+        }
+
+        type K @key(fields: "id") {
+          id: ID!
+        }
+
+        type V @key(fields: "id") {
+          id: ID!
+          v: String! @external
+        }
+      `,
+    };
+    const subgraph3 = {
+      name: 'C',
+      typeDefs: gql`
+        type U @key(fields: "k1 { id }") @key(fields: "k2") {
+          k1: K!
+          k2: ID!
+          v: V!
+        }
+
+        type K @key(fields: "id") {
+          id: ID!
+        }
+
+        type V @key(fields: "id") {
+          id: ID!
+          v: String!
+        }
+      `,
+    };
+
+    const [api, queryPlanner] = composeAndCreatePlanner(
+      subgraph1,
+      subgraph2,
+      subgraph3,
+    );
+    const operation = operationFromDocument(
+      api,
+      gql`
+        {
+          u {
+            f
+          }
+        }
+      `,
+    );
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    expect(plan).toMatchInlineSnapshot(`
+    QueryPlan {
+      Sequence {
+        Fetch(service: "A") {
+          {
+            u {
+              __typename
+              k1 {
+                id
+              }
+            }
+          }
+        },
+        Flatten(path: "u") {
+          Fetch(service: "B") {
+            {
+              ... on U {
+                __typename
+                k1 {
+                  id
+                }
+              }
+            } =>
+            {
+              ... on U {
+                k2
+              }
+            }
+          },
+        },
+        Flatten(path: "u") {
+          Fetch(service: "C") {
+            {
+              ... on U {
+                __typename
+                k2
+              }
+            } =>
+            {
+              ... on U {
+                v {
+                  v
+                }
+              }
+            }
+          },
+        },
+        Flatten(path: "u") {
+          Fetch(service: "B") {
+            {
+              ... on U {
+                __typename
+                v {
+                  v
+                }
+                k1 {
+                  id
+                }
+              }
+            } =>
+            {
+              ... on U {
+                f
+              }
+            }
+          },
+        },
+      },
+    }
+    `);
+  });
+});

query-planner-js/src/buildPlan.ts

@@ -944,7 +944,7 @@ class FetchGroup {
         // `inputs.contains(selection)`, so if a group is shown useful, it means that there
         // is some selections not in the inputs, but as long as we add to selections (and we
         // never remove from selections; `MutableSelectionSet` don't have removing methods),
-        // then this won't change. Only changing inputs may require some recomputation. 
+        // then this won't change. Only changing inputs may require some recomputation.
         this.isKnownUseful = false;
       }
     }
@@ -1188,7 +1188,7 @@ class FetchGroup {
     if (inputs) {
       this.cachedCost = undefined;
       const updated = inputs.selectionSets().reduce((prev, value) => prev.minus(value), this.selection);
-      this._selection = MutableSelectionSet.ofWithMemoized(this.selection.minus(updated), conditionsMemoizer);
+      this._selection = MutableSelectionSet.ofWithMemoized(updated, conditionsMemoizer);
     }
   }
 
@@ -1706,7 +1706,7 @@ function withFieldAliased(selectionSet: SelectionSet, aliases: FieldToAlias[]):
       const field = selection.element;
       const alias = pathElement && atCurrentLevel.get(pathElement);
       return !alias && selection.selectionSet === updatedSelectionSet
-        ? selection 
+        ? selection
         : selection.withUpdatedComponents(alias ? field.withUpdatedAlias(alias.alias) : field, updatedSelectionSet);
     } else {
       return selection.selectionSet === updatedSelectionSet
@@ -3701,8 +3701,8 @@ function addTypenameFieldForAbstractTypesInNamedFragments(fragments: NamedFragme
 }
 
 /**
- * Given a selection select (`selectionSet`) and given a set of directive applications that can be eliminated (`unneededDirectives`; in 
- * practice those are conditionals (@skip and @include) already accounted for), returns an equivalent selection set but with unecessary 
+ * Given a selection select (`selectionSet`) and given a set of directive applications that can be eliminated (`unneededDirectives`; in
+ * practice those are conditionals (@skip and @include) already accounted for), returns an equivalent selection set but with unecessary
  * "starting" fragments having the unneeded condition/directives removed.
  */
 function removeUnneededTopLevelFragmentDirectives(
@@ -3969,7 +3969,7 @@ function computeGroupsForTree(
             const inputSelections = newCompositeTypeSelectionSet(inputType);
             inputSelections.updates().add(edge.conditions!);
             newGroup.addInputs(
-              wrapInputsSelections(inputType, inputSelections.get(), newContext), 
+              wrapInputsSelections(inputType, inputSelections.get(), newContext),
               computeInputRewritesOnKeyFetch(inputType.name, destType),
             );