diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 71b4aece8b..f0db930fb5 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -894,10 +894,6 @@ impl Selection { } } - fn sub_selection_type_position(&self) -> Option { - Some(self.selection_set()?.type_position.clone()) - } - pub(crate) fn conditions(&self) -> Result { 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, ) -> Result { 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() 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 a5a8769c2a..4062311f5b 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/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 + } + } + } + }, + } + "### + ); +} diff --git a/apollo-federation/tests/query_plan/supergraphs/redundant_typename_for_inline_fragments_without_type_condition.graphql b/apollo-federation/tests/query_plan/supergraphs/redundant_typename_for_inline_fragments_without_type_condition.graphql new file mode 100644 index 0000000000..0b9050d640 --- /dev/null +++ b/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] +}