diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index 40961a8e5a..93d418606d 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/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. diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests.rs b/apollo-federation/tests/query_plan/build_query_plan_tests.rs index e849bf74c2..a5a8769c2a 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/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 + } + } + } + }, + }, + }, + } + "### + ); +} diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs index d33ef66191..b218969a35 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs +++ b/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 - } - } - }, - }, }, }, } diff --git a/apollo-federation/tests/query_plan/supergraphs/test_merging_fetches_do_not_create_cycle_in_fetch_dependency_graph.graphql b/apollo-federation/tests/query_plan/supergraphs/test_merging_fetches_do_not_create_cycle_in_fetch_dependency_graph.graphql new file mode 100644 index 0000000000..aea8d395a8 --- /dev/null +++ b/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 +}