fix(federation): fixed a QP panic saying "would create a cycle"

apollo-federation/src/query_plan/fetch_dependency_graph.rs

@@ -1451,7 +1451,9 @@ impl FetchDependencyGraph {
         // subgraph name, but have no worries for `mergeAt` since it contains either number of
         // field names, and the later is restricted by graphQL so as to not be an issue.
         let mut by_subgraphs = MultiMap::new();
-        for node_index in self.graph.node_indices() {
+        let sorted_nodes = petgraph::algo::toposort(&self.graph, None)
+            .map_err(|_| FederationError::internal("Failed to sort nodes due to cycle(s)"))?;
+        for node_index in sorted_nodes {
             let node = self.node_weight(node_index)?;
             // We exclude nodes without inputs because that's what we look for. In practice, this
             // mostly just exclude root nodes, which we don't really want to bother with anyway.

apollo-federation/tests/query_plan/build_query_plan_tests.rs

@@ -720,3 +720,197 @@ fn defer_gets_stripped_out() {
     );
     assert_eq!(plan_one, plan_two)
 }
+
+#[test]
+fn test_merging_fetches_do_not_create_cycle_in_fetch_dependency_graph() {
+    // This is a test for ROUTER-546 (the second part).
+    let planner = planner!(
+        S: r#"
+          type Query {
+            start: T!
+          }
+
+          type T @key(fields: "id") {
+            id: String!
+          }
+          "#,
+        A: r#"
+          type T @key(fields: "id") {
+            id: String! @shareable
+            u: U! @shareable
+          }
+
+          type U @key(fields: "id") {
+            id: ID!
+            a: String! @shareable
+            b: String @shareable
+          }
+          "#,
+        B: r#"
+          type T @key(fields: "id") {
+            id: String! @external
+            u: U! @shareable
+          }
+
+          type U @key(fields: "id") {
+            id: ID!
+            a: String! @shareable
+            # Note: b is not here.
+          }
+
+          # This definition is necessary.
+          extend type W @key(fields: "id") {
+            id: ID @external
+          }
+          "#,
+        C: r#"
+          extend type U @key(fields: "id") {
+            id: ID! @external
+            a: String! @external
+            b: String @external
+            w: W @requires(fields: "a b")
+          }
+
+          type W @key(fields: "id") {
+            id: ID
+            y: Y
+            w1: Int
+            w2: Int
+            w3: Int
+            w4: Int
+            w5: Int
+          }
+
+          type Y {
+            y1: Int
+            y2: Int
+            y3: Int
+          }
+          "#,
+    );
+    assert_plan!(
+        &planner,
+        r#"
+          {
+            start {
+              u {
+                w {
+                  id
+                  w1
+                  w2
+                  w3
+                  w4
+                  w5
+                  y {
+                    y1
+                    y2
+                    y3
+                  }
+                }
+              }
+            }
+          }
+        "#,
+        @r###"
+    QueryPlan {
+      Sequence {
+        Fetch(service: "S") {
+          {
+            start {
+              __typename
+              id
+            }
+          }
+        },
+        Parallel {
+          Sequence {
+            Flatten(path: "start") {
+              Fetch(service: "B") {
+                {
+                  ... on T {
+                    __typename
+                    id
+                  }
+                } =>
+                {
+                  ... on T {
+                    u {
+                      __typename
+                      id
+                    }
+                  }
+                }
+              },
+            },
+            Flatten(path: "start.u") {
+              Fetch(service: "A") {
+                {
+                  ... on U {
+                    __typename
+                    id
+                  }
+                } =>
+                {
+                  ... on U {
+                    b
+                    a
+                  }
+                }
+              },
+            },
+          },
+          Flatten(path: "start") {
+            Fetch(service: "A") {
+              {
+                ... on T {
+                  __typename
+                  id
+                }
+              } =>
+              {
+                ... on T {
+                  u {
+                    __typename
+                    id
+                    b
+                    a
+                  }
+                }
+              }
+            },
+          },
+        },
+        Flatten(path: "start.u") {
+          Fetch(service: "C") {
+            {
+              ... on U {
+                __typename
+                a
+                b
+                id
+              }
+            } =>
+            {
+              ... on U {
+                w {
+                  y {
+                    y1
+                    y2
+                    y3
+                  }
+                  id
+                  w1
+                  w2
+                  w3
+                  w4
+                  w5
+                }
+              }
+            }
+          },
+        },
+      },
+    }
+    "###
+    );
+}

apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs

@@ -1647,6 +1647,29 @@ fn it_handles_multiple_requires_with_multiple_fetches() {
               },
             },
             Parallel {
+              Flatten(path: "t") {
+                Fetch(service: "s2") {
+                  {
+                    ... on T {
+                      __typename
+                      x {
+                        isX
+                      }
+                      v {
+                        y {
+                          isY
+                        }
+                      }
+                      id
+                    }
+                  } =>
+                  {
+                    ... on T {
+                      foo
+                    }
+                  }
+                },
+              },
               Sequence {
                 Flatten(path: "t") {
                   Fetch(service: "s2") {
@@ -1709,29 +1732,6 @@ fn it_handles_multiple_requires_with_multiple_fetches() {
                   },
                 },
               },
-              Flatten(path: "t") {
-                Fetch(service: "s2") {
-                  {
-                    ... on T {
-                      __typename
-                      x {
-                        isX
-                      }
-                      v {
-                        y {
-                          isY
-                        }
-                      }
-                      id
-                    }
-                  } =>
-                  {
-                    ... on T {
-                      foo
-                    }
-                  }
-                },
-              },
             },
           },
         }

apollo-federation/tests/query_plan/supergraphs/test_merging_fetches_do_not_create_cycle_in_fetch_dependency_graph.graphql

@@ -0,0 +1,94 @@
+# Composed from subgraphs with hash: 58cfa42df5c5f20fb0fbe43d4a506b3654439de1
+schema
+  @link(url: "https://specs.apollo.dev/link/v1.0")
+  @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION)
+{
+  query: Query
+}
+
+directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
+
+directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
+
+directive @join__graph(name: String!, url: String!) on ENUM_VALUE
+
+directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
+
+directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR
+
+directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
+
+directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
+
+scalar join__FieldSet
+
+enum join__Graph {
+  A @join__graph(name: "A", url: "none")
+  B @join__graph(name: "B", url: "none")
+  C @join__graph(name: "C", url: "none")
+  S @join__graph(name: "S", url: "none")
+}
+
+scalar link__Import
+
+enum link__Purpose {
+  """
+  `SECURITY` features provide metadata necessary to securely resolve fields.
+  """
+  SECURITY
+
+  """
+  `EXECUTION` features provide metadata necessary for operation execution.
+  """
+  EXECUTION
+}
+
+type Query
+  @join__type(graph: A)
+  @join__type(graph: B)
+  @join__type(graph: C)
+  @join__type(graph: S)
+{
+  start: T! @join__field(graph: S)
+}
+
+type T
+  @join__type(graph: A, key: "id")
+  @join__type(graph: B, key: "id")
+  @join__type(graph: S, key: "id")
+{
+  id: String! @join__field(graph: A) @join__field(graph: B, external: true) @join__field(graph: S)
+  u: U! @join__field(graph: A) @join__field(graph: B)
+}
+
+type U
+  @join__type(graph: A, key: "id")
+  @join__type(graph: B, key: "id")
+  @join__type(graph: C, key: "id", extension: true)
+{
+  id: ID!
+  a: String! @join__field(graph: A) @join__field(graph: B) @join__field(graph: C, external: true)
+  b: String @join__field(graph: A) @join__field(graph: C, external: true)
+  w: W @join__field(graph: C, requires: "a b")
+}
+
+type W
+  @join__type(graph: B, key: "id", extension: true)
+  @join__type(graph: C, key: "id")
+{
+  id: ID
+  y: Y @join__field(graph: C)
+  w1: Int @join__field(graph: C)
+  w2: Int @join__field(graph: C)
+  w3: Int @join__field(graph: C)
+  w4: Int @join__field(graph: C)
+  w5: Int @join__field(graph: C)
+}
+
+type Y
+  @join__type(graph: C)
+{
+  y1: Int
+  y2: Int
+  y3: Int
+}