diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index eb362d5722..22ca906c4f 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -1699,8 +1699,9 @@ impl SelectionSet { let Some(sub_selection_type) = element.sub_selection_type_position()? else { return Err(FederationError::internal("unexpected error: add_at_path encountered a field that is not of a composite type".to_string())); }; + let element_key = element.key().to_owned_key(); let mut selection = Arc::make_mut(&mut self.selections) - .entry(ele.key()) + .entry(element_key.as_borrowed_key()) .or_insert(|| { Selection::from_element( element, diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index bbe087537e..227deae000 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -398,6 +398,17 @@ impl InlineFragment { CompositeTypeDefinitionPosition::try_from(rebased_condition_position) }) { + // Root types can always be rebased and the type condition is unnecessary. + // Moreover, the actual subgraph might have renamed the root types, but the + // supergraph schema does not contain that information. + // Note: We only handle when the rebased condition is the same as the parent type. They + // could be different in rare cases, but that will be fixed after the + // source-awareness initiative is complete. + if rebased_condition == *parent_type + && parent_schema.is_root_type(rebased_condition.type_name()) + { + return (true, None); + } // chained if let chains are not yet supported // see https://github.com/rust-lang/rust/issues/53667 if runtime_types_intersect(parent_type, &rebased_condition, parent_schema) { diff --git a/apollo-federation/src/schema/mod.rs b/apollo-federation/src/schema/mod.rs index 7b544642ba..265331eae2 100644 --- a/apollo-federation/src/schema/mod.rs +++ b/apollo-federation/src/schema/mod.rs @@ -168,6 +168,13 @@ impl FederationSchema { self.get_type(type_name).ok() } + pub(crate) fn is_root_type(&self, type_name: &Name) -> bool { + self.schema() + .schema_definition + .iter_root_operations() + .any(|op| *op.1 == *type_name) + } + /// Return the possible runtime types for a definition. /// /// For a union, the possible runtime types are its members. diff --git a/apollo-federation/src/schema/schema_upgrader.rs b/apollo-federation/src/schema/schema_upgrader.rs index ce9448caf7..8f54ef1bdb 100644 --- a/apollo-federation/src/schema/schema_upgrader.rs +++ b/apollo-federation/src/schema/schema_upgrader.rs @@ -629,11 +629,7 @@ impl SchemaUpgrader { } fn is_root_type(schema: &FederationSchema, ty: &TypeDefinitionPosition) -> bool { - schema - .schema() - .schema_definition - .iter_root_operations() - .any(|op| op.1.as_str() == ty.type_name().as_str()) + schema.is_root_type(ty.type_name()) } fn remove_directives_on_interface( 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 7f967ab380..5188034d62 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests.rs @@ -1213,13 +1213,15 @@ fn condition_order_router799() { } } "#, + // Note: `on Mutation` is suppressed in the query plan. But, the operation is still a + // mutation. @r###" QueryPlan { Include(if: $var1) { Skip(if: $var0) { Fetch(service: "books") { { - ... on Mutation { + ... { field0: __typename } } @@ -1240,13 +1242,15 @@ fn condition_order_router799() { } } "#, + // Note: `on Mutation` is suppressed in the query plan. But, the operation is still a + // mutation. @r###" QueryPlan { Include(if: $var1) { Skip(if: $var0) { Fetch(service: "books") { { - ... on Mutation { + ... { field0: __typename } } diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/defer.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/defer.rs index 8a199eb447..e11ef2b35d 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/defer.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/defer.rs @@ -2093,7 +2093,7 @@ fn defer_test_defer_on_query_root_type() { Flatten(path: "op2.next") { Fetch(service: "Subgraph2") { { - ... on Query { + ... { op3 } } @@ -2107,7 +2107,7 @@ fn defer_test_defer_on_query_root_type() { Flatten(path: "op2.next") { Fetch(service: "Subgraph1") { { - ... on Query { + ... { op1 } } @@ -2116,7 +2116,7 @@ fn defer_test_defer_on_query_root_type() { Flatten(path: "op2.next") { Fetch(service: "Subgraph2") { { - ... on Query { + ... { op4 } } @@ -2174,7 +2174,7 @@ fn defer_test_defer_on_everything_queried() { Flatten(path: "") { Fetch(service: "Subgraph1") { { - ... on Query { + ... { t { __typename id @@ -3478,3 +3478,53 @@ fn defer_test_the_path_in_defer_includes_traversed_fragments() { "### ); } + +#[test] +fn defer_on_renamed_root_type() { + let planner = planner!( + config = config_with_defer(), + Subgraph1: r#" + type MyQuery { + thing: Thing + } + + type Thing { + i: Int + } + + schema { query: MyQuery } + "#, + ); + assert_plan!( + &planner, + r#" + { + ... @defer { + thing { i } + } + } + "#, + @r###" + QueryPlan { + Defer { + Primary {}, [ + Deferred(depends: [], path: "") { + { thing { i } }: + Flatten(path: "") { + Fetch(service: "Subgraph1") { + { + ... { + thing { + i + } + } + } + }, + }, + }, + ] + }, + } + "### + ); +} diff --git a/apollo-federation/tests/query_plan/supergraphs/defer_on_renamed_root_type.graphql b/apollo-federation/tests/query_plan/supergraphs/defer_on_renamed_root_type.graphql new file mode 100644 index 0000000000..c3da7ab6e4 --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/defer_on_renamed_root_type.graphql @@ -0,0 +1,66 @@ +# Composed from subgraphs with hash: 342e57c5d2c95caa99ea85f70fdd3114d65d6ca7 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.5", 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, contextArguments: [join__ContextArgument!]) 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 + +input join__ContextArgument { + name: String! + type: String! + context: String! + selection: join__FieldValue! +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +scalar join__FieldValue + +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 +} + +type Query + @join__type(graph: SUBGRAPH1) +{ + thing: Thing +} + +type Thing + @join__type(graph: SUBGRAPH1) +{ + i: Int +} diff --git a/apollo-router/src/services/supergraph/tests.rs b/apollo-router/src/services/supergraph/tests.rs index b6c7f37930..f61c5beec7 100644 --- a/apollo-router/src/services/supergraph/tests.rs +++ b/apollo-router/src/services/supergraph/tests.rs @@ -1276,7 +1276,7 @@ async fn root_typename_with_defer_and_empty_first_response() { let subgraphs = MockedSubgraphs([ ("user", MockSubgraph::builder().with_json( serde_json::json!{{ - "query": "{ ... on Query { currentUser { activeOrganization { __typename id } } } }", + "query": "{ ... { currentUser { activeOrganization { __typename id } } } }", }}, serde_json::json!{{"data": {"currentUser": { "activeOrganization": { "__typename": "Organization", "id": "0" } }}}} ).build()),