Skip to content

Commit 3f78260

Browse files
fix(router, executor): fix abstract type project and improve projection plan performance (#633)
#641 + #634 + the following: This PR introduces three related improvements to debugging the projection logic: - pretty-printing of projection plans - a new `projection` command in `dev` CLI to pretty-print the plan for a given operation and supergrah - refactor to reduce condition to their minimal forms when possible The new `dev` command works like the `plan` command, you pass both supergraph and operation and get back a pretty-printed response projection plan. This helped me to understand why ROUTER-235 issue occured. The simplification of field projection conditions was needed to make the pretty-printed plan much cleaner and easier to understand, but also to make the checking more efficient at runtime (when the response projection happens - now there should be fewer conditions to check). An example: ``` $ cargo dev projection supergraph.graphql op.graphql contentPage: { conditions: (ParentType(Exact(Query)) AND FieldType(Exact(ContentPage))) selections: contentBody: { conditions: (ParentType(Exact(ContentPage)) AND FieldType(Exact(ContentContainer))) selections: section: { conditions: (ParentType(Exact(ContentContainer)) AND FieldType(OneOf(IContent, ContentA, ContentB))) selections: contentChildren: { conditions: ((ParentType(Exact(ContentA)) AND FieldType(Exact(ContentAChild))) OR (ParentType(Exact(ContentB)) AND FieldType(Exact(ContentBChild)))) selections: title: { conditions: ((ParentType(Exact(ContentAChild)) AND FieldType(Exact(String))) OR (ParentType(Exact(ContentBChild)) AND FieldType(Exact(String)))) } } } } } ``` --------- Co-authored-by: Dotan Simha <dotansimha@gmail.com>
1 parent f8c304c commit 3f78260

File tree

11 files changed

+997
-162
lines changed

11 files changed

+997
-162
lines changed

.changeset/projection-fix.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
router: patch
3+
executor: patch
4+
---
5+
6+
Fixed response projection for fields on different concrete types of interfaces and unions.
7+
8+
When a query included fragments on an abstract type (interface or union) that selected fields with the same name but different return types, the projection would incorrectly use a single, merged plan for all types. This caused projection to fail when processing responses where different concrete types had different field implementations.
9+
10+
For example, with `... on A { children { id } }` and `... on B { children { id } }` where `A.children` returns `[AChild]` and `B.children` returns `[BChild]`, the projection would fail to correctly distinguish between the types and return incomplete or incorrect data.
11+
12+
The fix introduces type-aware plan merging, which preserves the context of which concrete types a field came from. During response projection, the type is now determined dynamically for each object, ensuring the correct field type is used.
13+
14+
In addition, a refactor of the response projection logic was performed to improve performance.

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

bin/dev-cli/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ path = "src/main.rs"
99

1010
[dependencies]
1111
hive-router-query-planner = { path = "../../lib/query-planner" }
12+
hive-router-plan-executor = { path = "../../lib/executor" }
1213

1314
graphql-parser = { workspace = true }
1415
tracing-subscriber = { workspace = true }

bin/dev-cli/src/main.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
use std::env;
22
use std::process;
33

4+
use hive_router_plan_executor::introspection::schema::SchemaWithMetadata;
5+
use hive_router_plan_executor::projection::plan::FieldProjectionPlan;
46
use hive_router_query_planner::ast::normalization::normalize_operation;
57
use hive_router_query_planner::ast::operation::OperationDefinition;
68
use hive_router_query_planner::consumer_schema::ConsumerSchema;
@@ -101,6 +103,26 @@ fn main() {
101103

102104
println!("{}", operation);
103105
}
106+
"projection" => {
107+
let supergraph_sdl =
108+
std::fs::read_to_string(&args[2]).expect("Unable to read input file");
109+
let parsed_schema = parse_schema(&supergraph_sdl);
110+
let supergraph = SupergraphState::new(&parsed_schema);
111+
let document_text =
112+
std::fs::read_to_string(&args[3]).expect("Unable to read input file");
113+
let parsed_document = parse_operation(&document_text);
114+
let document = normalize_operation(&supergraph, &parsed_document, None).unwrap();
115+
let operation = document.executable_operation();
116+
let consumer_schema = ConsumerSchema::new_from_supergraph(&parsed_schema);
117+
let schema_metadata = consumer_schema.schema_metadata();
118+
119+
let (_, projection_plan) =
120+
FieldProjectionPlan::from_operation(operation, &schema_metadata);
121+
122+
for plan in &projection_plan {
123+
println!("{}", plan);
124+
}
125+
}
104126
_ => {
105127
eprintln!("Unknown command. Available commands: consumer_graph, graph, paths, tree, fetch_graph, plan");
106128
process::exit(1);

lib/executor/benches/executor_benches.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,9 @@ fn project_data_by_operation_test(c: &mut Criterion) {
2424
let normalized_document = normalize_operation(&planner.supergraph, &parsed_document, None)
2525
.expect("Failed to normalize operation");
2626
let normalized_operation = normalized_document.executable_operation();
27-
let (root_type_name, projection_plan) = FieldProjectionPlan::from_operation(
28-
normalized_operation,
29-
&planner.consumer_schema.schema_metadata(),
30-
);
27+
let schema_metadata = &planner.consumer_schema.schema_metadata();
28+
let (root_type_name, projection_plan) =
29+
FieldProjectionPlan::from_operation(normalized_operation, schema_metadata);
3130
let result_as_string = raw_result::get_result_as_string();
3231
let projected_data_as_json: sonic_rs::Value =
3332
sonic_rs::from_slice(result_as_string.as_bytes()).unwrap();
@@ -48,11 +47,12 @@ fn project_data_by_operation_test(c: &mut Criterion) {
4847
&bb_projection_plan,
4948
&None,
5049
result_as_string.len(),
50+
schema_metadata,
5151
)
5252
.unwrap();
5353
black_box(result);
5454
},
55-
criterion::BatchSize::LargeInput,
55+
criterion::BatchSize::SmallInput,
5656
);
5757
});
5858
}

lib/executor/src/execution/error.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use crate::{
88

99
#[derive(thiserror::Error, Debug, Clone, IntoStaticStr)]
1010
pub enum PlanExecutionErrorKind {
11-
#[error("Projection faiure: {0}")]
11+
#[error("Projection failure: {0}")]
1212
#[strum(serialize = "PROJECTION_FAILURE")]
1313
ProjectionFailure(#[from] ProjectionError),
1414

lib/executor/src/execution/plan.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ pub async fn execute_query_plan<'exec, 'req>(
114114
ctx.projection_plan,
115115
ctx.variable_values,
116116
exec_ctx.response_storage.estimate_final_response_size(),
117+
ctx.introspection_context.metadata,
117118
)
118119
.with_plan_context(LazyPlanContext {
119120
subgraph_name: || None,

lib/executor/src/introspection/schema.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ pub struct SchemaMetadata {
2121
pub object_types: HashSet<String>,
2222
pub scalar_types: HashSet<String>,
2323
pub union_types: HashSet<String>,
24+
pub interface_types: HashSet<String>,
2425
}
2526

2627
impl SchemaMetadata {
@@ -36,6 +37,10 @@ impl SchemaMetadata {
3637
self.union_types.contains(name)
3738
}
3839

40+
pub fn is_interface_type(&self, name: &str) -> bool {
41+
self.interface_types.contains(name)
42+
}
43+
3944
pub fn get_type_fields(&self, type_name: &str) -> Option<&HashMap<String, FieldTypeInfo>> {
4045
self.type_fields.get(type_name)
4146
}
@@ -88,6 +93,7 @@ impl SchemaWithMetadata for ConsumerSchema {
8893
]);
8994
let mut object_types: HashSet<String> = HashSet::default();
9095
let mut union_types: HashSet<String> = HashSet::default();
96+
let mut interface_types: HashSet<String> = HashSet::default();
9197

9298
for definition in &self.document.definitions {
9399
match definition {
@@ -123,6 +129,7 @@ impl SchemaWithMetadata for ConsumerSchema {
123129
}
124130
Definition::TypeDefinition(TypeDefinition::Interface(interface_type)) => {
125131
let name = interface_type.name.to_string();
132+
interface_types.insert(name.clone());
126133
let mut fields = HashMap::default();
127134
for field in &interface_type.fields {
128135
let field_type_name = field.field_type.type_name();
@@ -183,6 +190,7 @@ impl SchemaWithMetadata for ConsumerSchema {
183190
object_types,
184191
scalar_types,
185192
union_types,
193+
interface_types,
186194
}
187195
}
188196
}

lib/executor/src/projection/error.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,11 @@ pub enum ProjectionError {
66
ExtensionsSerializationFailure(String),
77
#[error("Failed to serialize a custom scalar: {0}")]
88
CustomScalarSerializationFailure(String),
9+
#[error("Type '{0}' not found in schema")]
10+
MissingType(String),
11+
#[error("Field '{field_name}' not found on type '{type_name}' in schema")]
12+
MissingField {
13+
field_name: String,
14+
type_name: String,
15+
},
916
}

0 commit comments

Comments
 (0)