fix(federation): remove redundant typename for inline fragments (#5864)

When adding `__typename` field for abstract types, we need to check for inline fragment type condition and not the parent selection set. If we have an inline fragment without type condition and it is pointing to and abstract type, then the required `__typename` field should have already been added to its parent field.

We continue adding `__typename` for fragments with a type condition as their type condition _may_ be different than the parent type.

```graphql
foo {  
  # if this is abstract type we will add __typename
  a
  ... @Skip(if: false) {  
    # if this fragment is on abstract type we don't need to add anything as
    # we already added the __typename to the parent selection
    b
  }
}
```

apollo-federation/src/operation/mod.rs

@@ -894,10 +894,6 @@ impl Selection {
         }
     }
 
-    fn sub_selection_type_position(&self) -> Option<CompositeTypeDefinitionPosition> {
-        Some(self.selection_set()?.type_position.clone())
-    }
-
     pub(crate) fn conditions(&self) -> Result<Conditions, FederationError> {
         let self_conditions = Conditions::from_directives(self.directives())?;
         if let Conditions::Boolean(false) = self_conditions {
@@ -2504,14 +2500,22 @@ impl SelectionSet {
         })
     }
 
+    /// Adds __typename field for selection sets on abstract types.
+    ///
+    /// __typename is added to the sub selection set of a given selection in following conditions
+    /// * if a given selection is a field, we add a __typename sub selection if its selection set type
+    /// position is an abstract type
+    /// * if a given selection is a fragment, we only add __typename sub selection if fragment specifies
+    /// type condition and that type condition is an abstract type.
     pub(crate) fn add_typename_field_for_abstract_types(
         &self,
         parent_type_if_abstract: Option<AbstractTypeDefinitionPosition>,
     ) -> Result<SelectionSet, FederationError> {
         let mut selection_map = SelectionMap::new();
         if let Some(parent) = parent_type_if_abstract {
-            // XXX(@goto-bus-stop): if the selection set has an *alias* named __typename for some
-            // other field, this doesn't work right. is that allowed?
+            // We don't handle aliased __typename fields. This means we may end up with additional
+            // __typename sub selection. This should be fine though as aliased __typenames should
+            // be rare occurrence.
             if !self.has_top_level_typename_field() {
                 let typename_selection = Selection::from_field(
                     Field::new_introspection_typename(&self.schema, &parent.into(), None),
@@ -2522,11 +2526,24 @@ impl SelectionSet {
         }
         for selection in self.selections.values() {
             selection_map.insert(if let Some(selection_set) = selection.selection_set() {
-                let type_if_abstract = selection
-                    .sub_selection_type_position()
-                    .and_then(|ty| ty.try_into().ok());
+                let abstract_type = match selection {
+                    Selection::Field(field_selection) => field_selection
+                        .selection_set
+                        .as_ref()
+                        .map(|s| s.type_position.clone()),
+                    Selection::FragmentSpread(fragment_selection) => {
+                        Some(fragment_selection.spread.type_condition_position.clone())
+                    }
+                    Selection::InlineFragment(inline_fragment_selection) => {
+                        inline_fragment_selection
+                            .inline_fragment
+                            .type_condition_position
+                            .clone()
+                    }
+                }
+                .and_then(|ty| ty.try_into().ok());
                 let updated_selection_set =
-                    selection_set.add_typename_field_for_abstract_types(type_if_abstract)?;
+                    selection_set.add_typename_field_for_abstract_types(abstract_type)?;
 
                 if updated_selection_set == *selection_set {
                     selection.clone()

apollo-federation/tests/query_plan/build_query_plan_tests.rs

@@ -914,3 +914,43 @@ fn test_merging_fetches_do_not_create_cycle_in_fetch_dependency_graph() {
     "###
     );
 }
+
+#[test]
+fn redundant_typename_for_inline_fragments_without_type_condition() {
+    let planner = planner!(
+        Subgraph1: r#"
+          type Query {
+            products: [Product]
+          }
+          interface Product {
+            name: String
+          }
+        "#,
+    );
+    assert_plan!(
+        &planner,
+        r#"
+          {
+            products {
+              ... @skip(if: false) {
+                name
+              }
+            }
+          }
+        "#,
+        @r###"
+        QueryPlan {
+          Fetch(service: "Subgraph1") {
+            {
+              products {
+                __typename
+                ... @skip(if: false) {
+                  name
+                }
+              }
+            }
+          },
+        }
+        "###
+    );
+}

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

@@ -0,0 +1,53 @@
+# Composed from subgraphs with hash: d267b3f18f7736e1a3e39a951696c318d1f46a34
+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 {
+  SUBGRAPH1 @join__graph(name: "Subgraph1", 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
+}
+
+interface Product
+  @join__type(graph: SUBGRAPH1)
+{
+  name: String
+}
+
+type Query
+  @join__type(graph: SUBGRAPH1)
+{
+  products: [Product]
+}