fix(federation): updated PathTree::from_path to use the last trigger …

…value (#6118)

apollo-federation/src/query_graph/path_tree.rs

@@ -226,11 +226,20 @@ where
 
         struct ByUniqueEdge<'inputs, TTrigger, GraphPathIter> {
             target_node: NodeIndex,
-            by_unique_trigger:
-                IndexMap<&'inputs Arc<TTrigger>, PathTreeChildInputs<'inputs, GraphPathIter>>,
+            by_unique_trigger: IndexMap<
+                &'inputs Arc<TTrigger>,
+                PathTreeChildInputs<'inputs, TTrigger, GraphPathIter>,
+            >,
         }
 
-        struct PathTreeChildInputs<'inputs, GraphPathIter> {
+        struct PathTreeChildInputs<'inputs, TTrigger, GraphPathIter> {
+            /// trigger: the final trigger value
+            ///   - Two equivalent triggers can have minor differences in the sibling_typename.
+            ///     This field holds the final trigger value that will be used.
+            /// PORT_NOTE: The JS QP used the last trigger value. So, we are following that
+            ///            to avoid mismatches. But, it can be revisited.
+            ///            We may want to keep or merge the sibling_typename values.
+            trigger: &'inputs Arc<TTrigger>,
             conditions: Option<Arc<OpPathTree>>,
             sub_paths_and_selections: Vec<(GraphPathIter, Option<&'inputs Arc<SelectionSet>>)>,
         }
@@ -263,6 +272,7 @@ where
             match for_edge.by_unique_trigger.entry(trigger) {
                 Entry::Occupied(entry) => {
                     let existing = entry.into_mut();
+                    existing.trigger = trigger;
                     existing.conditions = merge_conditions(&existing.conditions, conditions);
                     existing
                         .sub_paths_and_selections
@@ -271,6 +281,7 @@ where
                 }
                 Entry::Vacant(entry) => {
                     entry.insert(PathTreeChildInputs {
+                        trigger,
                         conditions: conditions.clone(),
                         sub_paths_and_selections: vec![(graph_path_iter, selection)],
                     });
@@ -280,10 +291,10 @@ where
 
         let mut childs = Vec::new();
         for (edge, by_unique_edge) in merged {
-            for (trigger, child) in by_unique_edge.by_unique_trigger {
+            for (_, child) in by_unique_edge.by_unique_trigger {
                 childs.push(Arc::new(PathTreeChild {
                     edge,
-                    trigger: trigger.clone(),
+                    trigger: child.trigger.clone(),
                     conditions: child.conditions.clone(),
                     tree: Arc::new(Self::from_paths(
                         graph.clone(),

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

@@ -295,3 +295,104 @@ fn add_back_sibling_typename_to_interface_object() {
     "###
     );
 }
+
+#[test]
+fn test_indirect_branch_merging_with_typename_sibling() {
+    let planner = planner!(
+        Subgraph1: r#"
+            type Query {
+                test: T
+            }
+
+            interface T {
+                id: ID!
+            }
+
+            type A implements T @key(fields: "id") {
+                id: ID!
+            }
+
+            type B implements T @key(fields: "id") {
+                id: ID!
+            }
+        "#,
+        Subgraph2: r#"
+            interface T {
+                id: ID!
+                f: Int!
+            }
+
+            type A implements T @key(fields: "id") {
+                id: ID!
+                f: Int!
+            }
+
+            type B implements T @key(fields: "id") {
+                id: ID!
+                f: Int!
+            }
+        "#,
+    );
+    // This operation has two `f` selection instances: One with __typename sibling and one without.
+    // It creates multiple identical branches in the form of `... on A { f }` with different `f`.
+    // The query plan must chose one over the other, which is implementation specific.
+    // Currently, the last one is chosen.
+    assert_plan!(
+        &planner,
+        r#"
+            {
+                test {
+                    __typename
+                    f # <= This will have a sibling typename value.
+                    ... on A {
+                        f # <= This one will have no sibling typename.
+                    }
+                }
+            }
+        "#,
+        @r###"
+    QueryPlan {
+      Sequence {
+        Fetch(service: "Subgraph1") {
+          {
+            test {
+              __typename
+              ... on A {
+                __typename
+                id
+              }
+              ... on B {
+                __typename
+                id
+              }
+            }
+          }
+        },
+        Flatten(path: "test") {
+          Fetch(service: "Subgraph2") {
+            {
+              ... on A {
+                __typename
+                id
+              }
+              ... on B {
+                __typename
+                id
+              }
+            } =>
+            {
+              ... on A {
+                f
+              }
+              ... on B {
+                __typename
+                f
+              }
+            }
+          },
+        },
+      },
+    }
+    "###
+    );
+}

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

@@ -0,0 +1,81 @@
+# Composed from subgraphs with hash: 9be0826e3b911556466c2c410f7df8b53c241774
+schema
+  @link(url: "https://specs.apollo.dev/link/v1.0")
+  @link(url: "https://specs.apollo.dev/join/v0.4", for: EXECUTION)
+{
+  query: Query
+}
+
+directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION
+
+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, overrideLabel: String) 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
+
+type A implements T
+  @join__implements(graph: SUBGRAPH1, interface: "T")
+  @join__implements(graph: SUBGRAPH2, interface: "T")
+  @join__type(graph: SUBGRAPH1, key: "id")
+  @join__type(graph: SUBGRAPH2, key: "id")
+{
+  id: ID!
+  f: Int! @join__field(graph: SUBGRAPH2)
+}
+
+type B implements T
+  @join__implements(graph: SUBGRAPH1, interface: "T")
+  @join__implements(graph: SUBGRAPH2, interface: "T")
+  @join__type(graph: SUBGRAPH1, key: "id")
+  @join__type(graph: SUBGRAPH2, key: "id")
+{
+  id: ID!
+  f: Int! @join__field(graph: SUBGRAPH2)
+}
+
+scalar join__DirectiveArguments
+
+scalar join__FieldSet
+
+enum join__Graph {
+  SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none")
+  SUBGRAPH2 @join__graph(name: "Subgraph2", 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: SUBGRAPH1)
+  @join__type(graph: SUBGRAPH2)
+{
+  test: T @join__field(graph: SUBGRAPH1)
+}
+
+interface T
+  @join__type(graph: SUBGRAPH1)
+  @join__type(graph: SUBGRAPH2)
+{
+  id: ID!
+  f: Int! @join__field(graph: SUBGRAPH2)
+}