Fix: Avoid infinite loop when merging siblings (#282)

Author: kamilkisiela

lib/query-planner/fixture/issues/281.supergraph.graphql

@@ -0,0 +1,102 @@
+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: "http://localhost/a")
+  B @join__graph(name: "b", url: "http://localhost/b")
+  C @join__graph(name: "c", url: "http://localhost/c")
+  D @join__graph(name: "d", url: "http://localhost/d")
+}
+
+scalar link__Import
+
+enum link__Purpose {
+  SECURITY
+  EXECUTION
+}
+
+type Product
+  @join__type(graph: A, key: "id")
+  @join__type(graph: B, key: "id")
+  @join__type(graph: B, key: "pid")
+  @join__type(graph: C, key: "pid")
+  @join__type(graph: D, key: "pid") {
+  id: ID! @join__field(graph: A) @join__field(graph: B)
+  pid: ID! @join__field(graph: B) @join__field(graph: C) @join__field(graph: D)
+  b: String! @join__field(graph: B)
+  c: String! @join__field(graph: C)
+  d: String! @join__field(graph: D)
+}
+
+type Query
+  @join__type(graph: A)
+  @join__type(graph: B)
+  @join__type(graph: C)
+  @join__type(graph: D) {
+  viewer: Viewer! @join__field(graph: A)
+}
+
+union Review
+  @join__type(graph: A)
+  @join__unionMember(graph: A, member: "AnonymousReview")
+  @join__unionMember(graph: A, member: "UserReview") =
+  | AnonymousReview
+  | UserReview
+
+type UserReview @join__type(graph: A) {
+  # user_body: String!
+  product: Product
+}
+
+type AnonymousReview @join__type(graph: A) {
+  # anonymous_body: String!
+  product: Product
+}
+
+type Viewer @join__type(graph: A) {
+  review: Review
+}

lib/query-planner/src/planner/fetch/fetch_graph.rs

@@ -63,6 +63,11 @@ impl FetchGraph {
         has_path_connecting(&self.graph, ancestor, descendant, None)
     }
 
+    /// Checks if one is ancestor of the other and vice versa
+    pub fn is_ancestor_or_descendant(&self, a: NodeIndex, b: NodeIndex) -> bool {
+        self.is_descendant_of(a, b) || self.is_descendant_of(b, a)
+    }
+
     pub fn step_indices(&self) -> NodeIndices<FetchStepData> {
         self.graph.node_indices()
     }

lib/query-planner/src/planner/fetch/optimize/merge_siblings.rs

@@ -128,6 +128,11 @@ impl FetchStepData {
             }
         }
 
+        if fetch_graph.is_ancestor_or_descendant(self_index, other_index) {
+            // Looks like they depend on each other
+            return false;
+        }
+
         can_merge_base
     }
 }

lib/query-planner/src/tests/issues.rs

@@ -0,0 +1,130 @@
+use crate::{
+    tests::testkit::{build_query_plan, init_logger},
+    utils::parsing::parse_operation,
+};
+use std::error::Error;
+
+#[test]
+fn issue_281_test() -> Result<(), Box<dyn Error>> {
+    init_logger();
+    let document = parse_operation(
+        r#"
+        {
+          viewer {
+            review {
+              ... on AnonymousReview {
+                __typename
+                product {
+                  b
+                }
+              }
+              ... on UserReview {
+                __typename
+                product {
+                  c
+                  d
+                }
+              }
+            }
+          }
+        }
+
+        "#,
+    );
+    let query_plan = build_query_plan("fixture/issues/281.supergraph.graphql", document)?;
+
+    insta::assert_snapshot!(format!("{}", query_plan), @r#"
+    QueryPlan {
+      Sequence {
+        Fetch(service: "a") {
+          {
+            viewer {
+              review {
+                __typename
+                ... on AnonymousReview {
+                  __typename
+                  product {
+    ...a              }
+                }
+                ... on UserReview {
+                  __typename
+                  product {
+    ...a              }
+                }
+              }
+            }
+          }
+          fragment a on Product {
+            __typename
+            id
+          }
+        },
+        Parallel {
+          Flatten(path: "viewer.review|[UserReview].product") {
+            Fetch(service: "b") {
+              {
+                ... on Product {
+                  __typename
+                  id
+                }
+              } =>
+              {
+                ... on Product {
+                  pid
+                }
+              }
+            },
+          },
+          Flatten(path: "viewer.review|[AnonymousReview].product") {
+            Fetch(service: "b") {
+              {
+                ... on Product {
+                  __typename
+                  id
+                }
+              } =>
+              {
+                ... on Product {
+                  b
+                }
+              }
+            },
+          },
+        },
+        Flatten(path: "viewer.review|[UserReview].product") {
+          Fetch(service: "c") {
+            {
+              ... on Product {
+                __typename
+                pid
+              }
+            } =>
+            {
+              ... on Product {
+                c
+                pid
+              }
+            }
+          },
+        },
+        Flatten(path: "viewer.review|[UserReview].product") {
+          Fetch(service: "d") {
+            {
+              ... on Product {
+                __typename
+                pid
+              }
+            } =>
+            {
+              ... on Product {
+                d
+              }
+            }
+          },
+        },
+      },
+    },
+    "#);
+
+    Ok(())
+}

lib/query-planner/src/tests/mod.rs

@@ -5,6 +5,7 @@ mod include_skip;
 mod interface;
 mod interface_object;
 mod interface_object_with_requires;
+mod issues;
 mod mutations;
 mod object_entities;
 mod override_requires;