Fix incorrect removal of dependencies between groups in some rare `@r…

…equires` (#2726)

Fulfilling some `@requires` can necessitate querying group(s)
(subgraphs) that are not the direct parent of the group with the
`@requires`. To generate plan that are as optimal as possible, the code
needs to figure out the proper parent of those new groups, that is
what is the "earliest" group those new groups truly depend on. This
is done by starting to look at the parent of the `@requires` group,
see if the new groups genuinely depend on anything that parent fetches,
and if not, iterate the same logic on the grand-parent (until we either
find a parent the new groups depend on, or we reach the beginning of
the plan.

But the code that checked if the new groups truly depend on a given
parent was incomplete: it was sometimes responding that there were
no dependency when that was not true.

The result is that for some specific configuration of a `@requires`,
we could end up with a plan whereby some group/fetch A depended on
the results of another group/fetch B, but A was performed in parallel
of B instead of after it. Which ultimately resulted in not getting
the proper data for the required fields, leading to incorrect executions
(the exact detail of how this would show up depends a bit on the exact
service implementation, but typically the resolver of the field on
which there is a `@requires` would get `null` for its requirements,
which might lead that resolver to throw an error as this is unexpected).

Fixes #2683

Co-authored-by: Chris Lenfest <clenfest@apollographql.com>

.changeset/strange-moons-prove.md

@@ -0,0 +1,15 @@
+---
+"@apollo/query-planner": major
+"@apollo/gateway": major
+---
+
+Fix some potentially incorrect query plans with `@requires` when some dependencies are involved.
+
+In some rare case of `@requires`, an over-eager optimisation was incorrectly considering that
+a dependency between 2 subgraph fetches was unnecessary, leading to doing 2 subgraphs queries
+in parallel when those should be done sequentially (because the 2nd query rely on results
+from the 1st one). This effectively resulted in the required fields not being provided (the
+consequence of which depends a bit on the resolver detail, but if the resolver expected
+the required fields to be populated (as they should), then this could typically result
+in a message of the form `GraphQLError: Cannot read properties of null`).
+

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

@@ -3545,6 +3545,371 @@ describe('executeQueryPlan', () => {
         }
       `);
     });
+
+    test('requires with nested field example from #2683 is fixed', async () => {
+      const parentItems = [
+        { "__typename": "ParentItem", "id": '1', "name": "Parent Item #1" },
+        { "__typename": "ParentItem", "id": '2', "name": "Parent Item #2" },
+        { "__typename": "ParentItem", "id": '3', "name": "Parent Item #3" },
+      ];
+
+      const childItems = [
+        { "__typename": "ChildItem", "id": '1', "name": "Child Item #1" },
+        { "__typename": "ChildItem", "id": '2', "name": "Child Item #2" },
+        { "__typename": "ChildItem", "id": '3', "name": "Child Item #3" },
+      ];
+
+      const s1 = {
+        name: 'S1',
+        typeDefs: gql`
+          type Query {
+            parentItems: [ParentItem!]!
+          }
+
+          type ParentItem @key(fields: "id") {
+            id: ID!
+            name: String!
+          }
+        `,
+        resolvers: {
+          Query: {
+            parentItems() {
+              return parentItems;
+            },
+          },
+          ParentItem: {
+            __resolveReference(ref: { id: string }) {
+              return parentItems.find(({id}) => ref.id === id);
+            },
+          }
+        }
+      }
+
+      const s2 = {
+        name: 'S2',
+        typeDefs: gql`
+          type ChildItem @key(fields: "id") {
+            id: ID!
+            name: String!
+            parentItem: ParentItem
+          }
+
+          type ParentItem @key(fields: "id") {
+            id: ID!
+            childItems: [ChildItem!]!
+          }
+        `,
+        resolvers: {
+          ChildItem: {
+            __resolveReference(ref: { id: string }) {
+              return childItems.find(({id}) => ref.id === id);
+            },
+            parentItem(ref: { id: string }) {
+              return parentItems.find(({id}) => ref.id === id);
+            }
+          },
+          ParentItem: {
+            __resolveReference(ref: { id: string }) {
+              return parentItems.find(({id}) => ref.id === id);
+            },
+            childItems(ref: { id: string }) {
+              return [childItems.find(({id}) => ref.id === id)];
+            }
+          },
+        }
+      }
+
+      const s3 = {
+        name: 'S3',
+        typeDefs: gql`
+          type ChildItem @key(fields: "id") {
+            id: ID!
+            name: String! @external
+            parentItem: ParentItem @external
+            message: String! @requires(fields: "name parentItem { name }")
+          }
+
+          type ParentItem {
+            name: String! @external
+          }
+        `,
+        resolvers: {
+          ChildItem: {
+            __resolveReference(ref: { id: string, name: string, parentItem: { name: string }}) {
+              console.log(ref);
+              return {
+                ...ref,
+                message: `${ref.parentItem.name} | ${ref.name}`,
+              };
+            }
+          }
+        }
+      }
+
+      const { serviceMap, schema, queryPlanner} = getFederatedTestingSchema([ s1, s2, s3 ]);
+      let operation = parseOp(`
+        query {
+          parentItems {
+            childItems {
+              message
+            }
+          }
+        }
+      `, schema);
+
+      let queryPlan = buildPlan(operation, queryPlanner);
+      expect(queryPlan).toMatchInlineSnapshot(`
+        QueryPlan {
+          Sequence {
+            Fetch(service: "S1") {
+              {
+                parentItems {
+                  __typename
+                  id
+                }
+              }
+            },
+            Flatten(path: "parentItems.@") {
+              Fetch(service: "S2") {
+                {
+                  ... on ParentItem {
+                    __typename
+                    id
+                  }
+                } =>
+                {
+                  ... on ParentItem {
+                    childItems {
+                      __typename
+                      id
+                      name
+                      parentItem {
+                        __typename
+                        id
+                      }
+                    }
+                  }
+                }
+              },
+            },
+            Flatten(path: "parentItems.@.childItems.@.parentItem") {
+              Fetch(service: "S1") {
+                {
+                  ... on ParentItem {
+                    __typename
+                    id
+                  }
+                } =>
+                {
+                  ... on ParentItem {
+                    name
+                  }
+                }
+              },
+            },
+            Flatten(path: "parentItems.@.childItems.@") {
+              Fetch(service: "S3") {
+                {
+                  ... on ChildItem {
+                    __typename
+                    name
+                    parentItem {
+                      name
+                    }
+                    id
+                  }
+                } =>
+                {
+                  ... on ChildItem {
+                    message
+                  }
+                }
+              },
+            },
+          },
+        }
+      `);
+      let response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
+      expect(response.errors).toBeUndefined();
+      expect(response.data).toMatchInlineSnapshot(`
+        Object {
+          "parentItems": Array [
+            Object {
+              "childItems": Array [
+                Object {
+                  "message": "Parent Item #1 | Child Item #1",
+                },
+              ],
+            },
+            Object {
+              "childItems": Array [
+                Object {
+                  "message": "Parent Item #2 | Child Item #2",
+                },
+              ],
+            },
+            Object {
+              "childItems": Array [
+                Object {
+                  "message": "Parent Item #3 | Child Item #3",
+                },
+              ],
+            },
+          ],
+        }
+      `);
+
+      operation = parseOp(`
+        query {
+          parentItems {
+            childItems {
+              ParentItem: parentItem {
+                name
+                id
+              }
+              message
+            }
+          }
+        }
+      `, schema);
+
+      queryPlan = buildPlan(operation, queryPlanner);
+      expect(queryPlan).toMatchInlineSnapshot(`
+        QueryPlan {
+          Sequence {
+            Fetch(service: "S1") {
+              {
+                parentItems {
+                  __typename
+                  id
+                }
+              }
+            },
+            Flatten(path: "parentItems.@") {
+              Fetch(service: "S2") {
+                {
+                  ... on ParentItem {
+                    __typename
+                    id
+                  }
+                } =>
+                {
+                  ... on ParentItem {
+                    childItems {
+                      __typename
+                      id
+                      ParentItem: parentItem {
+                        __typename
+                        id
+                      }
+                      name
+                      parentItem {
+                        __typename
+                        id
+                      }
+                    }
+                  }
+                }
+              },
+            },
+            Parallel {
+              Flatten(path: "parentItems.@.childItems.@.ParentItem") {
+                Fetch(service: "S1") {
+                  {
+                    ... on ParentItem {
+                      __typename
+                      id
+                    }
+                  } =>
+                  {
+                    ... on ParentItem {
+                      name
+                    }
+                  }
+                },
+              },
+              Sequence {
+                Flatten(path: "parentItems.@.childItems.@.parentItem") {
+                  Fetch(service: "S1") {
+                    {
+                      ... on ParentItem {
+                        __typename
+                        id
+                      }
+                    } =>
+                    {
+                      ... on ParentItem {
+                        name
+                      }
+                    }
+                  },
+                },
+                Flatten(path: "parentItems.@.childItems.@") {
+                  Fetch(service: "S3") {
+                    {
+                      ... on ChildItem {
+                        __typename
+                        name
+                        parentItem {
+                          name
+                        }
+                        id
+                      }
+                    } =>
+                    {
+                      ... on ChildItem {
+                        message
+                      }
+                    }
+                  },
+                },
+              },
+            },
+          },
+        }
+      `);
+      response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
+      expect(response.errors).toBeUndefined();
+      expect(response.data).toMatchInlineSnapshot(`
+        Object {
+          "parentItems": Array [
+            Object {
+              "childItems": Array [
+                Object {
+                  "ParentItem": Object {
+                    "id": "1",
+                    "name": "Parent Item #1",
+                  },
+                  "message": "Parent Item #1 | Child Item #1",
+                },
+              ],
+            },
+            Object {
+              "childItems": Array [
+                Object {
+                  "ParentItem": Object {
+                    "id": "2",
+                    "name": "Parent Item #2",
+                  },
+                  "message": "Parent Item #2 | Child Item #2",
+                },
+              ],
+            },
+            Object {
+              "childItems": Array [
+                Object {
+                  "ParentItem": Object {
+                    "id": "3",
+                    "name": "Parent Item #3",
+                  },
+                  "message": "Parent Item #3 | Child Item #3",
+                },
+              ],
+            },
+          ],
+        }
+      `);
+    });
   });
 
   describe('@key', () => {
@@ -6033,7 +6398,7 @@ describe('executeQueryPlan', () => {
     });
   });
 
-  it(`surface post-processing errors as extensions in the response`, async () => {
+  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