Fix removeInputsFromSelection to only remove inputs (#2859)

removeInputsFromSelection is tasked with removing inputs to a FetchGroup from the fetch group so that we don't retrieve the same values twice (and FetchGroups that don't functionally do anything can be removed). There was a bug in the code that meant that in certain cases, things that were required were actually being removed. This could manifest in subgraph jumps not being possible because the key it was using to make the jump was not available.
Fixes #2805

.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),
             );