diff --git a/.changeset/fix-cond.md b/.changeset/fix-cond.md new file mode 100644 index 000000000..770fc0bb6 --- /dev/null +++ b/.changeset/fix-cond.md @@ -0,0 +1,7 @@ +--- +node-addon: patch +router: patch +query-planner: patch +--- + +Fixed an issue where `@skip` and `@include` directives were incorrectly removed from the initial Fetch of the Query Plan. diff --git a/lib/query-planner/src/ast/merge_path.rs b/lib/query-planner/src/ast/merge_path.rs index b52bf1026..b478c0b9e 100644 --- a/lib/query-planner/src/ast/merge_path.rs +++ b/lib/query-planner/src/ast/merge_path.rs @@ -13,6 +13,21 @@ pub enum Condition { Include(String), } +impl Condition { + pub fn to_skip_if(&self) -> Option { + match self { + Condition::Skip(var) => Some(var.clone()), + _ => None, + } + } + pub fn to_include_if(&self) -> Option { + match self { + Condition::Include(var) => Some(var.clone()), + _ => None, + } + } +} + impl Display for Condition { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { diff --git a/lib/query-planner/src/ast/selection_item.rs b/lib/query-planner/src/ast/selection_item.rs index 6c036a279..db3b5cac7 100644 --- a/lib/query-planner/src/ast/selection_item.rs +++ b/lib/query-planner/src/ast/selection_item.rs @@ -1,5 +1,6 @@ use crate::{ - ast::normalization::utils::extract_type_condition, utils::pretty_display::PrettyDisplay, + ast::normalization::utils::extract_type_condition, + utils::pretty_display::{get_indent, PrettyDisplay}, }; use graphql_parser::query as query_ast; @@ -49,7 +50,10 @@ impl PrettyDisplay for SelectionItem { SelectionItem::InlineFragment(fragment_selection) => { fragment_selection.pretty_fmt(f, depth)? } - SelectionItem::FragmentSpread(name) => write!(f, "...{}", name)?, + SelectionItem::FragmentSpread(name) => { + let indent = get_indent(depth); + writeln!(f, "{indent}...{}", name)? + } } Ok(()) diff --git a/lib/query-planner/src/ast/selection_set.rs b/lib/query-planner/src/ast/selection_set.rs index c2e7787c1..85ac46e8b 100644 --- a/lib/query-planner/src/ast/selection_set.rs +++ b/lib/query-planner/src/ast/selection_set.rs @@ -303,6 +303,8 @@ impl PrettyDisplay for FieldSelection { None => String::new(), }; + write!(f, "{indent}{}{}{}", alias_str, self.name, args_str)?; + if let Some(skip_if) = &self.skip_if { write!(f, " @skip(if: ${})", skip_if)?; } @@ -312,10 +314,10 @@ impl PrettyDisplay for FieldSelection { } if self.is_leaf() { - return writeln!(f, "{indent}{}{}{}", alias_str, self.name, args_str); + return writeln!(f); } - writeln!(f, "{indent}{}{}{} {{", alias_str, self.name, args_str)?; + writeln!(f, " {{")?; self.selections.pretty_fmt(f, depth + 1)?; writeln!(f, "{indent}}}") } diff --git a/lib/query-planner/src/graph/edge.rs b/lib/query-planner/src/graph/edge.rs index 78cd83be4..9fb843caa 100644 --- a/lib/query-planner/src/graph/edge.rs +++ b/lib/query-planner/src/graph/edge.rs @@ -159,6 +159,10 @@ pub enum Edge { EntityMove(EntityMove), /// join__implements AbstractMove(String), + /// A special edge representing a case where an inline fragment is used with a condition, + /// and we don't really move anywhere, but we need to ensure that + /// the condition is preserved when planning the query. + Selfie(String), /// Represents a special case where going from @interfaceObject /// to an object type due to the `__typename` field usage, /// or usage of a type condition (fragment), @@ -227,6 +231,7 @@ impl Edge { Self::FieldMove(fm) => &fm.name, Self::EntityMove(EntityMove { key, .. }) => key, Self::AbstractMove(id) => id, + Self::Selfie(_) => "selfie", Self::SubgraphEntrypoint { name, .. } => &name.0, Self::InterfaceObjectTypeMove(InterfaceObjectTypeMove { object_type_name, .. @@ -264,6 +269,7 @@ impl Display for Edge { Edge::SubgraphEntrypoint { name, .. } => write!(f, "{}", name.0), Edge::EntityMove(EntityMove { .. }) => write!(f, "🔑"), Edge::AbstractMove(_) => write!(f, "🔮"), + Edge::Selfie(_) => write!(f, "🤳"), Edge::FieldMove(field_move) => write!(f, "{}", field_move.name), Edge::InterfaceObjectTypeMove(m) => write!(f, "🔎 {}", m.object_type_name), }?; @@ -318,6 +324,7 @@ impl Debug for Edge { write!(f, "🔑 {}", key) } Edge::AbstractMove(name) => write!(f, "🔮 {}", name), + Edge::Selfie(_) => write!(f, "🤳"), Edge::InterfaceObjectTypeMove(m) => write!(f, "🔎 {}", m.object_type_name), } } diff --git a/lib/query-planner/src/graph/mod.rs b/lib/query-planner/src/graph/mod.rs index a0e189349..bf0edba0e 100644 --- a/lib/query-planner/src/graph/mod.rs +++ b/lib/query-planner/src/graph/mod.rs @@ -643,6 +643,18 @@ impl Graph { ); } + trace!( + "[x] Creating self-referencing edge for '{}/{}'", + def_name, + graph_id + ); + let head = self.upsert_node(Node::new_node( + def_name, + state.resolve_graph_id(graph_id)?, + state.is_interface_object_in_subgraph(def_name, graph_id), + )); + self.upsert_edge(head, head, Edge::Selfie(def_name.clone())); + for (field_name, field_definition) in definition.fields().iter() { let (is_available, maybe_join_field) = FederationRules::check_field_subgraph_availability( diff --git a/lib/query-planner/src/graph/tests.rs b/lib/query-planner/src/graph/tests.rs index cd0e480eb..bfea8de64 100644 --- a/lib/query-planner/src/graph/tests.rs +++ b/lib/query-planner/src/graph/tests.rs @@ -287,8 +287,8 @@ mod graph_tests { .assert_field_edge("createdBy", "User/products") .assert_field_edge("__typename", "String/products") .no_field_edge("reviews"); - assert_eq!(incoming.edges.len(), 2); - assert_eq!(outgoing.edges.len(), 11); + assert_eq!(incoming.edges.len(), 3); + assert_eq!(outgoing.edges.len(), 12); // requires preserves selection set in the graph let outgoing = find_node(&graph, "Product/inventory").1; @@ -315,8 +315,8 @@ mod graph_tests { assert_eq!(graph.root_subscription_node(), None); let (incoming, outgoing) = find_node(&graph, "Product/products"); - assert_eq!(incoming.edges.len(), 11); - assert_eq!(outgoing.edges.len(), 15); + assert_eq!(incoming.edges.len(), 14); + assert_eq!(outgoing.edges.len(), 18); incoming .assert_key_edge("id", "Product/inventory") diff --git a/lib/query-planner/src/planner/fetch/fetch_graph.rs b/lib/query-planner/src/planner/fetch/fetch_graph.rs index 0dce7e2ea..b01d21294 100644 --- a/lib/query-planner/src/planner/fetch/fetch_graph.rs +++ b/lib/query-planner/src/planner/fetch/fetch_graph.rs @@ -186,6 +186,26 @@ impl FetchGraph { Ok(()) } + + /// Checks whether the given step is an ancestor of a step that has the given condition + pub fn is_ancestor_of_condition(&self, step_index: NodeIndex, condition: &Condition) -> bool { + let mut bfs = Bfs::new(&self.graph, step_index); + + while let Some(current_index) = bfs.next(&self.graph) { + let current_step = match self.get_step_data(current_index) { + Ok(s) => s, + Err(_) => continue, + }; + + if let Some(step_condition) = ¤t_step.condition { + if step_condition == condition { + return true; + } + } + } + + false + } } impl Display for FetchGraph { @@ -1004,6 +1024,92 @@ fn process_subgraph_entrypoint_edge( ) } +// TODO: simplfy args +#[allow(clippy::too_many_arguments)] +#[instrument(level = "trace",skip_all, fields( + parent_fetch_step_index = parent_fetch_step_index.index(), + requiring_fetch_step_index = requiring_fetch_step_index.map(|f| f.index()), + type_name = target_type_name, + response_path = response_path.to_string(), + fetch_path = fetch_path.to_string() +))] +fn process_selfie_edge( + graph: &Graph, + fetch_graph: &mut FetchGraph, + override_context: &PlannerOverrideContext, + query_node: &QueryTreeNode, + parent_fetch_step_index: NodeIndex, + requiring_fetch_step_index: Option, + response_path: &MergePath, + fetch_path: &MergePath, + target_type_name: &String, + condition: Option<&Condition>, +) -> Result, FetchGraphError> { + let is_ancestor_of_condition = match condition { + Some(c) => fetch_graph.is_ancestor_of_condition(parent_fetch_step_index, c), + None => false, + }; + + let parent_fetch_step = fetch_graph.get_step_data_mut(parent_fetch_step_index)?; + trace!( + "starting an inline fragment for type '{}' to fetch step [{}]", + parent_fetch_step_index.index(), + target_type_name, + ); + parent_fetch_step.output.add_at_path( + &TypeAwareSelection { + selection_set: SelectionSet { + items: vec![SelectionItem::InlineFragment(InlineFragmentSelection { + type_condition: target_type_name.clone(), + selections: SelectionSet::default(), + skip_if: condition.and_then(|c| { + if is_ancestor_of_condition { + None + } else { + c.to_skip_if() + } + }), + include_if: condition.and_then(|c| { + if is_ancestor_of_condition { + None + } else { + c.to_include_if() + } + }), + })], + }, + type_name: target_type_name.clone(), + }, + fetch_path.clone(), + false, + )?; + + let segment_condition = if is_ancestor_of_condition { + None + } else { + condition.cloned() + }; + let child_response_path = response_path.push(Segment::Cast( + target_type_name.clone(), + segment_condition.clone(), + )); + let child_fetch_path = + fetch_path.push(Segment::Cast(target_type_name.clone(), segment_condition)); + + process_children_for_fetch_steps( + graph, + fetch_graph, + override_context, + query_node, + parent_fetch_step_index, + &child_response_path, + &child_fetch_path, + requiring_fetch_step_index, + condition, + false, + ) +} + // TODO: simplfy args #[allow(clippy::too_many_arguments)] #[instrument(level = "trace",skip_all, fields( @@ -1111,6 +1217,25 @@ fn process_plain_field_edge( fetch_graph.connect(parent_fetch_step_index, requiring_fetch_step_index); } + // If we are inserting into "... | [Product] @include(if: $bool)", we don't need the condition on the field. + let condition_in_path = if let Some(condition) = condition { + fetch_path.inner.iter().any(|segment| { + matches!( + segment, + Segment::Cast(_, Some(c)) | Segment::Field(_, _, Some(c)) if c == condition + ) + }) + } else { + false + }; + + let ancestor_of_condition = match condition { + Some(c) => fetch_graph.is_ancestor_of_condition(parent_fetch_step_index, c), + None => false, + }; + + let should_strip_condition = condition_in_path || ancestor_of_condition; + let parent_fetch_step = fetch_graph.get_step_data_mut(parent_fetch_step_index)?; trace!( "adding output field '{}' to fetch step [{}]", @@ -1126,8 +1251,20 @@ fn process_plain_field_edge( alias: query_node.selection_alias().map(|a| a.to_string()), selections: SelectionSet::default(), arguments: query_node.selection_arguments().cloned(), - skip_if: None, - include_if: None, + skip_if: condition.and_then(|c| { + if should_strip_condition { + None + } else { + c.to_skip_if() + } + }), + include_if: condition.and_then(|c| { + if should_strip_condition { + None + } else { + c.to_include_if() + } + }), })], }, type_name: field_move.type_name.to_string(), @@ -1137,6 +1274,11 @@ fn process_plain_field_edge( )?; let child_segment = query_node.selection_alias().unwrap_or(&field_move.name); + let segment_condition = if ancestor_of_condition { + None + } else { + condition.cloned() + }; let segment_args_hash = query_node .selection_arguments() .map(|a| a.hash_u64()) @@ -1144,12 +1286,12 @@ fn process_plain_field_edge( let mut child_response_path = response_path.push(Segment::Field( child_segment.to_string(), segment_args_hash, - None, + segment_condition.clone(), )); let mut child_fetch_path = fetch_path.push(Segment::Field( child_segment.to_string(), segment_args_hash, - None, + segment_condition, )); if field_move.is_list { @@ -1595,6 +1737,18 @@ fn process_query_node( created_from_requires, ), }, + Edge::Selfie(type_name) => process_selfie_edge( + graph, + fetch_graph, + override_context, + query_node, + parent_fetch_step_index, + requiring_fetch_step_index, + response_path, + fetch_path, + type_name, + condition, + ), Edge::AbstractMove(type_name) => process_abstract_edge( graph, fetch_graph, diff --git a/lib/query-planner/src/planner/fetch/optimize/utils.rs b/lib/query-planner/src/planner/fetch/optimize/utils.rs index 07ddb85d5..72599ebc8 100644 --- a/lib/query-planner/src/planner/fetch/optimize/utils.rs +++ b/lib/query-planner/src/planner/fetch/optimize/utils.rs @@ -8,7 +8,8 @@ use tracing::{instrument, trace}; use crate::{ ast::{ - merge_path::Condition, selection_item::SelectionItem, + merge_path::{Condition, Segment}, + selection_item::SelectionItem, selection_set::InlineFragmentSelection, }, planner::fetch::{ @@ -46,25 +47,33 @@ pub(crate) fn perform_fetch_step_merge( // We can't do it when both are entity calls fetch steps. other.output.add(&other.input); - let old_selection_set = other.output.selection_set.clone(); - match condition { - Condition::Include(var_name) => { - other.output.selection_set.items = - vec![SelectionItem::InlineFragment(InlineFragmentSelection { - type_condition: other.output.type_name.clone(), - selections: old_selection_set, - skip_if: None, - include_if: Some(var_name), - })]; - } - Condition::Skip(var_name) => { - other.output.selection_set.items = - vec![SelectionItem::InlineFragment(InlineFragmentSelection { - type_condition: other.output.type_name.clone(), - selections: old_selection_set, - skip_if: Some(var_name), - include_if: None, - })]; + // Check if the condition is already enforced by the path + let condition_redundant = matches!( + other.response_path.last(), + Some(Segment::Cast(_, Some(c)) | Segment::Field(_, _, Some(c))) if c == &condition + ); + + if !condition_redundant { + let old_selection_set = other.output.selection_set.clone(); + match condition { + Condition::Include(var_name) => { + other.output.selection_set.items = + vec![SelectionItem::InlineFragment(InlineFragmentSelection { + type_condition: other.output.type_name.clone(), + selections: old_selection_set, + skip_if: None, + include_if: Some(var_name), + })]; + } + Condition::Skip(var_name) => { + other.output.selection_set.items = + vec![SelectionItem::InlineFragment(InlineFragmentSelection { + type_condition: other.output.type_name.clone(), + selections: old_selection_set, + skip_if: Some(var_name), + include_if: None, + })]; + } } } } else if me.is_entity_call() && other.condition.is_some() { diff --git a/lib/query-planner/src/planner/walker/mod.rs b/lib/query-planner/src/planner/walker/mod.rs index 4f4c42d60..5e649359c 100644 --- a/lib/query-planner/src/planner/walker/mod.rs +++ b/lib/query-planner/src/planner/walker/mod.rs @@ -9,6 +9,7 @@ use std::collections::VecDeque; use crate::{ ast::{ + merge_path::Condition, operation::OperationDefinition, selection_item::SelectionItem, selection_set::{FieldSelection, InlineFragmentSelection, SelectionSet}, @@ -18,7 +19,7 @@ use crate::{ node::Node, Graph, }, - planner::walker::pathfinder::NavigationTarget, + planner::walker::pathfinder::{find_self_referencing_direct_path, NavigationTarget}, state::supergraph_state::{OperationKind, SupergraphState}, utils::cancellation::CancellationToken, }; @@ -229,12 +230,56 @@ fn process_inline_fragment<'a>( }; if tail_type_name == &fragment.type_condition { + // It's the same type and no conditions are applied, we can skip the fragment processing + // and go directly to its selections. + if fragment.include_if.is_none() && fragment.skip_if.is_none() { + return process_selection_set( + graph, + supergraph, + override_context, + &fragment.selections, + paths, + fields_to_resolve_locally, + cancellation_token, + ); + } + + // Looks like the fragment has conditions, we need to process them differently. + // We aim to preserve the inline fragment due to conditions, instead of eliminating it, + // and jumping straight to its selections. + let condition: Option = fragment.into(); + + let mut next_paths: Vec = Vec::with_capacity(paths.len()); + for path in paths { + let path_span = span!( + Level::TRACE, + "explore_path", + path = path.pretty_print(graph) + ); + let _enter = path_span.enter(); + + // Find a direct path that references the same type as the current tail, + let direct_path = find_self_referencing_direct_path( + graph, + override_context, + path, + &fragment.type_condition, + condition.as_ref().expect("Condition should be present"), + cancellation_token, + )?; + + trace!("advanced: {}", path.pretty_print(graph)); + + next_paths.push(direct_path); + } + + // Now process the selections under the fragment using the advanced paths return process_selection_set( graph, supergraph, override_context, &fragment.selections, - paths, + &next_paths, fields_to_resolve_locally, cancellation_token, ); diff --git a/lib/query-planner/src/planner/walker/path.rs b/lib/query-planner/src/planner/walker/path.rs index 747f8f745..a5fce4c1c 100644 --- a/lib/query-planner/src/planner/walker/path.rs +++ b/lib/query-planner/src/planner/walker/path.rs @@ -20,7 +20,6 @@ use crate::{ pub struct SelectionAttributes { pub alias: Option, pub arguments: Option, - // TODO: Add custom directives, @skip/@include conditions } impl PartialEq for SelectionAttributes { diff --git a/lib/query-planner/src/planner/walker/pathfinder.rs b/lib/query-planner/src/planner/walker/pathfinder.rs index f05bd6d32..b3bc7bc72 100644 --- a/lib/query-planner/src/planner/walker/pathfinder.rs +++ b/lib/query-planner/src/planner/walker/pathfinder.rs @@ -280,6 +280,41 @@ fn try_advance_direct_path<'a>( } } +pub fn find_self_referencing_direct_path( + graph: &Graph, + override_context: &PlannerOverrideContext, + path: &OperationPath, + type_name: &str, + condition: &Condition, + cancellation_token: &CancellationToken, +) -> Result { + let path_tail_index = path.tail(); + + for edge_ref in graph + .edges_from(path_tail_index) + .filter(move |e| match e.weight() { + Edge::Selfie(t) => t == type_name, + _ => false, + }) + { + if let Some(new_path) = try_advance_direct_path( + graph, + path, + override_context, + &edge_ref, + &NavigationTarget::ConcreteType(type_name, Some(condition.clone())), + cancellation_token, + )? { + trace!("Finished finding direct path, found one",); + return Ok(new_path); + } + } + + trace!("Finished finding direct path, found none",); + + Err(WalkOperationError::NoPathsFound(type_name.to_string())) +} + #[instrument(level = "trace", skip_all, fields( path = path.pretty_print(graph), current_cost = path.cost, diff --git a/lib/query-planner/src/tests/alias.rs b/lib/query-planner/src/tests/alias.rs index 25d42f6c1..ea1d6d3d6 100644 --- a/lib/query-planner/src/tests/alias.rs +++ b/lib/query-planner/src/tests/alias.rs @@ -55,11 +55,13 @@ fn circular_reference_interface() -> Result<(), Box> { samePriceProduct { __typename ... on Book { - ...a } + ...a + } samePriceProduct { __typename ... on Book { - ...a } + ...a + } } } ... on Book { @@ -553,13 +555,15 @@ fn nested_internal_mismatch_between_fields() -> Result<(), Box> { id name similarAccounts { - ...a } + ...a + } } ... on Admin { _internal_qp_alias_0: id name similarAccounts { - ...a } + ...a + } } } } @@ -827,13 +831,15 @@ fn deeply_nested_internal_mismatch_between_fields() -> Result<(), Box id name similarAccounts { - ...a } + ...a + } } ... on Admin { _internal_qp_alias_0: id name similarAccounts { - ...a } + ...a + } } } } @@ -843,13 +849,15 @@ fn deeply_nested_internal_mismatch_between_fields() -> Result<(), Box id name similarAccounts { - ...b } + ...b + } } ... on Admin { _internal_qp_alias_0: id name similarAccounts { - ...b } + ...b + } } } fragment b on Account { @@ -1207,12 +1215,14 @@ fn deeply_nested_no_conflicts() -> Result<(), Box> { ... on User { name similarAccounts { - ...a } + ...a + } } ... on Admin { name similarAccounts { - ...a } + ...a + } } } } @@ -1221,12 +1231,14 @@ fn deeply_nested_no_conflicts() -> Result<(), Box> { ... on User { name similarAccounts { - ...b } + ...b + } } ... on Admin { name similarAccounts { - ...b } + ...b + } } } fragment b on Account { diff --git a/lib/query-planner/src/tests/arguments.rs b/lib/query-planner/src/tests/arguments.rs index a46a56c0f..a21f8b988 100644 --- a/lib/query-planner/src/tests/arguments.rs +++ b/lib/query-planner/src/tests/arguments.rs @@ -1855,9 +1855,11 @@ fn arguments_with_aliases() -> Result<(), Box> { Fetch(service: "d") { { firstProduct: productFromD(id: "1") { - ...a } + ...a + } secondProduct: productFromD(id: "2") { - ...a } + ...a + } } fragment a on Product { __typename @@ -2012,9 +2014,11 @@ fn arguments_variables_mixed() -> Result<(), Box> { Fetch(service: "d") { query ($secondProductId:ID!) { firstProduct: productFromD(id: "1") { - ...a } + ...a + } secondProduct: productFromD(id: $secondProductId) { - ...a } + ...a + } } fragment a on Product { __typename diff --git a/lib/query-planner/src/tests/include_skip.rs b/lib/query-planner/src/tests/include_skip.rs index afce98320..6e2db65cb 100644 --- a/lib/query-planner/src/tests/include_skip.rs +++ b/lib/query-planner/src/tests/include_skip.rs @@ -82,6 +82,84 @@ fn include_basic_test() -> Result<(), Box> { Ok(()) } +#[test] +fn include_fragment_test() -> Result<(), Box> { + init_logger(); + let document = parse_operation( + r#" + query ($bool: Boolean) { + product { + price + ... on Product @include(if: $bool) { + neverCalledInclude + } + } + } + "#, + ); + let query_plan = build_query_plan( + "fixture/tests/simple-include-skip.supergraph.graphql", + document, + )?; + + insta::assert_snapshot!(format!("{}", query_plan), @r#" + QueryPlan { + Sequence { + Fetch(service: "a") { + query ($bool:Boolean) { + product { + price + ... on Product @include(if: $bool) { + __typename + id + price + } + } + } + }, + Include(if: $bool) { + Sequence { + Flatten(path: "product|[Product]") { + Fetch(service: "b") { + { + ... on Product { + __typename + price + id + } + } => + { + ... on Product { + isExpensive + } + } + }, + }, + Flatten(path: "product|[Product]") { + Fetch(service: "c") { + { + ... on Product { + __typename + isExpensive + id + } + } => + { + ... on Product { + neverCalledInclude + } + } + }, + }, + }, + }, + }, + }, + "#); + + Ok(()) +} + #[test] fn skip_basic_test() -> Result<(), Box> { init_logger(); @@ -159,3 +237,201 @@ fn skip_basic_test() -> Result<(), Box> { Ok(()) } + +#[test] +fn include_at_root_fetch_test() -> Result<(), Box> { + init_logger(); + let document = parse_operation( + r#" + query ($bool: Boolean) { + product { + id + price @include(if: $bool) + } + } + "#, + ); + let query_plan = build_query_plan( + "fixture/tests/simple-include-skip.supergraph.graphql", + document, + )?; + + insta::assert_snapshot!(format!("{}", query_plan), @r#" + QueryPlan { + Fetch(service: "a") { + query ($bool:Boolean) { + product { + id + price @include(if: $bool) + } + } + }, + }, + "#); + + Ok(()) +} + +#[test] +fn include_fragment_at_root_fetch_test() -> Result<(), Box> { + init_logger(); + let document = parse_operation( + r#" + query ($bool: Boolean) { + product { + id + ... on Product @include(if: $bool) { + price + } + } + } + "#, + ); + let query_plan = build_query_plan( + "fixture/tests/simple-include-skip.supergraph.graphql", + document, + )?; + + insta::assert_snapshot!(format!("{}", query_plan), @r#" + QueryPlan { + Fetch(service: "a") { + query ($bool:Boolean) { + product { + id + ... on Product @include(if: $bool) { + price + } + } + } + }, + }, + "#); + + Ok(()) +} + +#[test] +fn include_interface_at_root_fetch_test() -> Result<(), Box> { + init_logger(); + let document = parse_operation( + r#" + query ($bool: Boolean) { + accounts { + id + name @include(if: $bool) + } + } + "#, + ); + let query_plan = build_query_plan( + "fixture/tests/simple-interface-object.supergraph.graphql", + document, + )?; + + insta::assert_snapshot!(format!("{}", query_plan), @r#" + QueryPlan { + Fetch(service: "b") { + query ($bool:Boolean) { + accounts { + id + name @include(if: $bool) + } + } + }, + }, + "#); + + Ok(()) +} + +#[test] +fn include_interface_fragment_at_root_fetch_test() -> Result<(), Box> { + init_logger(); + let document = parse_operation( + r#" + query ($bool: Boolean) { + accounts { + id + ... on Account @include(if: $bool) { + name + } + } + } + "#, + ); + let query_plan = build_query_plan( + "fixture/tests/simple-interface-object.supergraph.graphql", + document, + )?; + + insta::assert_snapshot!(format!("{}", query_plan), @r#" + QueryPlan { + Fetch(service: "b") { + query ($bool:Boolean) { + accounts { + id + ... on Account @include(if: $bool) { + name + } + } + } + }, + }, + "#); + + Ok(()) +} + +#[test] +fn include_union_fragment_at_root_fetch_test() -> Result<(), Box> { + init_logger(); + let document = parse_operation( + r#" + query ($bool: Boolean) { + review { + ... on UserReview @include(if: $bool) { + product { + id + } + } + ... on AnonymousReview @include(if: $bool) { + product { + id + } + } + } + } + "#, + ); + let query_plan = build_query_plan( + "fixture/tests/union-overfetching.supergraph.graphql", + document, + )?; + + insta::assert_snapshot!(format!("{}", query_plan), @r#" + QueryPlan { + Fetch(service: "a") { + query ($bool:Boolean) { + review { + __typename + ... on UserReview { + product @include(if: $bool) { + ...a + } + } + ... on AnonymousReview { + product @include(if: $bool) { + ...a + } + } + } + } + fragment a on Product { + id + } + }, + }, + "#); + + Ok(()) +} diff --git a/lib/query-planner/src/tests/interface.rs b/lib/query-planner/src/tests/interface.rs index 215639d85..9ccab371c 100644 --- a/lib/query-planner/src/tests/interface.rs +++ b/lib/query-planner/src/tests/interface.rs @@ -31,9 +31,11 @@ fn node_query_with_aliases_on_interface_field() -> Result<(), Box> { Fetch(service: "a") { { account: node(id: "a1") { - ...a } + ...a + } chat: node(id: "c1") { - ...a } + ...a + } } fragment a on Node { __typename @@ -612,9 +614,11 @@ fn requires_on_field_with_args_test() -> Result<(), Box> { Fetch(service: "products") { { book: similar(id: "p1") { - ...a } + ...a + } magazine: similar(id: "p2") { - ...a } + ...a + } } fragment a on Product { id @@ -624,14 +628,16 @@ fn requires_on_field_with_args_test() -> Result<(), Box> { sku id dimensions { - ...b } + ...b + } } ... on Magazine { __typename sku id dimensions { - ...b } + ...b + } } } fragment b on ProductDimension { diff --git a/lib/query-planner/src/tests/issues.rs b/lib/query-planner/src/tests/issues.rs index ceba035d3..8e899ff86 100644 --- a/lib/query-planner/src/tests/issues.rs +++ b/lib/query-planner/src/tests/issues.rs @@ -44,12 +44,14 @@ fn issue_281_test() -> Result<(), Box> { ... on AnonymousReview { __typename product { - ...a } + ...a + } } ... on UserReview { __typename product { - ...a } + ...a + } } } } diff --git a/lib/query-planner/src/tests/union.rs b/lib/query-planner/src/tests/union.rs index 34ec7beaa..5db2d9eae 100644 --- a/lib/query-planner/src/tests/union.rs +++ b/lib/query-planner/src/tests/union.rs @@ -329,9 +329,11 @@ fn union_member_entity_call_many() -> Result<(), Box> { { viewer { media { - ...a } + ...a + } book { - ...a } + ...a + } } } fragment a on ViewerMedia { @@ -345,9 +347,11 @@ fn union_member_entity_call_many() -> Result<(), Box> { { viewer { media { - ...a } + ...a + } book { - ...a } + ...a + } song { __typename ... on Song { @@ -431,11 +435,13 @@ fn union_overfetching_test() -> Result<(), Box> { __typename ... on AnonymousReview { product { - ...a } + ...a + } } ... on UserReview { product { - ...a } + ...a + } } } }