Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apollo-federation/src/operation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems redundant? guessing its a leftover from debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a necessary change. After can_rebase_on change, some tests failed at .or_insert call below because ele.key() and element.key() became different in some cases (... on Query vs ...).

In general, ele and element may be different and it makes sense to look up element.key(), instead of ele.key(). It appears that resulting in a different key via rebase here never happened before.

BTW, to_owned_key/as_borrowed_key calls had to be added, since element itself is going to move into the closure below. So, the key had to be cloned using to_owned_key (the normal clone() would only do a shallow cloning).

.or_insert(|| {
Selection::from_element(
element,
Expand Down
11 changes: 11 additions & 0 deletions apollo-federation/src/operation/rebase.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
7 changes: 7 additions & 0 deletions apollo-federation/src/schema/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 1 addition & 5 deletions apollo-federation/src/schema/schema_upgrader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand All @@ -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
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2093,7 +2093,7 @@ fn defer_test_defer_on_query_root_type() {
Flatten(path: "op2.next") {
Fetch(service: "Subgraph2") {
{
... on Query {
... {
op3
}
}
Expand All @@ -2107,7 +2107,7 @@ fn defer_test_defer_on_query_root_type() {
Flatten(path: "op2.next") {
Fetch(service: "Subgraph1") {
{
... on Query {
... {
op1
}
}
Expand All @@ -2116,7 +2116,7 @@ fn defer_test_defer_on_query_root_type() {
Flatten(path: "op2.next") {
Fetch(service: "Subgraph2") {
{
... on Query {
... {
op4
}
}
Expand Down Expand Up @@ -2174,7 +2174,7 @@ fn defer_test_defer_on_everything_queried() {
Flatten(path: "") {
Fetch(service: "Subgraph1") {
{
... on Query {
... {
t {
__typename
id
Expand Down Expand Up @@ -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
}
}
}
},
},
},
]
},
}
"###
);
}
Original file line number Diff line number Diff line change
@@ -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
}
2 changes: 1 addition & 1 deletion apollo-router/src/services/supergraph/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Expand Down