From a9b8d0ed0a5a11b01982b2886f884f1a0c8f3f7a Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Wed, 29 May 2024 14:19:14 -0700 Subject: [PATCH] FED-191: [Operation Processing] port `operation.optimize` (#5207) Co-authored-by: Iryna Shestak --- apollo-federation/src/query_plan/display.rs | 2 +- .../src/query_plan/fetch_dependency_graph.rs | 8 +- apollo-federation/src/query_plan/operation.rs | 152 +-- apollo-federation/src/query_plan/optimize.rs | 928 +++++++++++++++++- .../src/query_plan/query_planner.rs | 193 +++- .../query_plan/build_query_plan_tests.rs | 4 +- .../named_fragments_preservation.rs | 203 ++-- .../operation_optimization_tests.rs | 350 ------- ...option_reuse_query_fragments_false.graphql | 2 +- ..._option_reuse_query_fragments_true.graphql | 2 +- 10 files changed, 1307 insertions(+), 537 deletions(-) diff --git a/apollo-federation/src/query_plan/display.rs b/apollo-federation/src/query_plan/display.rs index 37042d08d7..e1b8a7fa96 100644 --- a/apollo-federation/src/query_plan/display.rs +++ b/apollo-federation/src/query_plan/display.rs @@ -322,7 +322,7 @@ fn write_operation( )? } for fragment in operation_document.fragments.values() { - state.new_line()?; + state.write("\n\n")?; // new line without indentation (since `fragment` adds indentation) state.write( fragment .serialize() diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index 271cf95dc0..a943a7ae7a 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -1301,9 +1301,11 @@ impl FetchDependencyGraphNode { &operation_name, )? }; - let fragments = fragments - .map(|rebased| rebased.for_subgraph(self.subgraph_name.clone(), subgraph_schema)); - operation.optimize(fragments, Default::default()); + if let Some(fragments) = fragments + .map(|rebased| rebased.for_subgraph(self.subgraph_name.clone(), subgraph_schema)) + { + operation.optimize(fragments)?; + } let operation_document = operation.try_into()?; let node = super::PlanNode::Fetch(Box::new(super::FetchNode { diff --git a/apollo-federation/src/query_plan/operation.rs b/apollo-federation/src/query_plan/operation.rs index d2946144d3..d79b48061f 100644 --- a/apollo-federation/src/query_plan/operation.rs +++ b/apollo-federation/src/query_plan/operation.rs @@ -1049,14 +1049,14 @@ impl Fragment { } // PORT NOTE: in JS code this is stored on the fragment - fn fragment_usages(&self) -> HashMap { + pub(crate) fn fragment_usages(&self) -> HashMap { let mut usages = HashMap::new(); self.selection_set.collect_used_fragment_names(&mut usages); usages } // PORT NOTE: in JS code this is stored on the fragment - fn collect_used_fragment_names(&self, aggregator: &mut HashMap) { + pub(crate) fn collect_used_fragment_names(&self, aggregator: &mut HashMap) { self.selection_set.collect_used_fragment_names(aggregator) } @@ -1069,6 +1069,7 @@ mod normalized_field_selection { use std::collections::HashSet; use std::sync::Arc; + use apollo_compiler::ast; use apollo_compiler::executable; use apollo_compiler::executable::Name; use apollo_compiler::Node; @@ -1296,6 +1297,10 @@ mod normalized_field_selection { self.alias.clone().unwrap_or_else(|| self.name().clone()) } + fn output_ast_type(&self) -> Result<&ast::Type, FederationError> { + Ok(&self.field_position.get(self.schema.schema())?.ty) + } + pub(crate) fn output_base_type(&self) -> Result { let definition = self.field_position.get(self.schema.schema())?; self.schema @@ -1856,27 +1861,6 @@ pub(crate) use normalized_inline_fragment_selection::InlineFragmentSelection; use crate::schema::position::INTROSPECTION_TYPENAME_FIELD_NAME; -// TODO(@goto-bus-stop): merge this with the other Operation impl block. -impl Operation { - pub(crate) fn optimize( - &mut self, - fragments: Option<&NamedFragments>, - min_usages_to_optimize: Option, - ) { - let min_usages_to_optimize = min_usages_to_optimize.unwrap_or(2); - let Some(fragments) = fragments else { return }; - if fragments.is_empty() { - return; - } - assert!( - min_usages_to_optimize >= 1, - "Expected 'min_usages_to_optimize' to be at least 1, but got {min_usages_to_optimize}" - ); - - todo!(); // TODO: port JS `Operation.optimize` from `operations.ts` - } -} - /// A simple MultiMap implementation using IndexMap with Vec as its value type. /// - Preserves the insertion order of keys and values. struct MultiIndexMap(IndexMap>); @@ -1910,7 +1894,7 @@ where /// the return type of `lazy_map` function's `mapper` closure argument #[derive(derive_more::From)] -enum SelectionMapperReturn { +pub(crate) enum SelectionMapperReturn { None, Selection(Selection), SelectionList(Vec), @@ -2348,6 +2332,7 @@ impl SelectionSet { } else { // convert to inline fragment let expanded = InlineFragmentSelection::from_fragment_spread_selection( + selection_set.type_position.clone(), // the parent type of this inline selection spread_selection, )?; destination.push(Selection::InlineFragment(Arc::new(expanded))); @@ -2619,8 +2604,9 @@ impl SelectionSet { // version, but its Rust version doesn't use `lazy_map`. // PORT_NOTE #2: Taking ownership of `self` in this method was considered. However, calling // `Arc::make_mut` on the `Arc` fields of `self` didn't seem better than cloning Arc instances. - fn lazy_map( + pub(crate) fn lazy_map( &self, + named_fragments: &NamedFragments, mut mapper: impl FnMut(&Selection) -> Result, ) -> Result { let mut iter = self.selections.values(); @@ -2669,12 +2655,12 @@ impl SelectionSet { &self.schema, &self.type_position, updated_selections.values().map(|v| v.iter()), - /*named_fragments*/ &Default::default(), + named_fragments, ) } pub(crate) fn add_back_typename_in_attachments(&self) -> Result { - self.lazy_map(|selection| { + self.lazy_map(/*named_fragments*/ &Default::default(), |selection| { let selection_element = selection.element()?; let updated = selection .map_selection_set(|ss| ss.add_back_typename_in_attachments().map(Some))?; @@ -2870,7 +2856,7 @@ impl SelectionSet { Ok(()) } - fn collect_used_fragment_names(&self, aggregator: &mut HashMap) { + pub(crate) fn collect_used_fragment_names(&self, aggregator: &mut HashMap) { self.selections .iter() .for_each(|(_, s)| s.collect_used_fragment_names(aggregator)); @@ -4012,13 +3998,14 @@ impl InlineFragmentSelection { } pub(crate) fn from_fragment_spread_selection( + parent_type_position: CompositeTypeDefinitionPosition, fragment_spread_selection: &Arc, ) -> Result { let fragment_spread_data = fragment_spread_selection.spread.data(); Ok(InlineFragmentSelection { inline_fragment: InlineFragment::new(InlineFragmentData { schema: fragment_spread_data.schema.clone(), - parent_type_position: fragment_spread_data.type_condition_position.clone(), + parent_type_position, type_condition_position: Some(fragment_spread_data.type_condition_position.clone()), directives: fragment_spread_data.directives.clone(), selection_id: SelectionId::new(), @@ -4029,6 +4016,26 @@ impl InlineFragmentSelection { }) } + /// Construct a new InlineFragmentSelection out of a selection set. + /// - The new type condition will be the same as the selection set's type. + pub(crate) fn from_selection_set( + parent_type_position: CompositeTypeDefinitionPosition, + selection_set: SelectionSet, + directives: Arc, + ) -> Self { + let inline_fragment_data = InlineFragmentData { + schema: selection_set.schema.clone(), + parent_type_position, + type_condition_position: selection_set.type_position.clone().into(), + directives, + selection_id: SelectionId::new(), + }; + Self { + inline_fragment: InlineFragment::new(inline_fragment_data), + selection_set, + } + } + pub(crate) fn normalize( &self, parent_type: &CompositeTypeDefinitionPosition, @@ -4546,11 +4553,24 @@ impl NamedFragments { self.fragments.values() } - pub(crate) fn insert(&mut self, fragment: Fragment) { + pub(crate) fn iter_rev(&self) -> impl Iterator> { + self.fragments.values().rev() + } + + pub(crate) fn iter_mut(&mut self) -> indexmap::map::IterMut<'_, Name, Node> { + Arc::make_mut(&mut self.fragments).iter_mut() + } + + // Calls `retain` on the underlying `IndexMap`. + pub(crate) fn retain(&mut self, mut predicate: impl FnMut(&Name, &Node) -> bool) { + Arc::make_mut(&mut self.fragments).retain(|name, fragment| predicate(name, fragment)); + } + + fn insert(&mut self, fragment: Fragment) { Arc::make_mut(&mut self.fragments).insert(fragment.name.clone(), Node::new(fragment)); } - pub(crate) fn try_insert(&mut self, fragment: Fragment) -> Result<(), FederationError> { + fn try_insert(&mut self, fragment: Fragment) -> Result<(), FederationError> { match Arc::make_mut(&mut self.fragments).entry(fragment.name.clone()) { indexmap::map::Entry::Occupied(_) => { Err(FederationError::internal("Duplicate fragment name")) @@ -4597,7 +4617,9 @@ impl NamedFragments { depends_on: Vec, } - let mut fragments_map: HashMap = HashMap::new(); + // Note: We use IndexMap to stabilize the ordering of the result, which influences + // the outcome of `map_to_expanded_selection_sets`. + let mut fragments_map: IndexMap = IndexMap::new(); for fragment in fragments.values() { let mut fragment_usages: HashMap = HashMap::new(); NamedFragments::collect_fragment_usages(&fragment.selection_set, &mut fragment_usages); @@ -4744,10 +4766,10 @@ impl NamedFragments { &fragment.schema, NormalizeSelectionOption::NormalizeRecursively, )?; - let mapped_selection_set = mapper(&expanded_selection_set)?; - let optimized_selection_set = mapped_selection_set; // TODO: call SelectionSet::optimize (FED-191) + let mut mapped_selection_set = mapper(&expanded_selection_set)?; + mapped_selection_set.optimize_at_root(&result)?; let updated = Fragment { - selection_set: optimized_selection_set, + selection_set: mapped_selection_set, schema: fragment.schema.clone(), name: fragment.name.clone(), type_condition_position: fragment.type_condition_position.clone(), @@ -4848,9 +4870,9 @@ pub(crate) struct RebasedFragments { } impl RebasedFragments { - pub(crate) fn new(fragments: &NamedFragments) -> Self { + pub(crate) fn new(fragments: NamedFragments) -> Self { Self { - original_fragments: fragments.clone(), + original_fragments: fragments, rebased_fragments: Arc::new(HashMap::new()), } } @@ -5067,6 +5089,10 @@ impl Display for Operation { Ok(operation) => operation, Err(_) => return Err(std::fmt::Error), }; + for fragment_def in self.named_fragments.iter() { + fragment_def.fmt(f)?; + f.write_str("\n\n")?; + } operation.serialize().fmt(f) } } @@ -5320,23 +5346,23 @@ type Foo { .named_operations .get_mut("NamedFragmentQuery") { - let normalized_operation = normalize_operation( + let mut normalized_operation = normalize_operation( operation, NamedFragments::new(&executable_document.fragments, &schema), &schema, &IndexSet::new(), ) .unwrap(); - - let expected = r#"query NamedFragmentQuery { - foo { - id - bar - baz - } -}"#; - let actual = normalized_operation.to_string(); - assert_eq!(expected, actual); + normalized_operation.named_fragments = Default::default(); + insta::assert_snapshot!(normalized_operation, @r###" + query NamedFragmentQuery { + foo { + id + bar + baz + } + } + "###); } } @@ -5374,23 +5400,23 @@ type Foo { let (schema, mut executable_document) = parse_schema_and_operation(operation_with_named_fragment); if let Some((_, operation)) = executable_document.named_operations.first_mut() { - let normalized_operation = normalize_operation( + let mut normalized_operation = normalize_operation( operation, NamedFragments::new(&executable_document.fragments, &schema), &schema, &IndexSet::new(), ) .unwrap(); - - let expected = r#"query NestedFragmentQuery { - foo { - id - bar - baz - } -}"#; - let actual = normalized_operation.to_string(); - assert_eq!(expected, actual); + normalized_operation.named_fragments = Default::default(); + insta::assert_snapshot!(normalized_operation, @r###" + query NestedFragmentQuery { + foo { + id + bar + baz + } + } + "###); } } @@ -7105,6 +7131,10 @@ type T { None, ) .unwrap(), @r###" + fragment frag on HasA { + intfField + } + { intf { ... on HasA { @@ -7232,7 +7262,7 @@ type T { ss: &SelectionSet, pred: &impl Fn(&Selection) -> bool, ) -> Result { - ss.lazy_map(|s| { + ss.lazy_map(/*named_fragments*/ &Default::default(), |s| { if !pred(s) { return Ok(SelectionMapperReturn::None); } @@ -7319,7 +7349,7 @@ type T { ss: &SelectionSet, pred: &impl Fn(&Selection) -> bool, ) -> Result { - ss.lazy_map(|s| { + ss.lazy_map(/*named_fragments*/ &Default::default(), |s| { let to_add_typename = pred(s); let updated = s.map_selection_set(|ss| add_typename_if(ss, pred).map(Some))?; if !to_add_typename { diff --git a/apollo-federation/src/query_plan/optimize.rs b/apollo-federation/src/query_plan/optimize.rs index 06de27e5f3..a3372f258a 100644 --- a/apollo-federation/src/query_plan/optimize.rs +++ b/apollo-federation/src/query_plan/optimize.rs @@ -22,12 +22,23 @@ //! fragments that can be used to optimize the selection set. If there is a single fragment that //! covers the full selection set, then that fragment is used. Otherwise, we attempted to reduce //! the number of fragments applied (but optimality is not guaranteed yet). +//! +//! ## Retain certain fragments in selection sets while expanding the rest +//! Unlike the `expand_all_fragments` method, this methods retains the listed fragments. +//! +//! ## Optimize (or reduce) the named fragments in the query +//! Optimization of named fragment definitions in query documents based on the usage of +//! fragments in (optimized) operations. +//! +//! ## `optimize` methods (putting everything together) +//! Recursive optimization of selection and selection sets. use std::collections::HashMap; use std::collections::HashSet; use std::ops::Not; use std::sync::Arc; +use apollo_compiler::executable; use apollo_compiler::executable::Name; use apollo_compiler::Node; @@ -35,12 +46,17 @@ use super::operation::CollectedFieldInSet; use super::operation::Containment; use super::operation::ContainmentOptions; use super::operation::Field; +use super::operation::FieldSelection; use super::operation::Fragment; use super::operation::FragmentSpreadSelection; +use super::operation::InlineFragmentSelection; use super::operation::NamedFragments; use super::operation::NormalizeSelectionOption; +use super::operation::Operation; use super::operation::Selection; use super::operation::SelectionKey; +use super::operation::SelectionMapperReturn; +use super::operation::SelectionOrSet; use super::operation::SelectionSet; use crate::error::FederationError; use crate::schema::position::CompositeTypeDefinitionPosition; @@ -151,7 +167,7 @@ impl SelectionSet { } //============================================================================= -// Filtering applicable fragments +// Collect applicable fragments at given type. impl Fragment { /// Whether this fragment may apply _directly_ at the provided type, meaning that the fragment @@ -577,6 +593,50 @@ impl Fragment { } } +enum FullMatchingFragmentCondition<'a> { + ForFieldSelection, + ForInlineFragmentSelection { + // the type condition and directives on an inline fragment selection. + type_condition_position: &'a CompositeTypeDefinitionPosition, + directives: &'a Arc, + }, +} + +impl<'a> FullMatchingFragmentCondition<'a> { + /// Determines whether the given fragment is allowed to match the whole selection set by itself + /// (without another selection set wrapping it). + fn check(&self, fragment: &Node) -> bool { + match self { + // We can never apply a fragments that has directives on it at the field level. + Self::ForFieldSelection => fragment.directives.is_empty(), + + // To be able to use a matching inline fragment, it needs to have either no directives, + // or if it has some, then: + // 1. All it's directives should also be on the current element. + // 2. The type condition of this element should be the fragment's condition. because + // If those 2 conditions are true, we can replace the whole current inline fragment + // with the match spread and directives will still match. + Self::ForInlineFragmentSelection { + type_condition_position, + directives, + } => { + if fragment.directives.is_empty() { + return true; + } + + // PORT_NOTE: The JS version handles `@defer` directive differently. However, Rust + // version can't have `@defer` at this point (see comments on `enum SelectionKey` + // definition) + fragment.type_condition_position == **type_condition_position + && fragment + .directives + .iter() + .all(|d1| directives.iter().any(|d2| d1 == d2)) + } + } + } +} + /// The return type for `SelectionSet::try_optimize_with_fragments`. #[derive(derive_more::From)] enum SelectionSetOrFragment { @@ -665,7 +725,7 @@ impl SelectionSet { parent_type: &CompositeTypeDefinitionPosition, fragments: &NamedFragments, validator: &mut FieldsConflictMultiBranchValidator, - can_use_full_matching_fragment: impl Fn(&Fragment) -> bool, + full_match_condition: FullMatchingFragmentCondition, ) -> Result { // We limit to fragments whose selection could be applied "directly" at `parent_type`, // meaning without taking the fragment condition into account. The idea being that if the @@ -718,7 +778,7 @@ impl SelectionSet { if matches!(res, Containment::NotContained) { continue; // Not eligible; Skip it. } - if matches!(res, Containment::Equal) && can_use_full_matching_fragment(&candidate) { + if matches!(res, Containment::Equal) && full_match_condition.check(&candidate) { // Special case: Found a fragment that covers the full selection set. return Ok(candidate.into()); } @@ -765,3 +825,865 @@ impl SelectionSet { .into()) } } + +//============================================================================= +// Retain fragments in selection sets while expanding the rest + +impl Selection { + /// Expand fragments that are not in the `fragments_to_keep`. + // PORT_NOTE: The JS version's name was `expandFragments`, which was confusing with + // `expand_all_fragments`. So, it was renamed to `retain_fragments`. + fn retain_fragments( + &self, + parent_type: &CompositeTypeDefinitionPosition, + fragments_to_keep: &NamedFragments, + ) -> Result { + match self { + Selection::FragmentSpread(fragment) => { + if fragments_to_keep.contains(&fragment.spread.data().fragment_name) { + // Keep this spread + Ok(self.clone().into()) + } else { + // Expand the fragment + let expanded_sub_selections = + fragment.selection_set.retain_fragments(fragments_to_keep)?; + if *parent_type == fragment.spread.data().type_condition_position { + // The fragment is of the same type as the parent, so we can just use + // the expanded sub-selections directly. + Ok(expanded_sub_selections.into()) + } else { + // Create an inline fragment since type condition is necessary. + let inline = InlineFragmentSelection::from_fragment_spread_selection( + parent_type.clone(), + fragment, + )?; + Ok(Selection::from(inline).into()) + } + } + } + + // Otherwise, expand the sub-selections. + _ => Ok(self + .map_selection_set(|selection_set| { + Ok(Some(selection_set.retain_fragments(fragments_to_keep)?)) + })? + .into()), + } + } +} + +// Note: `retain_fragments` methods may return a selection or a selection set. +impl From for SelectionMapperReturn { + fn from(value: SelectionOrSet) -> Self { + match value { + SelectionOrSet::Selection(selection) => selection.into(), + SelectionOrSet::SelectionSet(selections) => { + // The items in a selection set needs to be cloned here, since it's sub-selections + // are contained in an `Arc`. + Vec::from_iter(selections.selections.values().cloned()).into() + } + } + } +} + +impl SelectionSet { + /// Expand fragments that are not in the `fragments_to_keep`. + // PORT_NOTE: The JS version's name was `expandFragments`, which was confusing with + // `expand_all_fragments`. So, it was renamed to `retain_fragments`. + fn retain_fragments( + &self, + fragments_to_keep: &NamedFragments, + ) -> Result { + self.lazy_map(fragments_to_keep, |selection| { + Ok(selection + .retain_fragments(&self.type_position, fragments_to_keep)? + .into()) + }) + } +} + +//============================================================================= +// Optimize (or reduce) the named fragments in the query +// +// Things to consider: +// - Unused fragment definitions can be dropped without an issue. +// - Dropping low-usage named fragments and expanding them may insert other fragments resulting in +// increased usage of those inserted. +// +// Example: +// ```graphql +// query { +// ...F1 +// } +// +// fragment F1 { +// a { ...F2 } +// b { ...F2 } +// } +// +// fragment F2 { +// // something +// } +// ``` +// then at this point where we've only counted usages in the query selection, `usages` will be +// `{ F1: 1, F2: 0 }`. But we do not want to expand _both_ F1 and F2. Instead, we want to expand +// F1 first, and then realize that this increases F2 usages to 2, which means we stop there and keep F2. + +impl NamedFragments { + /// Compute the reduced set of NamedFragments that are used in the selection set at least + /// `min_usage_to_optimize` times. Also, computes the new selection set that uses only the + /// reduced set of fragments by expanding the other ones. + fn reduce( + &mut self, + selection_set: &SelectionSet, + min_usage_to_optimize: u32, + ) -> Result { + // Initial computation of fragment usages in `selection_set`. + let mut usages = HashMap::new(); + selection_set.collect_used_fragment_names(&mut usages); + + // Short-circuiting: Nothing was used => Drop everything (selection_set is unchanged). + if usages.is_empty() { + self.retain(|_, _| false); + return Ok(selection_set.clone()); + } + + // Determine which one to retain. + // - Calculate the usage count of each fragment in both query and other fragment definitions. + // - If a fragment is to keep, fragments used in it are counted. + // - If a fragment is to drop, fragments used in it are counted and multiplied by its usage. + // - Decide in reverse dependency order, so that at each step, the fragment being visited + // has following properties: + // - It is either indirectly used by a previous fragment; Or, not used directly by any + // one visited & retained before. + // - Its usage count should be correctly calculated as if dropped fragments were expanded. + // - We take advantage of the fact that `NamedFragments` is already sorted in dependency + // order. + let min_usage_to_optimize: i32 = min_usage_to_optimize.try_into().unwrap_or(i32::MAX); + let original_size = self.size(); + for fragment in self.iter_rev() { + let usage_count = usages.get(&fragment.name).copied().unwrap_or_default(); + if usage_count >= min_usage_to_optimize { + // Count indirect usages within the fragment definition. + fragment.collect_used_fragment_names(&mut usages); + } else { + // Compute the new usage count after expanding the `fragment`. + Self::update_usages(&mut usages, fragment, usage_count); + } + } + self.retain(|name, _fragment| { + let usage_count = usages.get(name).copied().unwrap_or_default(); + usage_count >= min_usage_to_optimize + }); + + // Short-circuiting: Nothing was dropped (fully used) => Nothing to change. + if self.size() == original_size { + return Ok(selection_set.clone()); + } + + // Update the fragment definitions in `self` after reduction. + // Note: This is an unfortunate clone, since `self` can't be passed to `retain_fragments`, + // while being mutated. + let fragments_to_keep = self.clone(); + for (_, fragment) in self.iter_mut() { + Node::make_mut(fragment).selection_set = fragment + .selection_set + .retain_fragments(&fragments_to_keep)?; + } + + // Compute the new selection set based on the new reduced set of fragments. + selection_set.retain_fragments(self) + } + + fn update_usages(usages: &mut HashMap, fragment: &Node, usage_count: i32) { + let mut inner_usages = HashMap::new(); + fragment.collect_used_fragment_names(&mut inner_usages); + + for (name, inner_count) in inner_usages { + *usages.entry(name).or_insert(0) += inner_count * usage_count; + } + } +} + +//============================================================================= +// `optimize` methods (putting everything together) + +impl Selection { + fn optimize( + &self, + fragments: &NamedFragments, + validator: &mut FieldsConflictMultiBranchValidator, + ) -> Result { + match self { + Selection::Field(field) => Ok(field.optimize(fragments, validator)?.into()), + Selection::FragmentSpread(_) => Ok(self.clone()), // Do nothing + Selection::InlineFragment(inline_fragment) => { + Ok(inline_fragment.optimize(fragments, validator)?.into()) + } + } + } +} + +impl FieldSelection { + fn optimize( + &self, + fragments: &NamedFragments, + validator: &mut FieldsConflictMultiBranchValidator, + ) -> Result { + let Some(base_composite_type): Option = + self.field.data().output_base_type()?.try_into().ok() + else { + return Ok(self.clone()); + }; + let Some(ref selection_set) = self.selection_set else { + return Ok(self.clone()); + }; + + let mut field_validator = validator.for_field(&self.field); + + // First, see if we can reuse fragments for the selection of this field. + let opt = selection_set.try_optimize_with_fragments( + &base_composite_type, + fragments, + &mut field_validator, + FullMatchingFragmentCondition::ForFieldSelection, + )?; + + let mut optimized; + match opt { + SelectionSetOrFragment::Fragment(fragment) => { + let fragment_selection = FragmentSpreadSelection::from_fragment( + &fragment, + /*directives*/ &Default::default(), + ); + optimized = + SelectionSet::from_selection(base_composite_type, fragment_selection.into()); + } + SelectionSetOrFragment::SelectionSet(selection_set) => { + optimized = selection_set; + } + } + optimized = optimized.optimize(fragments, &mut field_validator)?; + Ok(self.with_updated_selection_set(Some(optimized))) + } +} + +/// Return type for `InlineFragmentSelection::optimize`. +#[derive(derive_more::From)] +enum InlineOrFragmentSelection { + // Note: Enum variants are named to match those of `Selection`. + InlineFragment(InlineFragmentSelection), + FragmentSpread(FragmentSpreadSelection), +} + +impl From for Selection { + fn from(value: InlineOrFragmentSelection) -> Self { + match value { + InlineOrFragmentSelection::InlineFragment(inline_fragment) => inline_fragment.into(), + InlineOrFragmentSelection::FragmentSpread(fragment_spread) => fragment_spread.into(), + } + } +} + +impl InlineFragmentSelection { + fn optimize( + &self, + fragments: &NamedFragments, + validator: &mut FieldsConflictMultiBranchValidator, + ) -> Result { + let mut optimized = self.selection_set.clone(); + + let type_condition_position = &self.inline_fragment.data().type_condition_position; + if let Some(type_condition_position) = type_condition_position { + let opt = self.selection_set.try_optimize_with_fragments( + type_condition_position, + fragments, + validator, + FullMatchingFragmentCondition::ForInlineFragmentSelection { + type_condition_position, + directives: &self.inline_fragment.data().directives, + }, + )?; + + match opt { + SelectionSetOrFragment::Fragment(fragment) => { + // We're fully matching the sub-selection. If the fragment condition is also + // this element condition, then we can replace the whole element by the spread + // (not just the sub-selection). + if *type_condition_position == fragment.type_condition_position { + // Optimized as `...`, dropping the original inline spread (`self`). + + // Note that `FullMatchingFragmentCondition::ForInlineFragmentSelection` + // above guarantees that this element directives are a superset of the + // fragment directives. But there can be additional directives, and in that + // case they should be kept on the spread. + // PORT_NOTE: We are assuming directives on fragment definitions are + // carried over to their spread sites as JS version does, which + // is handled differently in Rust version (see `FragmentSpreadData`). + let directives: executable::DirectiveList = self + .inline_fragment + .data() + .directives + .iter() + .filter(|d1| !fragment.directives.iter().any(|d2| *d1 == d2)) + .cloned() + .collect(); + return Ok( + FragmentSpreadSelection::from_fragment(&fragment, &directives).into(), + ); + } else { + // Otherwise, we keep this element and use a sub-selection with just the spread. + // Optimized as `...on { ... }` + optimized = SelectionSet::from_selection( + type_condition_position.clone(), + FragmentSpreadSelection::from_fragment( + &fragment, + /*directives*/ &Default::default(), + ) + .into(), + ); + // fall-through + } + } + SelectionSetOrFragment::SelectionSet(selection_set) => { + optimized = selection_set; + // fall-through + } + } + } + + // Then, recurse inside the field sub-selection (note that if we matched some fragments + // above, this recursion will "ignore" those as `FragmentSpreadSelection`'s `optimize()` is + // a no-op). + optimized = optimized.optimize(fragments, validator)?; + Ok(InlineFragmentSelection { + inline_fragment: self.inline_fragment.clone(), + selection_set: optimized, + } + .into()) + } +} + +impl SelectionSet { + /// Recursively call `optimize` on each selection in the selection set. + fn optimize( + &self, + fragments: &NamedFragments, + validator: &mut FieldsConflictMultiBranchValidator, + ) -> Result { + self.lazy_map(fragments, |selection| { + Ok(vec![selection.optimize(fragments, validator)?].into()) + }) + } + + /// Specialized version of `optimize` for top-level sub-selections under Operation + /// or Fragment. + pub(crate) fn optimize_at_root( + &mut self, + fragments: &NamedFragments, + ) -> Result<(), FederationError> { + if fragments.is_empty() { + return Ok(()); + } + + // Calling optimize() will not match a fragment that would have expanded at + // top-level. That is, say we have the selection set `{ x y }` for a top-level `Query`, and + // we have a fragment + // ``` + // fragment F on Query { + // x + // y + // } + // ``` + // then calling `self.optimize(fragments)` would only apply check if F apply to + // `x` and then `y`. + // + // To ensure the fragment match in this case, we "wrap" the selection into a trivial + // fragment of the selection parent, so in the example above, we create selection `... on + // Query { x y}`. With that, `optimize` will correctly match on the `on Query` + // fragment; after which we can unpack the final result. + let wrapped = InlineFragmentSelection::from_selection_set( + self.type_position.clone(), // parent type + self.clone(), // selection set + Default::default(), // directives + ); + let mut validator = FieldsConflictMultiBranchValidator::from_initial_validator( + FieldsConflictValidator::from_selection_set(self), + ); + let optimized = wrapped.optimize(fragments, &mut validator)?; + + // Now, it's possible we matched a full fragment, in which case `optimized` will be just + // the named fragment, and in that case we return a singleton selection with just that. + // Otherwise, it's our wrapping inline fragment with the sub-selections optimized, and we + // just return that subselection. + match optimized { + InlineOrFragmentSelection::FragmentSpread(_) => { + let self_selections = Arc::make_mut(&mut self.selections); + self_selections.clear(); + self_selections.insert(optimized.into()); + } + + InlineOrFragmentSelection::InlineFragment(inline_fragment) => { + // Note: `inline_fragment.selection_set` can't be moved (since it's inside Arc). + // So, it's cloned. + *self = inline_fragment.selection_set.clone(); + } + } + Ok(()) + } +} + +impl Operation { + // PORT_NOTE: The JS version of `optimize` takes an optional `minUsagesToOptimize` argument. + // However, it's only used in tests. So, it's removed in the Rust version. + const DEFAULT_MIN_USAGES_TO_OPTIMIZE: u32 = 2; + + pub(crate) fn optimize(&mut self, fragments: &NamedFragments) -> Result<(), FederationError> { + if fragments.is_empty() { + return Ok(()); + } + + // Optimize the operation's selection set by re-using existing fragments. + let before_optimization = self.selection_set.clone(); + self.selection_set.optimize_at_root(fragments)?; + if before_optimization == self.selection_set { + return Ok(()); + } + + // Optimize the named fragment definitions by dropping low-usage ones. + let mut final_fragments = fragments.clone(); + let final_selection_set = + final_fragments.reduce(&self.selection_set, Self::DEFAULT_MIN_USAGES_TO_OPTIMIZE)?; + + self.selection_set = final_selection_set; + self.named_fragments = final_fragments; + Ok(()) + } + + // Mainly for testing. + fn expand_all_fragments(&self) -> Result { + let selection_set = self.selection_set.expand_all_fragments()?; + Ok(Self { + named_fragments: Default::default(), + selection_set, + ..self.clone() + }) + } +} + +//============================================================================= +// Tests + +#[cfg(test)] +mod tests { + use apollo_compiler::schema::Schema; + + use super::*; + use crate::schema::ValidFederationSchema; + + fn parse_schema(schema_doc: &str) -> ValidFederationSchema { + let schema = Schema::parse_and_validate(schema_doc, "schema.graphql").unwrap(); + ValidFederationSchema::new(schema).unwrap() + } + + fn parse_operation(schema: &ValidFederationSchema, query: &str) -> Operation { + let executable_document = apollo_compiler::ExecutableDocument::parse_and_validate( + schema.schema(), + query, + "query.graphql", + ) + .unwrap(); + let operation = executable_document.get_operation(None).unwrap(); + let named_fragments = NamedFragments::new(&executable_document.fragments, schema); + let selection_set = + SelectionSet::from_selection_set(&operation.selection_set, &named_fragments, schema) + .unwrap(); + + Operation { + schema: schema.clone(), + root_kind: operation.operation_type.into(), + name: operation.name.clone(), + variables: Arc::new(operation.variables.clone()), + directives: Arc::new(operation.directives.clone()), + selection_set, + named_fragments, + } + } + + macro_rules! assert_without_fragments { + ($operation: expr, @$expected: literal) => {{ + let without_fragments = $operation.expand_all_fragments().unwrap(); + insta::assert_snapshot!(without_fragments, @$expected); + without_fragments + }}; + } + + macro_rules! assert_optimized { + ($operation: expr, $named_fragments: expr, @$expected: literal) => {{ + let mut optimized = $operation.clone(); + optimized.optimize(&$named_fragments).unwrap(); + insta::assert_snapshot!(optimized, @$expected) + }}; + } + + #[test] + fn optimize_fragments_using_other_fragments_when_possible() { + let schema = r#" + type Query { + t: I + } + + interface I { + b: Int + u: U + } + + type T1 implements I { + a: Int + b: Int + u: U + } + + type T2 implements I { + x: String + y: String + b: Int + u: U + } + + union U = T1 | T2 + "#; + + let query = r#" + fragment OnT1 on T1 { + a + b + } + + fragment OnT2 on T2 { + x + y + } + + fragment OnI on I { + b + } + + fragment OnU on U { + ...OnI + ...OnT1 + ...OnT2 + } + + query { + t { + ...OnT1 + ...OnT2 + ...OnI + u { + ...OnU + } + } + } + "#; + + let operation = parse_operation(&parse_schema(schema), query); + + let expanded = assert_without_fragments!( + operation, + @r###" + { + t { + ... on T1 { + a + b + } + ... on T2 { + x + y + } + b + u { + ... on I { + b + } + ... on T1 { + a + b + } + ... on T2 { + x + y + } + } + } + } + "### + ); + + assert_optimized!(expanded, operation.named_fragments, @r###" + fragment OnU on U { + ... on I { + b + } + ... on T1 { + a + b + } + ... on T2 { + x + y + } + } + + { + t { + ...OnU + u { + ...OnU + } + } + } + "###); + } + + #[test] + #[ignore] // Appears to be an expansion bug. + fn handles_fragments_using_other_fragments() { + let schema = r#" + type Query { + t: I + } + + interface I { + b: Int + c: Int + u1: U + u2: U + } + + type T1 implements I { + a: Int + b: Int + c: Int + me: T1 + u1: U + u2: U + } + + type T2 implements I { + x: String + y: String + b: Int + c: Int + u1: U + u2: U + } + + union U = T1 | T2 + "#; + + let query = r#" + fragment OnT1 on T1 { + a + b + } + + fragment OnT2 on T2 { + x + y + } + + fragment OnI on I { + b + c + } + + fragment OnU on U { + ...OnI + ...OnT1 + ...OnT2 + } + + query { + t { + ...OnT1 + ...OnT2 + u1 { + ...OnU + } + u2 { + ...OnU + } + ... on T1 { + me { + ...OnI + } + } + } + } + "#; + + let operation = parse_operation(&parse_schema(schema), query); + + let expanded = assert_without_fragments!( + &operation, + @r###" + { + t { + ... on T1 { + a + b + me { + b + c + } + } + ... on T2 { + x + y + } + u1 { + ... on I { + b + c + } + ... on T1 { + a + b + } + ... on T2 { + x + y + } + } + u2 { + ... on I { + b + c + } + ... on T1 { + a + b + } + ... on T2 { + x + y + } + } + } + } + "###); + + // We should reuse and keep all fragments, because 1) onU is used twice and 2) + // all the other ones are used once in the query, and once in onU definition. + assert_optimized!(expanded, operation.named_fragments, @r###" + fragment OnT1 on T1 { + a + b + } + + fragment OnT2 on T2 { + x + y + } + + fragment OnI on I { + b + c + } + + fragment OnU on U { + ...OnI + ...OnT1 + ...OnT2 + } + + { + t { + ... on T1 { + ...OnT1 + me { + ...OnI + } + } + ...OnT2 + u1 { + ...OnU + } + u2 { + ...OnU + } + } + } + "###); + } + + macro_rules! test_fragments_roundtrip { + ($schema_doc: expr, $query: expr, @$expanded: literal) => {{ + let schema = parse_schema($schema_doc); + let operation = parse_operation(&schema, $query); + let without_fragments = operation.expand_all_fragments().unwrap(); + insta::assert_snapshot!(without_fragments, @$expanded); + + let mut optimized = without_fragments; + optimized.optimize(&operation.named_fragments).unwrap(); + assert_eq!(optimized.to_string(), operation.to_string()); + }}; + } + + #[test] + fn handles_fragments_with_nested_selections() { + let schema_doc = r#" + type Query { + t1a: T1 + t2a: T1 + } + + type T1 { + t2: T2 + } + + type T2 { + x: String + y: String + } + "#; + + let query = r#" + fragment OnT1 on T1 { + t2 { + x + } + } + + query { + t1a { + ...OnT1 + t2 { + y + } + } + t2a { + ...OnT1 + } + } + "#; + + test_fragments_roundtrip!(schema_doc, query, @r###" + { + t1a { + t2 { + x + y + } + } + t2a { + t2 { + x + } + } + } + "###); + } +} diff --git a/apollo-federation/src/query_plan/query_planner.rs b/apollo-federation/src/query_plan/query_planner.rs index ae5085c9ab..da44adf876 100644 --- a/apollo-federation/src/query_plan/query_planner.rs +++ b/apollo-federation/src/query_plan/query_planner.rs @@ -357,19 +357,9 @@ impl QueryPlanner { } let reuse_query_fragments = self.config.reuse_query_fragments; - let mut named_fragments = NamedFragments::new(&document.fragments, &self.api_schema); - if reuse_query_fragments { - // For all subgraph fetches we query `__typename` on every abstract types (see - // `FetchDependencyGraphNode::to_plan_node`) so if we want to have a chance to reuse - // fragments, we should make sure those fragments also query `__typename` for every - // abstract type. - named_fragments = - named_fragments.add_typename_field_for_abstract_types_in_named_fragments()?; - } - let normalized_operation = normalize_operation( operation, - named_fragments, + NamedFragments::new(&document.fragments, &self.api_schema), &self.api_schema, &self.interface_types_with_interface_objects, )?; @@ -413,9 +403,22 @@ impl QueryPlanner { ); }; + let rebased_fragments = if reuse_query_fragments { + // For all subgraph fetches we query `__typename` on every abstract types (see + // `FetchDependencyGraphNode::to_plan_node`) so if we want to have a chance to reuse + // fragments, we should make sure those fragments also query `__typename` for every + // abstract type. + Some(RebasedFragments::new( + normalized_operation + .named_fragments + .add_typename_field_for_abstract_types_in_named_fragments()?, + )) + } else { + None + }; let processor = FetchDependencyGraphToQueryPlanProcessor::new( operation.variables.clone(), - Some(RebasedFragments::new(&normalized_operation.named_fragments)), + rebased_fragments, operation.name.clone(), assigned_defer_labels, ); @@ -1109,4 +1112,170 @@ type User } "###); } + + #[test] + fn test_optimize_basic() { + let supergraph = Supergraph::new(TEST_SUPERGRAPH).unwrap(); + let api_schema = supergraph.to_api_schema(Default::default()).unwrap(); + let document = ExecutableDocument::parse_and_validate( + api_schema.schema(), + r#" + { + userById(id: 1) { + id + ...userFields + }, + another_user: userById(id: 2) { + name + email + } + } + fragment userFields on User { + name + email + } + "#, + "operation.graphql", + ) + .unwrap(); + + let planner = QueryPlanner::new(&supergraph, Default::default()).unwrap(); + let plan = planner.build_query_plan(&document, None).unwrap(); + insta::assert_snapshot!(plan, @r###" + QueryPlan { + Fetch(service: "accounts") { + { + userById(id: 1) { + ...userFields + id + } another_user: userById(id: 2) { + ...userFields + } + } + + fragment userFields on User { + name + email + } + }, + } + "###); + } + + #[test] + fn test_optimize_inline_fragment() { + let supergraph = Supergraph::new(TEST_SUPERGRAPH).unwrap(); + let api_schema = supergraph.to_api_schema(Default::default()).unwrap(); + let document = ExecutableDocument::parse_and_validate( + api_schema.schema(), + r#" + { + userById(id: 1) { + id + ...userFields + }, + partial_optimize: userById(id: 2) { + ... on User { + id + name + email + } + }, + full_optimize: userById(id: 3) { + ... on User { + name + email + } + } + } + fragment userFields on User { + name + email + } + "#, + "operation.graphql", + ) + .unwrap(); + + let planner = QueryPlanner::new(&supergraph, Default::default()).unwrap(); + let plan = planner.build_query_plan(&document, None).unwrap(); + insta::assert_snapshot!(plan, @r###" + QueryPlan { + Fetch(service: "accounts") { + { + userById(id: 1) { + ...userFields + id + } partial_optimize: userById(id: 2) { + ...userFields + id + } full_optimize: userById(id: 3) { + ...userFields + } + } + + fragment userFields on User { + name + email + } + }, + } + "###); + } + + #[test] + fn test_optimize_fragment_definition() { + let supergraph = Supergraph::new(TEST_SUPERGRAPH).unwrap(); + let api_schema = supergraph.to_api_schema(Default::default()).unwrap(); + let document = ExecutableDocument::parse_and_validate( + api_schema.schema(), + r#" + { + userById(id: 1) { + ...F1 + ...F2 + }, + case2: userById(id: 2) { + id + name + email + }, + } + fragment F1 on User { + name + email + } + fragment F2 on User { + id + name + email + } + "#, + "operation.graphql", + ) + .unwrap(); + + let planner = QueryPlanner::new(&supergraph, Default::default()).unwrap(); + let plan = planner.build_query_plan(&document, None).unwrap(); + // Make sure `fragment F2` contains `...F1`. + insta::assert_snapshot!(plan, @r###" + QueryPlan { + Fetch(service: "accounts") { + { + userById(id: 1) { + ...F2 + } case2: userById(id: 2) { + ...F2 + } + } + + fragment F2 on User { + name + email + id + } + }, + } + "###); + } } 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 6203cadebb..9b6bcce98e 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests.rs @@ -215,8 +215,8 @@ fn field_covariance_and_type_explosion() { } #[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure +#[should_panic(expected = "snapshot assertion")] +// TODO: investigate this failure - unexpected inline spread fn handles_non_intersecting_fragment_conditions() { let planner = planner!( Subgraph1: r#" diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs index f5f1a0cf52..5ef3155939 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs @@ -1,8 +1,8 @@ use apollo_federation::query_plan::query_planner::QueryPlannerConfig; #[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure +#[should_panic(expected = "snapshot assertion")] +// TODO: investigate this failure - missing `__typename` under `a` fn it_works_with_nested_fragments_1() { let planner = planner!( Subgraph1: r#" @@ -127,8 +127,6 @@ fn it_works_with_nested_fragments_1() { } #[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure fn it_avoid_fragments_usable_only_once() { let planner = planner!( Subgraph1: r#" @@ -248,49 +246,52 @@ fn it_avoid_fragments_usable_only_once() { } "#, @r###" - QueryPlan { - Sequence { - Fetch(service: "Subgraph1") { - { - t { - __typename - id - } + QueryPlan { + Sequence { + Fetch(service: "Subgraph1") { + { + t { + __typename + id + } + } + }, + Flatten(path: "t") { + Fetch(service: "Subgraph2") { + { + ... on T { + __typename + id } - }, - Flatten(path: "t") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - } - } => - { - ... on T { - v2 { - ...OnV - } - v3 { - ...OnV - } - } + } => + { + ... on T { + v2 { + ...OnV } - - fragment OnV on V { - a - b - c + v3 { + ...OnV } - }, - }, + } + } + + fragment OnV on V { + a + b + c + } }, - } - "### + }, + }, + } + "### ); } -const SUBGRAPH: &str = r#" +mod respects_query_planner_option_reuse_query_fragments { + use super::*; + + const SUBGRAPH1: &str = r#" type Query { t: T } @@ -304,9 +305,8 @@ const SUBGRAPH: &str = r#" x: Int y: Int } -"#; - -const QUERY: &str = r#" + "#; + const QUERY: &str = r#" query { t { a1 { @@ -317,26 +317,26 @@ const QUERY: &str = r#" } } } - + fragment Selection on A { x y } -"#; - -#[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure - -fn respects_query_planner_option_reuse_query_fragments_true() { - let planner = planner!( - config = QueryPlannerConfig { reuse_query_fragments: true, ..Default::default()}, - Subgraph1: SUBGRAPH, - ); - assert_plan!( - &planner, - QUERY, - @r#" + "#; + + #[test] + fn respects_query_planner_option_reuse_query_fragments_true() { + let reuse_query_fragments = true; + let planner = planner!( + config = QueryPlannerConfig {reuse_query_fragments, ..Default::default()}, + Subgraph1: SUBGRAPH1, + ); + let query = QUERY; + + assert_plan!( + &planner, + query, + @r###" QueryPlan { Fetch(service: "Subgraph1") { { @@ -349,53 +349,52 @@ fn respects_query_planner_option_reuse_query_fragments_true() { } } } - + fragment Selection on A { x y } }, } - "# - ); -} - -#[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure - -fn respects_query_planner_option_reuse_query_fragments_false() { - let planner = planner!( - config = QueryPlannerConfig { reuse_query_fragments: false, ..Default::default()}, - Subgraph1: SUBGRAPH, - ); - assert_plan!( - &planner, - QUERY, - @r#" - QueryPlan { - Fetch(service: "Subgraph1") { - { - t { - a1 { - x - y - } - a2 { - x - y + "### + ); + } + + #[test] + fn respects_query_planner_option_reuse_query_fragments_false() { + let reuse_query_fragments = false; + let planner = planner!( + config = QueryPlannerConfig {reuse_query_fragments, ..Default::default()}, + Subgraph1: SUBGRAPH1, + ); + let query = QUERY; + + assert_plan!( + &planner, + query, + @r#" + QueryPlan { + Fetch(service: "Subgraph1") { + { + t { + a1 { + x + y + } + a2 { + x + y + } + } } - } + }, } - }, - } - "# - ); + "# + ); + } } #[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure fn it_works_with_nested_fragments_when_only_the_nested_fragment_gets_preserved() { let planner = planner!( Subgraph1: r#" @@ -463,10 +462,8 @@ fn it_works_with_nested_fragments_when_only_the_nested_fragment_gets_preserved() } #[test] -#[should_panic( - expected = "variable `$if` of type `Boolean` cannot be used for argument `if` of type `Boolean!`" -)] -// TODO: investigate this failure +#[should_panic(expected = r#"called `Result::unwrap()` on an `Err` value: "#)] +// TODO: investigate this failure (Error: variable `$if` of type `Boolean` cannot be used for argument `if` of type `Boolean!`) fn it_preserves_directives_when_fragment_not_used() { // (because used only once) let planner = planner!( @@ -651,8 +648,8 @@ fn it_does_not_try_to_apply_fragments_that_are_not_valid_for_the_subgaph() { } #[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure +#[should_panic(expected = "snapshot assertion")] +// TODO: investigate this failure - snapshot mismatch (complicated) fn it_handles_fragment_rebasing_in_a_subgraph_where_some_subtyping_relation_differs() { // This test is designed such that type `Outer` implements the interface `I` in `Subgraph1` // but not in `Subgraph2`, yet `I` exists in `Subgraph2` (but only `Inner` implements it @@ -999,8 +996,8 @@ fn it_handles_fragment_rebasing_in_a_subgraph_where_some_subtyping_relation_diff } #[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure +#[should_panic(expected = "called `Result::unwrap()` on an `Err` value")] +// TODO: investigate this failure (Object type "Outer" has no field "v") fn it_handles_fragment_rebasing_in_a_subgraph_where_some_union_membership_relation_differs() { // This test is similar to the subtyping case (it tests the same problems), but test the case // of unions instead of interfaces. diff --git a/apollo-federation/tests/query_plan/operation_optimization_tests.rs b/apollo-federation/tests/query_plan/operation_optimization_tests.rs index 3bae447e23..9ebae62e79 100644 --- a/apollo-federation/tests/query_plan/operation_optimization_tests.rs +++ b/apollo-federation/tests/query_plan/operation_optimization_tests.rs @@ -1,353 +1,3 @@ -#[test] -fn optimize_fragments_using_other_fragments_when_possible() { - // test('optimize fragments using other fragments when possible', () => { - // const schema = parseSchema(` - // type Query { - // t: I - // } - // - // interface I { - // b: Int - // u: U - // } - // - // type T1 implements I { - // a: Int - // b: Int - // u: U - // } - // - // type T2 implements I { - // x: String - // y: String - // b: Int - // u: U - // } - // - // union U = T1 | T2 - // `); - // - // const operation = parseOperation(schema, ` - // fragment OnT1 on T1 { - // a - // b - // } - // - // fragment OnT2 on T2 { - // x - // y - // } - // - // fragment OnI on I { - // b - // } - // - // fragment OnU on U { - // ...OnI - // ...OnT1 - // ...OnT2 - // } - // - // query { - // t { - // ...OnT1 - // ...OnT2 - // ...OnI - // u { - // ...OnU - // } - // } - // } - // `); - // - // const withoutFragments = operation.expandAllFragments(); - // expect(withoutFragments.toString()).toMatchString(` - // { - // t { - // ... on T1 { - // a - // b - // } - // ... on T2 { - // x - // y - // } - // b - // u { - // ... on I { - // b - // } - // ... on T1 { - // a - // b - // } - // ... on T2 { - // x - // y - // } - // } - // } - // } - // `); - // - // const optimized = withoutFragments.optimize(operation.fragments!); - // expect(optimized.toString()).toMatchString(` - // fragment OnU on U { - // ... on I { - // b - // } - // ... on T1 { - // a - // b - // } - // ... on T2 { - // x - // y - // } - // } - // - // { - // t { - // ...OnU - // u { - // ...OnU - // } - // } - // } - // `); - // }); -} - -#[test] -fn handles_fragments_using_other_fragments() { - //test('handles fragments using other fragments', () => { - // const schema = parseSchema(` - // type Query { - // t: I - // } - // - // interface I { - // b: Int - // c: Int - // u1: U - // u2: U - // } - // - // type T1 implements I { - // a: Int - // b: Int - // c: Int - // me: T1 - // u1: U - // u2: U - // } - // - // type T2 implements I { - // x: String - // y: String - // b: Int - // c: Int - // u1: U - // u2: U - // } - // - // union U = T1 | T2 - // `); - // - // const operation = parseOperation(schema, ` - // fragment OnT1 on T1 { - // a - // b - // } - // - // fragment OnT2 on T2 { - // x - // y - // } - // - // fragment OnI on I { - // b - // c - // } - // - // fragment OnU on U { - // ...OnI - // ...OnT1 - // ...OnT2 - // } - // - // query { - // t { - // ...OnT1 - // ...OnT2 - // u1 { - // ...OnU - // } - // u2 { - // ...OnU - // } - // ... on T1 { - // me { - // ...OnI - // } - // } - // } - // } - // `); - // - // const withoutFragments = operation.expandAllFragments(); - // expect(withoutFragments.toString()).toMatchString(` - // { - // t { - // ... on T1 { - // a - // b - // me { - // b - // c - // } - // } - // ... on T2 { - // x - // y - // } - // u1 { - // ... on I { - // b - // c - // } - // ... on T1 { - // a - // b - // } - // ... on T2 { - // x - // y - // } - // } - // u2 { - // ... on I { - // b - // c - // } - // ... on T1 { - // a - // b - // } - // ... on T2 { - // x - // y - // } - // } - // } - // } - // `); - // - // const optimized = withoutFragments.optimize(operation.fragments!); - // // We should reuse and keep all fragments, because 1) onU is used twice and 2) - // // all the other ones are used once in the query, and once in onU definition. - // expect(optimized.toString()).toMatchString(` - // fragment OnT1 on T1 { - // a - // b - // } - // - // fragment OnT2 on T2 { - // x - // y - // } - // - // fragment OnI on I { - // b - // c - // } - // - // fragment OnU on U { - // ...OnI - // ...OnT1 - // ...OnT2 - // } - // - // { - // t { - // ... on T1 { - // ...OnT1 - // me { - // ...OnI - // } - // } - // ...OnT2 - // u1 { - // ...OnU - // } - // u2 { - // ...OnU - // } - // } - // } - // `); - // }); -} - -#[test] -fn handles_fragments_with_nested_selections() { - //test('handles fragments with nested selections', () => { - // const schema = parseSchema(` - // type Query { - // t1a: T1 - // t2a: T1 - // } - // - // type T1 { - // t2: T2 - // } - // - // type T2 { - // x: String - // y: String - // } - // `); - // - // testFragmentsRoundtrip({ - // schema, - // query: ` - // fragment OnT1 on T1 { - // t2 { - // x - // } - // } - // - // query { - // t1a { - // ...OnT1 - // t2 { - // y - // } - // } - // t2a { - // ...OnT1 - // } - // } - // `, - // expanded: ` - // { - // t1a { - // t2 { - // x - // y - // } - // } - // t2a { - // t2 { - // x - // } - // } - // } - // `, - // }); - // }); -} - #[test] fn handles_nested_fragments_with_field_intersection() { //test('handles nested fragments with field intersection', () => { diff --git a/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_false.graphql b/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_false.graphql index 11c5d8c279..42f3923d30 100644 --- a/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_false.graphql +++ b/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_false.graphql @@ -1,4 +1,4 @@ -# Composed from subgraphs with hash: ca8bc5b745ab72ecb5d72159ed30d089d2084d77 +# Composed from subgraphs with hash: efdbea0791d6ac8577eb1b7b63618adec547f1c8 schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) diff --git a/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_true.graphql b/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_true.graphql index 11c5d8c279..42f3923d30 100644 --- a/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_true.graphql +++ b/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_true.graphql @@ -1,4 +1,4 @@ -# Composed from subgraphs with hash: ca8bc5b745ab72ecb5d72159ed30d089d2084d77 +# Composed from subgraphs with hash: efdbea0791d6ac8577eb1b7b63618adec547f1c8 schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION)