diff --git a/.changesets/breaking_drop_reuse_fragment.md b/.changesets/breaking_drop_reuse_fragment.md new file mode 100644 index 0000000000..643f061b64 --- /dev/null +++ b/.changesets/breaking_drop_reuse_fragment.md @@ -0,0 +1,7 @@ +### Drop experimental reuse fragment query optimization option ([PR #6353](https://github.com/apollographql/router/pull/6353)) + +Drop support for the experimental reuse fragment query optimization. This implementation was not only very slow but also very buggy due to its complexity. + +Auto generation of fragments is a much simpler (and faster) algorithm that in most cases produces better results. Fragment auto generation is the default optimization since v1.58 release. + +By [@dariuszkuc](https://github.com/dariuszkuc) in https://github.com/apollographql/router/pull/6353 diff --git a/apollo-federation/cli/src/main.rs b/apollo-federation/cli/src/main.rs index 28bb5f7921..cfef296154 100644 --- a/apollo-federation/cli/src/main.rs +++ b/apollo-federation/cli/src/main.rs @@ -23,9 +23,6 @@ struct QueryPlannerArgs { /// Enable @defer support. #[arg(long, default_value_t = false)] enable_defer: bool, - /// Reuse fragments to compress subgraph queries. - #[arg(long, default_value_t = false)] - reuse_fragments: bool, /// Generate fragments to compress subgraph queries. #[arg(long, default_value_t = false)] generate_fragments: bool, @@ -109,8 +106,6 @@ enum Command { impl QueryPlannerArgs { fn apply(&self, config: &mut QueryPlannerConfig) { config.incremental_delivery.enable_defer = self.enable_defer; - // --generate-fragments trumps --reuse-fragments - config.reuse_query_fragments = self.reuse_fragments && !self.generate_fragments; config.generate_query_fragments = self.generate_fragments; config.subgraph_graphql_validation = self.subgraph_validation.unwrap_or(true); if let Some(max_evaluated_plans) = self.max_evaluated_plans { diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index f93f805661..9454842f1d 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -921,17 +921,6 @@ impl FragmentSpreadSelection { }) } - pub(crate) fn from_fragment( - fragment: &Node, - directives: &executable::DirectiveList, - ) -> Self { - let spread = FragmentSpread::from_fragment(fragment, directives); - Self { - spread, - selection_set: fragment.selection_set.clone(), - } - } - /// Creates a fragment spread selection (in an optimized operation). /// - `named_fragments`: Named fragment definitions that are rebased for the element's schema. pub(crate) fn new( @@ -2171,15 +2160,6 @@ impl SelectionSet { }) } - /// In a normalized selection set containing only fields and inline fragments, - /// iterate over all the fields that may be selected. - /// - /// # Preconditions - /// The selection set must not contain named fragment spreads. - pub(crate) fn field_selections(&self) -> FieldSelectionsIter<'_> { - FieldSelectionsIter::new(self.selections.values()) - } - /// # Preconditions /// The selection set must not contain named fragment spreads. fn fields_in_set(&self) -> Vec { @@ -2320,36 +2300,6 @@ impl<'a> IntoIterator for &'a SelectionSet { } } -pub(crate) struct FieldSelectionsIter<'sel> { - stack: Vec>, -} - -impl<'sel> FieldSelectionsIter<'sel> { - fn new(iter: selection_map::Values<'sel>) -> Self { - Self { stack: vec![iter] } - } -} - -impl<'sel> Iterator for FieldSelectionsIter<'sel> { - type Item = &'sel Arc; - - fn next(&mut self) -> Option { - match self.stack.last_mut()?.next() { - None if self.stack.len() == 1 => None, - None => { - self.stack.pop(); - self.next() - } - Some(Selection::Field(field)) => Some(field), - Some(Selection::InlineFragment(frag)) => { - self.stack.push(frag.selection_set.selections.values()); - self.next() - } - Some(Selection::FragmentSpread(_frag)) => unreachable!(), - } - } -} - #[derive(Clone, Debug)] pub(crate) struct SelectionSetAtPath { path: Vec, @@ -2625,16 +2575,6 @@ impl Field { pub(crate) fn parent_type_position(&self) -> CompositeTypeDefinitionPosition { self.field_position.parent() } - - pub(crate) fn types_can_be_merged(&self, other: &Self) -> Result { - let self_definition = self.field_position.get(self.schema().schema())?; - let other_definition = other.field_position.get(self.schema().schema())?; - types_can_be_merged( - &self_definition.ty, - &other_definition.ty, - self.schema().schema(), - ) - } } impl InlineFragmentSelection { @@ -2732,23 +2672,6 @@ 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: DirectiveList, - ) -> Self { - let inline_fragment_data = InlineFragment { - schema: selection_set.schema.clone(), - parent_type_position, - type_condition_position: selection_set.type_position.clone().into(), - directives, - selection_id: SelectionId::new(), - }; - InlineFragmentSelection::new(inline_fragment_data, selection_set) - } - pub(crate) fn casted_type(&self) -> &CompositeTypeDefinitionPosition { self.inline_fragment .type_condition_position @@ -2811,31 +2734,10 @@ impl NamedFragments { NamedFragments::initialize_in_dependency_order(fragments, schema) } - pub(crate) fn is_empty(&self) -> bool { - self.fragments.len() == 0 - } - - pub(crate) fn len(&self) -> usize { - self.fragments.len() - } - pub(crate) fn iter(&self) -> impl Iterator> { self.fragments.values() } - 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)); } @@ -2929,32 +2831,6 @@ impl NamedFragments { } }) } - - /// When we rebase named fragments on a subgraph schema, only a subset of what the fragment handles may belong - /// to that particular subgraph. And there are a few sub-cases where that subset is such that we basically need or - /// want to consider to ignore the fragment for that subgraph, and that is when: - /// 1. the subset that apply is actually empty. The fragment wouldn't be valid in this case anyway. - /// 2. the subset is a single leaf field: in that case, using the one field directly is just shorter than using - /// the fragment, so we consider the fragment don't really apply to that subgraph. Technically, using the - /// fragment could still be of value if the fragment name is a lot smaller than the one field name, but it's - /// enough of a niche case that we ignore it. Note in particular that one sub-case of this rule that is likely - /// to be common is when the subset ends up being just `__typename`: this would basically mean the fragment - /// don't really apply to the subgraph, and that this will ensure this is the case. - pub(crate) fn is_selection_set_worth_using(selection_set: &SelectionSet) -> bool { - if selection_set.selections.len() == 0 { - return false; - } - if selection_set.selections.len() == 1 { - // true if NOT field selection OR non-leaf field - return if let Some(Selection::Field(field_selection)) = selection_set.selections.first() - { - field_selection.selection_set.is_some() - } else { - true - }; - } - true - } } // @defer handling: removing and normalization @@ -3393,49 +3269,6 @@ impl Operation { } } -// Collect fragment usages from operation types. - -impl Selection { - fn collect_used_fragment_names(&self, aggregator: &mut IndexMap) { - match self { - Selection::Field(field_selection) => { - if let Some(s) = &field_selection.selection_set { - s.collect_used_fragment_names(aggregator) - } - } - Selection::InlineFragment(inline) => { - inline.selection_set.collect_used_fragment_names(aggregator); - } - Selection::FragmentSpread(fragment) => { - let current_count = aggregator - .entry(fragment.spread.fragment_name.clone()) - .or_default(); - *current_count += 1; - } - } - } -} - -impl SelectionSet { - pub(crate) fn collect_used_fragment_names(&self, aggregator: &mut IndexMap) { - for s in self.selections.values() { - s.collect_used_fragment_names(aggregator); - } - } - - pub(crate) fn used_fragments(&self) -> IndexMap { - let mut usages = IndexMap::default(); - self.collect_used_fragment_names(&mut usages); - usages - } -} - -impl Fragment { - pub(crate) fn collect_used_fragment_names(&self, aggregator: &mut IndexMap) { - self.selection_set.collect_used_fragment_names(aggregator) - } -} - // Collect used variables from operation types. pub(crate) struct VariableCollector<'s> { @@ -3533,16 +3366,6 @@ impl<'s> VariableCollector<'s> { } } -impl Fragment { - /// Returns the variable names that are used by this fragment. - pub(crate) fn used_variables(&self) -> IndexSet<&'_ Name> { - let mut collector = VariableCollector::new(); - collector.visit_directive_list(&self.directives); - collector.visit_selection_set(&self.selection_set); - collector.into_inner() - } -} - impl SelectionSet { /// Returns the variable names that are used by this selection set, including through fragment /// spreads. @@ -3905,7 +3728,9 @@ pub(crate) fn normalize_operation( variables: Arc::new(operation.variables.clone()), directives: operation.directives.clone().into(), selection_set: normalized_selection_set, - named_fragments, + // fragments were already expanded into selection sets + // new ones will be generated when optimizing the final subgraph fetch operations + named_fragments: Default::default(), }; Ok(normalized_operation) } diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index c7e54b23f7..7bdd0842a2 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -38,17 +38,10 @@ use std::sync::Arc; use apollo_compiler::collections::IndexMap; -use apollo_compiler::collections::IndexSet; use apollo_compiler::executable; -use apollo_compiler::executable::VariableDefinition; use apollo_compiler::Name; use apollo_compiler::Node; -use super::Containment; -use super::ContainmentOptions; -use super::DirectiveList; -use super::Field; -use super::FieldSelection; use super::Fragment; use super::FragmentSpreadSelection; use super::HasSelectionKey; @@ -62,152 +55,6 @@ use super::SelectionSet; use crate::error::FederationError; use crate::operation::FragmentSpread; use crate::operation::SelectionValue; -use crate::schema::position::CompositeTypeDefinitionPosition; - -#[derive(Debug)] -struct ReuseContext<'a> { - fragments: &'a NamedFragments, - operation_variables: Option>, -} - -impl<'a> ReuseContext<'a> { - fn for_fragments(fragments: &'a NamedFragments) -> Self { - Self { - fragments, - operation_variables: None, - } - } - - // Taking two separate parameters so the caller can still mutate the operation's selection set. - fn for_operation( - fragments: &'a NamedFragments, - operation_variables: &'a [Node], - ) -> Self { - Self { - fragments, - operation_variables: Some(operation_variables.iter().map(|var| &var.name).collect()), - } - } -} - -//============================================================================= -// Add __typename field for abstract types in named fragment definitions - -impl NamedFragments { - // - Expands all nested fragments - // - Applies the provided `mapper` to each selection set of the expanded fragments. - // - Finally, re-fragments the nested fragments. - // - `mapper` must return a fragment-spread-free selection set. - fn map_to_expanded_selection_sets( - &self, - mut mapper: impl FnMut(&SelectionSet) -> Result, - ) -> Result { - let mut result = NamedFragments::default(); - // Note: `self.fragments` has insertion order topologically sorted. - for fragment in self.fragments.values() { - let expanded_selection_set = fragment - .selection_set - .expand_all_fragments()? - .flatten_unnecessary_fragments( - &fragment.type_condition_position, - &Default::default(), - &fragment.schema, - )?; - let mut mapped_selection_set = mapper(&expanded_selection_set)?; - // `mapped_selection_set` must be fragment-spread-free. - mapped_selection_set.reuse_fragments(&ReuseContext::for_fragments(&result))?; - let updated = Fragment { - selection_set: mapped_selection_set, - schema: fragment.schema.clone(), - name: fragment.name.clone(), - type_condition_position: fragment.type_condition_position.clone(), - directives: fragment.directives.clone(), - }; - result.insert(updated); - } - Ok(result) - } - - pub(crate) fn add_typename_field_for_abstract_types_in_named_fragments( - &self, - ) -> Result { - // This method is a bit tricky due to potentially nested fragments. More precisely, suppose that - // we have: - // fragment MyFragment on T { - // a { - // b { - // ...InnerB - // } - // } - // } - // - // fragment InnerB on B { - // __typename - // x - // y - // } - // then if we were to "naively" add `__typename`, the first fragment would end up being: - // fragment MyFragment on T { - // a { - // __typename - // b { - // __typename - // ...InnerX - // } - // } - // } - // but that's not ideal because the inner-most `__typename` is already within `InnerX`. And that - // gets in the way to re-adding fragments (the `SelectionSet::reuse_fragments` method) because if we start - // with: - // { - // a { - // __typename - // b { - // __typename - // x - // y - // } - // } - // } - // and add `InnerB` first, we get: - // { - // a { - // __typename - // b { - // ...InnerB - // } - // } - // } - // and it becomes tricky to recognize the "updated-with-typename" version of `MyFragment` now (we "seem" - // to miss a `__typename`). - // - // Anyway, to avoid this issue, what we do is that for every fragment, we: - // 1. expand any nested fragments in its selection. - // 2. add `__typename` where we should in that expanded selection. - // 3. re-optimize all fragments (using the "updated-with-typename" versions). - // which is what `mapToExpandedSelectionSets` gives us. - - if self.is_empty() { - // PORT_NOTE: This was an assertion failure in JS version. But, it's actually ok to - // return unchanged if empty. - return Ok(self.clone()); - } - let updated = self.map_to_expanded_selection_sets(|ss| { - // Note: Since `ss` won't have any fragment spreads, `add_typename_field_for_abstract_types`'s return - // value won't have any fragment spreads. - ss.add_typename_field_for_abstract_types(/*parent_type_if_abstract*/ None) - })?; - // PORT_NOTE: The JS version asserts if `updated` is empty or not. But, we really want to - // check the `updated` has the same set of fragments. To avoid performance hit, only the - // size is checked here. - if updated.len() != self.len() { - return Err(FederationError::internal( - "Unexpected change in the number of fragments", - )); - } - Ok(updated) - } -} //============================================================================= // Selection/SelectionSet intersection/minus operations @@ -234,26 +81,6 @@ impl Selection { } Ok(None) } - - /// Computes the set-intersection of self and other - /// - If there are respective sub-selections, then we compute their intersections and add them - /// (if not empty). - /// - Otherwise, the intersection is same as `self`. - fn intersection(&self, other: &Selection) -> Result, FederationError> { - if let (Some(self_sub_selection), Some(other_sub_selection)) = - (self.selection_set(), other.selection_set()) - { - let common = self_sub_selection.intersection(other_sub_selection)?; - if common.is_empty() { - return Ok(None); - } else { - return self - .with_updated_selections(self_sub_selection.type_position.clone(), common) - .map(Some); - } - } - Ok(Some(self.clone())) - } } impl SelectionSet { @@ -279,1549 +106,279 @@ impl SelectionSet { iter, )) } - - /// Computes the set-intersection of self and other - fn intersection(&self, other: &SelectionSet) -> Result { - if self.is_empty() { - return Ok(self.clone()); - } - if other.is_empty() { - return Ok(other.clone()); - } - - let iter = self - .selections - .values() - .map(|v| { - if let Some(other_v) = other.selections.get(v.key()) { - v.intersection(other_v) - } else { - Ok(None) - } - }) - .collect::, _>>()? // early break in case of Err - .into_iter() - .flatten(); - Ok(SelectionSet::from_raw_selections( - self.schema.clone(), - self.type_position.clone(), - iter, - )) - } } //============================================================================= -// Collect applicable fragments at given type. - -impl Fragment { - /// Whether this fragment may apply _directly_ at the provided type, meaning that the fragment - /// sub-selection (_without_ the fragment condition, hence the "directly") can be normalized at - /// `ty` without overly "widening" the runtime types. - /// - /// * `ty` - the type at which we're looking at applying the fragment - // - // The runtime types of the fragment condition must be at least as general as those of the - // provided `ty`. Otherwise, putting it at `ty` without its condition would "generalize" - // more than the fragment meant to (and so we'd "widen" the runtime types more than what the - // query meant to. - fn can_apply_directly_at_type( - &self, - ty: &CompositeTypeDefinitionPosition, - ) -> Result { - // Short-circuit #1: the same type => trivially true. - if self.type_condition_position == *ty { - return Ok(true); - } - - // Short-circuit #2: The type condition is not an abstract type (too restrictive). - // - It will never cover all of the runtime types of `ty` unless it's the same type, which is - // already checked by short-circuit #1. - if !self.type_condition_position.is_abstract_type() { - return Ok(false); - } - - // Short-circuit #3: The type condition is not an object (due to short-circuit #2) nor a - // union type, but the `ty` may be too general. - // - In other words, the type condition must be an interface but `ty` is a (different) - // interface or a union. - // PORT_NOTE: In JS, this check was later on the return statement (negated). But, this - // should be checked before `possible_runtime_types` check, since this is - // cheaper to execute. - // PORT_NOTE: This condition may be too restrictive (potentially a bug leading to - // suboptimal compression). If ty is a union whose members all implements the - // type condition (interface). Then, this function should've returned true. - // Thus, `!ty.is_union_type()` might be needed. - if !self.type_condition_position.is_union_type() && !ty.is_object_type() { - return Ok(false); - } +// Matching fragments with selection set (`try_optimize_with_fragments`) - // Check if the type condition is a superset of the provided type. - // - The fragment condition must be at least as general as the provided type. - let condition_types = self - .schema - .possible_runtime_types(self.type_condition_position.clone())?; - let ty_types = self.schema.possible_runtime_types(ty.clone())?; - Ok(condition_types.is_superset(&ty_types)) - } +/// The return type for `SelectionSet::try_optimize_with_fragments`. +#[derive(derive_more::From)] +enum SelectionSetOrFragment { + SelectionSet(SelectionSet), + Fragment(Node), } -impl NamedFragments { - /// Returns fragments that can be applied directly at the given type. - fn get_all_may_apply_directly_at_type<'a>( - &'a self, - ty: &'a CompositeTypeDefinitionPosition, - ) -> impl Iterator, FederationError>> + 'a { - self.iter().filter_map(|fragment| { - fragment - .can_apply_directly_at_type(ty) - .map(|can_apply| can_apply.then_some(fragment)) - .transpose() - }) +// 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() + } + } } } //============================================================================= -// Field validation +// `reuse_fragments` methods (putting everything together) -// PORT_NOTE: Not having a validator and having a FieldsConflictValidator with empty -// `by_response_name` map has no difference in behavior. So, we could drop the `Option` from -// `Option`. However, `None` validator makes it clearer that validation is -// unnecessary. -struct FieldsConflictValidator { - by_response_name: IndexMap>>>, +/// Return type for `InlineFragmentSelection::reuse_fragments`. +#[derive(derive_more::From)] +enum FragmentSelection { + // Note: Enum variants are named to match those of `Selection`. + InlineFragment(InlineFragmentSelection), + FragmentSpread(FragmentSpreadSelection), } -impl FieldsConflictValidator { - /// Build a field merging validator for a selection set. - /// - /// # Preconditions - /// The selection set must not contain named fragment spreads. - fn from_selection_set(selection_set: &SelectionSet) -> Self { - Self::for_level(&[selection_set]) - } - - fn for_level<'a>(level: &[&'a SelectionSet]) -> Self { - // Group `level`'s fields by the response-name/field - let mut at_level: IndexMap>> = - IndexMap::default(); - for selection_set in level { - for field_selection in selection_set.field_selections() { - let response_name = field_selection.field.response_name(); - let at_response_name = at_level.entry(response_name.clone()).or_default(); - let entry = at_response_name - .entry(field_selection.field.clone()) - .or_default(); - if let Some(ref field_selection_set) = field_selection.selection_set { - entry.push(field_selection_set); - } - } - } - - // Collect validators per response-name/field - let mut by_response_name = IndexMap::default(); - for (response_name, fields) in at_level { - let mut at_response_name: IndexMap>> = - IndexMap::default(); - for (field, selection_sets) in fields { - if selection_sets.is_empty() { - at_response_name.insert(field, None); - } else { - let validator = Arc::new(Self::for_level(&selection_sets)); - at_response_name.insert(field, Some(validator)); - } - } - by_response_name.insert(response_name, at_response_name); - } - Self { by_response_name } - } - - fn for_field<'v>(&'v self, field: &Field) -> impl Iterator> + 'v { - self.by_response_name - .get(field.response_name()) - .into_iter() - .flat_map(|by_response_name| by_response_name.values()) - .flatten() - .cloned() - } - - fn has_same_response_shape( - &self, - other: &FieldsConflictValidator, - ) -> Result { - for (response_name, self_fields) in self.by_response_name.iter() { - let Some(other_fields) = other.by_response_name.get(response_name) else { - continue; - }; - - for (self_field, self_validator) in self_fields { - for (other_field, other_validator) in other_fields { - if !self_field.types_can_be_merged(other_field)? { - return Ok(false); - } - - if let Some(self_validator) = self_validator { - if let Some(other_validator) = other_validator { - if !self_validator.has_same_response_shape(other_validator)? { - return Ok(false); - } - } - } - } - } - } - Ok(true) - } - - fn do_merge_with(&self, other: &FieldsConflictValidator) -> Result { - for (response_name, self_fields) in self.by_response_name.iter() { - let Some(other_fields) = other.by_response_name.get(response_name) else { - continue; - }; - - // We're basically checking - // [FieldsInSetCanMerge](https://spec.graphql.org/draft/#FieldsInSetCanMerge()), but - // from 2 set of fields (`self_fields` and `other_fields`) of the same response that we - // know individually merge already. - for (self_field, self_validator) in self_fields { - for (other_field, other_validator) in other_fields { - if !self_field.types_can_be_merged(other_field)? { - return Ok(false); - } - - let p1 = self_field.parent_type_position(); - let p2 = other_field.parent_type_position(); - if p1 == p2 || !p1.is_object_type() || !p2.is_object_type() { - // Additional checks of `FieldsInSetCanMerge` when same parent type or one - // isn't object - if self_field.name() != other_field.name() - || self_field.arguments != other_field.arguments - { - return Ok(false); - } - if let (Some(self_validator), Some(other_validator)) = - (self_validator, other_validator) - { - if !self_validator.do_merge_with(other_validator)? { - return Ok(false); - } - } - } else { - // Otherwise, the sub-selection must pass - // [SameResponseShape](https://spec.graphql.org/draft/#SameResponseShape()). - if let (Some(self_validator), Some(other_validator)) = - (self_validator, other_validator) - { - if !self_validator.has_same_response_shape(other_validator)? { - return Ok(false); - } - } - } - } - } +impl From for Selection { + fn from(value: FragmentSelection) -> Self { + match value { + FragmentSelection::InlineFragment(inline_fragment) => inline_fragment.into(), + FragmentSelection::FragmentSpread(fragment_spread) => fragment_spread.into(), } - Ok(true) - } - - fn do_merge_with_all<'a>( - &self, - mut iter: impl Iterator, - ) -> Result { - iter.try_fold(true, |acc, v| Ok(acc && v.do_merge_with(self)?)) } } -struct FieldsConflictMultiBranchValidator { - validators: Vec>, - used_spread_trimmed_part_at_level: Vec>, -} - -impl FieldsConflictMultiBranchValidator { - fn new(validators: Vec>) -> Self { - Self { - validators, - used_spread_trimmed_part_at_level: Vec::new(), - } - } - - fn from_initial_validator(validator: FieldsConflictValidator) -> Self { - Self { - validators: vec![Arc::new(validator)], - used_spread_trimmed_part_at_level: Vec::new(), - } - } - - fn for_field(&self, field: &Field) -> Self { - let for_all_branches = self.validators.iter().flat_map(|v| v.for_field(field)); - Self::new(for_all_branches.collect()) - } - - // When this method is used in the context of `try_optimize_with_fragments`, we know that the - // fragment, restricted to the current parent type, matches a subset of the sub-selection. - // However, there is still one case we we cannot use it that we need to check, and this is if - // using the fragment would create a field "conflict" (in the sense of the graphQL spec - // [`FieldsInSetCanMerge`](https://spec.graphql.org/draft/#FieldsInSetCanMerge())) and thus - // create an invalid selection. To be clear, `at_type.selections` cannot create a conflict, - // since it is a subset of the target selection set and it is valid by itself. *But* there may - // be some part of the fragment that is not `at_type.selections` due to being "dead branches" - // for type `parent_type`. And while those branches _are_ "dead" as far as execution goes, the - // `FieldsInSetCanMerge` validation does not take this into account (it's 1st step says - // "including visiting fragments and inline fragments" but has no logic regarding ignoring any - // fragment that may not apply due to the intersection of runtime types between multiple - // fragment being empty). - fn check_can_reuse_fragment_and_track_it( - &mut self, - fragment_restriction: &FragmentRestrictionAtType, - ) -> Result { - // No validator means that everything in the fragment selection was part of the selection - // we're optimizing away (by using the fragment), and we know the original selection was - // ok, so nothing to check. - let Some(validator) = &fragment_restriction.validator else { - return Ok(true); // Nothing to check; Trivially ok. - }; - - if !validator.do_merge_with_all(self.validators.iter().map(Arc::as_ref))? { - return Ok(false); - } - - // We need to make sure the trimmed parts of `fragment` merges with the rest of the - // selection, but also that it merge with any of the trimmed parts of any fragment we have - // added already. - // Note: this last condition means that if 2 fragment conflict on their "trimmed" parts, - // then the choice of which is used can be based on the fragment ordering and selection - // order, which may not be optimal. This feels niche enough that we keep it simple for now, - // but we can revisit this decision if we run into real cases that justify it (but making - // it optimal would be a involved in general, as in theory you could have complex - // dependencies of fragments that conflict, even cycles, and you need to take the size of - // fragments into account to know what's best; and even then, this could even depend on - // overall usage, as it can be better to reuse a fragment that is used in other places, - // than to use one for which it's the only usage. Adding to all that the fact that conflict - // can happen in sibling branches). - if !validator.do_merge_with_all( - self.used_spread_trimmed_part_at_level - .iter() - .map(Arc::as_ref), - )? { - return Ok(false); - } - - // We're good, but track the fragment. - self.used_spread_trimmed_part_at_level - .push(validator.clone()); - Ok(true) +impl Operation { + /// Optimize the parsed size of the operation by generating fragments based on the selections + /// in the operation. + pub(crate) fn generate_fragments(&mut self) -> Result<(), FederationError> { + // Currently, this method simply pulls out every inline fragment into a named fragment. If + // multiple inline fragments are the same, they use the same named fragment. + // + // This method can generate named fragments that are only used once. It's not ideal, but it + // also doesn't seem that bad. Avoiding this is possible but more work, and keeping this + // as simple as possible is a big benefit for now. + // + // When we have more advanced correctness testing, we can add more features to fragment + // generation, like factoring out partial repeated slices of selection sets or only + // introducing named fragments for patterns that occur more than once. + let mut generator = FragmentGenerator::default(); + generator.visit_selection_set(&mut self.selection_set)?; + self.named_fragments = generator.into_inner(); + Ok(()) } } -//============================================================================= -// Matching fragments with selection set (`try_optimize_with_fragments`) - -/// Return type for `expanded_selection_set_at_type` method. -struct FragmentRestrictionAtType { - /// Selections that are expanded from a given fragment at a given type and then normalized. - /// - This represents the part of given type's sub-selections that are covered by the fragment. - selections: SelectionSet, - - /// A runtime validator to check the fragment selections against other fields. - /// - `None` means that there is nothing to check. - /// - See `check_can_reuse_fragment_and_track_it` for more details. - validator: Option>, +#[derive(Debug, Default)] +struct FragmentGenerator { + fragments: NamedFragments, + // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! + names: IndexMap<(String, usize), usize>, } -#[derive(Default)] -struct FragmentRestrictionAtTypeCache { - map: IndexMap<(Name, CompositeTypeDefinitionPosition), Arc>, -} +impl FragmentGenerator { + // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! + // In the future, we will just use `.next_name()`. + fn generate_name(&mut self, frag: &InlineFragmentSelection) -> Name { + use std::fmt::Write as _; -impl FragmentRestrictionAtTypeCache { - fn expanded_selection_set_at_type( - &mut self, - fragment: &Fragment, - ty: &CompositeTypeDefinitionPosition, - ) -> Result, FederationError> { - // I would like to avoid the Arc here, it seems unnecessary, but with `.entry()` - // the lifetime does not really want to work out. - // (&'cache mut self) -> Result<&'cache FragmentRestrictionAtType> - match self.map.entry((fragment.name.clone(), ty.clone())) { - indexmap::map::Entry::Occupied(entry) => Ok(Arc::clone(entry.get())), - indexmap::map::Entry::Vacant(entry) => Ok(Arc::clone( - entry.insert(Arc::new(fragment.expanded_selection_set_at_type(ty)?)), - )), - } - } -} + let type_condition = frag + .inline_fragment + .type_condition_position + .as_ref() + .map_or_else( + || "undefined".to_string(), + |condition| condition.to_string(), + ); + let selections = frag.selection_set.selections.len(); + let mut name = format!("_generated_on{type_condition}{selections}"); -impl FragmentRestrictionAtType { - fn new(selections: SelectionSet, validator: Option) -> Self { - Self { - selections, - validator: validator.map(Arc::new), - } + let key = (type_condition, selections); + let index = self + .names + .entry(key) + .and_modify(|index| *index += 1) + .or_default(); + _ = write!(&mut name, "_{index}"); + + Name::new_unchecked(&name) } - // It's possible that while the fragment technically applies at `parent_type`, it's "rebasing" on - // `parent_type` is empty, or contains only `__typename`. For instance, suppose we have - // a union `U = A | B | C`, and then a fragment: - // ```graphql - // fragment F on U { - // ... on A { - // x - // } - // ... on B { - // y - // } - // } - // ``` - // It is then possible to apply `F` when the parent type is `C`, but this ends up selecting - // nothing at all. - // - // Using `F` in those cases is, while not 100% incorrect, at least not productive, and so we - // skip it that case. This is essentially an optimization. - fn is_useless(&self) -> bool { - let mut iter = self.selections.iter(); + /// Is a selection set worth using for a newly generated named fragment? + fn is_worth_using(selection_set: &SelectionSet) -> bool { + let mut iter = selection_set.iter(); let Some(first) = iter.next() else { + // An empty selection is not worth using (and invalid!) + return false; + }; + let Selection::Field(field) = first else { return true; }; - iter.next().is_none() && first.is_typename_field() - } -} - -impl Fragment { - /// Computes the expanded selection set of this fragment along with its validator to check - /// against other fragments applied under the same selection set. - fn expanded_selection_set_at_type( - &self, - ty: &CompositeTypeDefinitionPosition, - ) -> Result { - let expanded_selection_set = self.selection_set.expand_all_fragments()?; - let selection_set = expanded_selection_set.flatten_unnecessary_fragments( - ty, - /*named_fragments*/ &Default::default(), - &self.schema, - )?; - - if !self.type_condition_position.is_object_type() { - // When the type condition of the fragment is not an object type, the - // `FieldsInSetCanMerge` rule is more restrictive and any fields can create conflicts. - // Thus, we have to use the full validator in this case. (see - // https://github.com/graphql/graphql-spec/issues/1085 for details.) - return Ok(FragmentRestrictionAtType::new( - selection_set.clone(), - Some(FieldsConflictValidator::from_selection_set( - &expanded_selection_set, - )), - )); - } - - // Use a smaller validator for efficiency. - // Note that `trimmed` is the difference of 2 selections that may not have been normalized - // on the same parent type, so in practice, it is possible that `trimmed` contains some of - // the selections that `selectionSet` contains, but that they have been simplified in - // `selectionSet` in such a way that the `minus` call does not see it. However, it is not - // trivial to deal with this, and it is fine given that we use trimmed to create the - // validator because we know the non-trimmed parts cannot create field conflict issues so - // we're trying to build a smaller validator, but it's ok if trimmed is not as small as it - // theoretically can be. - let trimmed = expanded_selection_set.minus(&selection_set)?; - let validator = - (!trimmed.is_empty()).then(|| FieldsConflictValidator::from_selection_set(&trimmed)); - Ok(FragmentRestrictionAtType::new( - selection_set.clone(), - validator, - )) - } - - /// Checks whether `self` fragment includes the other fragment (`other_fragment_name`). - // - // Note that this is slightly different from `self` "using" `other_fragment` in that this - // essentially checks if the full selection set of `other_fragment` is contained by `self`, so - // this only look at "top-level" usages. - // - // Note that this is guaranteed to return `false` if passed self's name. - // Note: This is a heuristic looking for the other named fragment used directly in the - // selection set. It may not return `true` even though the other fragment's selections - // are actually covered by self's selection set. - // PORT_NOTE: The JS version memoizes the result of this function. But, the current Rust port - // does not. - fn includes(&self, other_fragment_name: &Name) -> bool { - if self.name == *other_fragment_name { - return false; - } - - self.selection_set.selections.values().any(|selection| { - matches!( - selection, - Selection::FragmentSpread(fragment) if fragment.spread.fragment_name == *other_fragment_name - ) - }) + // If there's more than one selection, or one selection with a subselection, + // it's probably worth using + iter.next().is_some() || field.selection_set.is_some() } -} - -enum FullMatchingFragmentCondition<'a> { - ForFieldSelection, - ForInlineFragmentSelection { - // the type condition and directives on an inline fragment selection. - type_condition_position: &'a CompositeTypeDefinitionPosition, - directives: &'a DirectiveList, - }, -} -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(), + /// Modify the selection set so that eligible inline fragments are moved to named fragment spreads. + fn visit_selection_set( + &mut self, + selection_set: &mut SelectionSet, + ) -> Result<(), FederationError> { + let mut new_selection_set = SelectionSet::empty( + selection_set.schema.clone(), + selection_set.type_position.clone(), + ); - // 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; + for selection in Arc::make_mut(&mut selection_set.selections).values_mut() { + match selection { + SelectionValue::Field(mut field) => { + if let Some(selection_set) = field.get_selection_set_mut() { + self.visit_selection_set(selection_set)?; + } + new_selection_set + .add_local_selection(&Selection::Field(Arc::clone(field.get())))?; + } + SelectionValue::FragmentSpread(frag) => { + new_selection_set + .add_local_selection(&Selection::FragmentSpread(Arc::clone(frag.get())))?; } + SelectionValue::InlineFragment(frag) + if !Self::is_worth_using(&frag.get().selection_set) => + { + new_selection_set + .add_local_selection(&Selection::InlineFragment(Arc::clone(frag.get())))?; + } + SelectionValue::InlineFragment(mut candidate) => { + self.visit_selection_set(candidate.get_selection_set_mut())?; - // 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 { - SelectionSet(SelectionSet), - Fragment(Node), -} - -impl SelectionSet { - /// Reduce the list of applicable fragments by eliminating fragments that directly include - /// another fragment. - // - // We have found the list of fragments that applies to some subset of sub-selection. In - // general, we want to now produce the selection set with spread for those fragments plus - // any selection that is not covered by any of the fragments. For instance, suppose that - // `subselection` is `{ a b c d e }` and we have found that `fragment F1 on X { a b c }` - // and `fragment F2 on X { c d }` applies, then we will generate `{ ...F1 ...F2 e }`. - // - // In that example, `c` is covered by both fragments. And this is fine in this example as - // it is worth using both fragments in general. A special case of this however is if a - // fragment is entirely included into another. That is, consider that we now have `fragment - // F1 on X { a ...F2 }` and `fragment F2 on X { b c }`. In that case, the code above would - // still match both `F1 and `F2`, but as `F1` includes `F2` already, we really want to only - // use `F1`. So in practice, we filter away any fragment spread that is known to be - // included in another one that applies. - // - // TODO: note that the logic used for this is theoretically a bit sub-optimal. That is, we - // only check if one of the fragment happens to directly include a spread for another - // fragment at top-level as in the example above. We do this because it is cheap to check - // and is likely the most common case of this kind of inclusion. But in theory, we would - // have `fragment F1 on X { a b c }` and `fragment F2 on X { b c }`, in which case `F2` is - // still included in `F1`, but we'd have to work harder to figure this out and it's unclear - // it's a good tradeoff. And while you could argue that it's on the user to define its - // fragments a bit more optimally, it's actually a tad more complex because we're looking - // at fragments in a particular context/parent type. Consider an interface `I` and: - // ```graphql - // fragment F3 on I { - // ... on X { - // a - // } - // ... on Y { - // b - // c - // } - // } - // - // fragment F4 on I { - // ... on Y { - // c - // } - // ... on Z { - // d - // } - // } - // ``` - // In that case, neither fragment include the other per-se. But what if we have - // sub-selection `{ b c }` but where parent type is `Y`. In that case, both `F3` and `F4` - // applies, and in that particular context, `F3` is fully included in `F4`. Long story - // short, we'll currently return `{ ...F3 ...F4 }` in that case, but it would be - // technically better to return only `F4`. However, this feels niche, and it might be - // costly to verify such inclusions, so not doing it for now. - fn reduce_applicable_fragments( - applicable_fragments: &mut Vec<(Node, Arc)>, - ) { - // Note: It's not possible for two fragments to include each other. So, we don't need to - // worry about inclusion cycles. - let included_fragments: IndexSet = applicable_fragments - .iter() - .filter(|(fragment, _)| { - applicable_fragments - .iter() - .any(|(other_fragment, _)| other_fragment.includes(&fragment.name)) - }) - .map(|(fragment, _)| fragment.name.clone()) - .collect(); - - applicable_fragments.retain(|(fragment, _)| !included_fragments.contains(&fragment.name)); - } - - /// Try to reuse existing fragments to optimize this selection set. - /// Returns either - /// - a new selection set partially optimized by re-using given `fragments`, or - /// - a single fragment that covers the full selection set. - // PORT_NOTE: Moved from `Selection` class in JS code to SelectionSet struct in Rust. - // PORT_NOTE: `parent_type` argument seems always to be the same as `self.type_position`. - // PORT_NOTE: In JS, this was called `tryOptimizeWithFragments`. - fn try_apply_fragments( - &self, - parent_type: &CompositeTypeDefinitionPosition, - context: &ReuseContext<'_>, - validator: &mut FieldsConflictMultiBranchValidator, - fragments_at_type: &mut FragmentRestrictionAtTypeCache, - 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 - // fragment condition would be needed inside `parent_type`, then that condition will not - // have been "normalized away" and so we want for this very call to be called on the - // fragment whose type _is_ the fragment condition (at which point, this - // `can_apply_directly_at_type` method will apply. Also note that this is because we have - // this restriction that calling `expanded_selection_set_at_type` is ok. - let candidates = context - .fragments - .get_all_may_apply_directly_at_type(parent_type); - - // First, we check which of the candidates do apply inside the selection set, if any. If we - // find a candidate that applies to the whole selection set, then we stop and only return - // that one candidate. Otherwise, we cumulate in `applicable_fragments` the list of fragments - // that applies to a subset. - let mut applicable_fragments = Vec::new(); - for candidate in candidates { - let candidate = candidate?; - let at_type = - fragments_at_type.expanded_selection_set_at_type(candidate, parent_type)?; - if at_type.is_useless() { - continue; - } - - // I don't love this, but fragments may introduce new fields to the operation, including - // fields that use variables that are not declared in the operation. There are two ways - // to work around this: adjusting the fragments so they only list the fields that we - // actually need, or excluding fragments that introduce variable references from reuse. - // The former would be ideal, as we would not execute more fields than required. It's - // also much trickier to do. The latter fixes this particular issue but leaves the - // output in a less than ideal state. - // The consideration here is: `generate_query_fragments` has significant advantages - // over fragment reuse, and so we do not want to invest a lot of time into improving - // fragment reuse. We do the simple, less-than-ideal thing. - if let Some(variable_definitions) = &context.operation_variables { - let fragment_variables = candidate.used_variables(); - if fragment_variables - .difference(variable_definitions) - .next() - .is_some() - { - continue; - } - } - - // As we check inclusion, we ignore the case where the fragment queries __typename - // but the `self` does not. The rational is that querying `__typename` - // unnecessarily is mostly harmless (it always works and it's super cheap) so we - // don't want to not use a fragment just to save querying a `__typename` in a few - // cases. But the underlying context of why this matters is that the query planner - // always requests __typename for abstract type, and will do so in fragments too, - // but we can have a field that _does_ return an abstract type within a fragment, - // but that _does not_ end up returning an abstract type when applied in a "more - // specific" context (think a fragment on an interface I1 where a inside field - // returns another interface I2, but applied in the context of a implementation - // type of I1 where that particular field returns an implementation of I2 rather - // than I2 directly; we would have added __typename to the fragment (because it's - // all interfaces), but the selection itself, which only deals with object type, - // may not have __typename requested; using the fragment might still be a good - // idea, and querying __typename needlessly is a very small price to pay for that). - let res = self.containment( - &at_type.selections, - ContainmentOptions { - ignore_missing_typename: true, - }, - ); - match res { - Containment::Equal if full_match_condition.check(candidate) => { - if !validator.check_can_reuse_fragment_and_track_it(&at_type)? { - // We cannot use it at all, so no point in adding to `applicable_fragments`. + // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! + // JS federation does not consider fragments without a type condition. + if candidate + .get() + .inline_fragment + .type_condition_position + .is_none() + { + new_selection_set.add_local_selection(&Selection::InlineFragment( + Arc::clone(candidate.get()), + ))?; continue; } - // Special case: Found a fragment that covers the full selection set. - return Ok(candidate.clone().into()); - } - // Note that if a fragment applies to only a subset of the sub-selections, then we - // really only can use it if that fragment is defined _without_ directives. - Containment::Equal | Containment::StrictlyContained - if candidate.directives.is_empty() => - { - applicable_fragments.push((candidate.clone(), at_type)); - } - // Not eligible; Skip it. - _ => (), - } - } - - if applicable_fragments.is_empty() { - return Ok(self.clone().into()); // Not optimizable - } - // Narrow down the list of applicable fragments by removing those that are included in - // another. - Self::reduce_applicable_fragments(&mut applicable_fragments); - - // Build a new optimized selection set. - let mut not_covered_so_far = self.clone(); - let mut optimized = SelectionSet::empty(self.schema.clone(), self.type_position.clone()); - for (fragment, at_type) in applicable_fragments { - if !validator.check_can_reuse_fragment_and_track_it(&at_type)? { - continue; - } - let not_covered = self.minus(&at_type.selections)?; - not_covered_so_far = not_covered_so_far.intersection(¬_covered)?; + let directives = &candidate.get().inline_fragment.directives; + let skip_include = directives + .iter() + .map(|directive| match directive.name.as_str() { + "skip" | "include" => Ok(directive.clone()), + _ => Err(()), + }) + .collect::>(); - // PORT_NOTE: The JS version uses `parent_type` as the "sourceType", which may be - // different from `fragment.type_condition_position`. But, Rust version does - // not have "sourceType" field for `FragmentSpreadSelection`. - let fragment_selection = FragmentSpreadSelection::from_fragment( - &fragment, - /*directives*/ &Default::default(), - ); - optimized.add_local_selection(&fragment_selection.into())?; - } + // If there are any directives *other* than @skip and @include, + // we can't just transfer them to the generated fragment spread, + // so we have to keep this inline fragment. + let Ok(skip_include) = skip_include else { + new_selection_set.add_local_selection(&Selection::InlineFragment( + Arc::clone(candidate.get()), + ))?; + continue; + }; - optimized.add_local_selection_set(¬_covered_so_far)?; - Ok(optimized.into()) - } -} + // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! + // JS does not special-case @skip and @include. It never extracts a fragment if + // there's any directives on it. This code duplicates the body from the + // previous condition so it's very easy to remove when we're ready :) + if !skip_include.is_empty() { + new_selection_set.add_local_selection(&Selection::InlineFragment( + Arc::clone(candidate.get()), + ))?; + continue; + } -//============================================================================= -// Retain fragments in selection sets while expanding the rest + let existing = self.fragments.iter().find(|existing| { + existing.type_condition_position + == candidate.get().inline_fragment.casted_type() + && existing.selection_set == candidate.get().selection_set + }); -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.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.type_condition_position - && fragment.spread.directives.is_empty() - { - // 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()) + let existing = if let Some(existing) = existing { + existing } else { - // Create an inline fragment since type condition is necessary. - let inline = InlineFragmentSelection::from_selection_set( - parent_type.clone(), - expanded_sub_selections, - fragment.spread.directives.clone(), - ); - Ok(Selection::from(inline).into()) - } + // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! + // This should be reverted to `self.next_name();` when we're ready. + let name = self.generate_name(candidate.get()); + self.fragments.insert(Fragment { + schema: selection_set.schema.clone(), + name: name.clone(), + type_condition_position: candidate.get().inline_fragment.casted_type(), + directives: Default::default(), + selection_set: candidate.get().selection_set.clone(), + }); + self.fragments.get(&name).unwrap() + }; + new_selection_set.add_local_selection(&Selection::from( + FragmentSpreadSelection { + spread: FragmentSpread { + schema: selection_set.schema.clone(), + fragment_name: existing.name.clone(), + type_condition_position: existing.type_condition_position.clone(), + directives: skip_include.into(), + fragment_directives: existing.directives.clone(), + selection_id: crate::operation::SelectionId::new(), + }, + selection_set: existing.selection_set.clone(), + }, + ))?; } } - - // 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 { - /// Updates `self` by computing the reduced set of NamedFragments that are used in the - /// selection set and other fragments 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. - /// - Returned selection set will be normalized. - fn reduce( - &mut self, - selection_set: &SelectionSet, - min_usage_to_optimize: u32, - ) -> Result { - // Call `reduce_inner` repeatedly until we reach a fix-point, since newly computed - // selection set may drop some fragment references due to normalization, which could lead - // to further reduction. - // - It is hard to avoid this chain reaction, since we need to account for the effects of - // normalization. - let mut last_size = self.len(); - let mut last_selection_set = selection_set.clone(); - while last_size > 0 { - let new_selection_set = - self.reduce_inner(&last_selection_set, min_usage_to_optimize)?; - - // Reached a fix-point => stop - if self.len() == last_size { - // Assumes that `new_selection_set` is the same as `last_selection_set` in this - // case. - break; - } - - // If we've expanded some fragments but kept others, then it's not 100% impossible that - // some fragment was used multiple times in some expanded fragment(s), but that - // post-expansion all of it's usages are "dead" branches that are removed by the final - // `flatten_unnecessary_fragments`. In that case though, we need to ensure we don't include the now-unused - // fragment in the final list of fragments. - // TODO: remark that the same reasoning could leave a single instance of a fragment - // usage, so if we really really want to never have less than `minUsagesToOptimize`, we - // could do some loop of `expand then flatten` unless all fragments are provably used - // enough. We don't bother, because leaving this is not a huge deal and it's not worth - // the complexity, but it could be that we can refactor all this later to avoid this - // case without additional complexity. - - // Prepare the next iteration - last_size = self.len(); - last_selection_set = new_selection_set; - } - Ok(last_selection_set) - } - - /// The inner loop body of `reduce` method. - fn reduce_inner( - &mut self, - selection_set: &SelectionSet, - min_usage_to_optimize: u32, - ) -> Result { - let mut usages = selection_set.used_fragments(); - - // Short-circuiting: Nothing was used => Drop everything (selection_set is unchanged). - if usages.is_empty() { - *self = Default::default(); - 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. - // PORT_NOTE: The `computeFragmentsToKeep` function is implemented here. - let original_size = self.len(); - 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.len() == 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)? - .flatten_unnecessary_fragments( - &fragment.selection_set.type_position, - &fragments_to_keep, - &fragment.schema, - )?; - } - - // Compute the new selection set based on the new reduced set of fragments. - // Note that optimizing all fragments to potentially re-expand some is not entirely - // optimal, but it's unclear how to do otherwise, and it probably don't matter too much in - // practice (we only call this optimization on the final computed query plan, so not a very - // hot path; plus in most cases we won't even reach that point either because there is no - // fragment, or none will have been optimized away so we'll exit above). - let reduced_selection_set = selection_set.retain_fragments(self)?; + *selection_set = new_selection_set; - // Expanding fragments could create some "inefficiencies" that we wouldn't have if we - // hadn't re-optimized the fragments to de-optimize it later, so we do a final "flatten" - // pass to remove those. - reduced_selection_set.flatten_unnecessary_fragments( - &reduced_selection_set.type_position, - self, - &selection_set.schema, - ) + Ok(()) } - fn update_usages( - usages: &mut IndexMap, - fragment: &Node, - usage_count: u32, - ) { - let mut inner_usages = IndexMap::default(); - 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; - } + /// Consumes the generator and returns the fragments it generated. + fn into_inner(self) -> NamedFragments { + self.fragments } } //============================================================================= -// `reuse_fragments` methods (putting everything together) - -impl Selection { - fn reuse_fragments_inner( - &self, - context: &ReuseContext<'_>, - validator: &mut FieldsConflictMultiBranchValidator, - fragments_at_type: &mut FragmentRestrictionAtTypeCache, - ) -> Result { - match self { - Selection::Field(field) => Ok(field - .reuse_fragments_inner(context, validator, fragments_at_type)? - .into()), - Selection::FragmentSpread(_) => Ok(self.clone()), // Do nothing - Selection::InlineFragment(inline_fragment) => Ok(inline_fragment - .reuse_fragments_inner(context, validator, fragments_at_type)? - .into()), - } - } -} - -impl FieldSelection { - fn reuse_fragments_inner( - &self, - context: &ReuseContext<'_>, - validator: &mut FieldsConflictMultiBranchValidator, - fragments_at_type: &mut FragmentRestrictionAtTypeCache, - ) -> Result { - let Some(base_composite_type): Option = - self.field.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_apply_fragments( - &base_composite_type, - context, - &mut field_validator, - fragments_at_type, - FullMatchingFragmentCondition::ForFieldSelection, - )?; - - let mut optimized = match opt { - SelectionSetOrFragment::Fragment(fragment) => { - let fragment_selection = FragmentSpreadSelection::from_fragment( - &fragment, - /*directives*/ &Default::default(), - ); - SelectionSet::from_selection(base_composite_type, fragment_selection.into()) - } - SelectionSetOrFragment::SelectionSet(selection_set) => selection_set, - }; - optimized = - optimized.reuse_fragments_inner(context, &mut field_validator, fragments_at_type)?; - Ok(self.with_updated_selection_set(Some(optimized))) - } -} - -/// Return type for `InlineFragmentSelection::reuse_fragments`. -#[derive(derive_more::From)] -enum FragmentSelection { - // Note: Enum variants are named to match those of `Selection`. - InlineFragment(InlineFragmentSelection), - FragmentSpread(FragmentSpreadSelection), -} - -impl From for Selection { - fn from(value: FragmentSelection) -> Self { - match value { - FragmentSelection::InlineFragment(inline_fragment) => inline_fragment.into(), - FragmentSelection::FragmentSpread(fragment_spread) => fragment_spread.into(), - } - } -} - -impl InlineFragmentSelection { - fn reuse_fragments_inner( - &self, - context: &ReuseContext<'_>, - validator: &mut FieldsConflictMultiBranchValidator, - fragments_at_type: &mut FragmentRestrictionAtTypeCache, - ) -> Result { - let optimized; +// Tests - let type_condition_position = &self.inline_fragment.type_condition_position; - if let Some(type_condition_position) = type_condition_position { - let opt = self.selection_set.try_apply_fragments( - type_condition_position, - context, - validator, - fragments_at_type, - FullMatchingFragmentCondition::ForInlineFragmentSelection { - type_condition_position, - directives: &self.inline_fragment.directives, - }, - )?; +#[cfg(test)] +mod tests { + use super::*; + use crate::operation::tests::*; - 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`). + /// Returns a consistent GraphQL name for the given index. + fn fragment_name(mut index: usize) -> Name { + /// https://spec.graphql.org/draft/#NameContinue + const NAME_CHARS: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_"; + /// https://spec.graphql.org/draft/#NameStart + const NAME_START_CHARS: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_"; - // 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 `FragmentSpread`). - let directives: executable::DirectiveList = self - .inline_fragment - .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(), - ); - } - } - SelectionSetOrFragment::SelectionSet(selection_set) => { - optimized = selection_set; - } - } + if index < NAME_START_CHARS.len() { + Name::new_static_unchecked(&NAME_START_CHARS[index..index + 1]) } else { - optimized = self.selection_set.clone(); - } - - Ok(InlineFragmentSelection::new( - self.inline_fragment.clone(), - // Then, recurse inside the field sub-selection (note that if we matched some fragments - // above, this recursion will "ignore" those as `FragmentSpreadSelection`'s - // `reuse_fragments()` is a no-op). - optimized.reuse_fragments_inner(context, validator, fragments_at_type)?, - ) - .into()) - } -} - -impl SelectionSet { - fn reuse_fragments_inner( - &self, - context: &ReuseContext<'_>, - validator: &mut FieldsConflictMultiBranchValidator, - fragments_at_type: &mut FragmentRestrictionAtTypeCache, - ) -> Result { - self.lazy_map(context.fragments, |selection| { - Ok(selection - .reuse_fragments_inner(context, validator, fragments_at_type)? - .into()) - }) - } - - fn contains_fragment_spread(&self) -> bool { - self.iter().any(|selection| { - matches!(selection, Selection::FragmentSpread(_)) - || selection - .selection_set() - .map(|subselection| subselection.contains_fragment_spread()) - .unwrap_or(false) - }) - } - - /// ## Errors - /// Returns an error if the selection set contains a named fragment spread. - fn reuse_fragments(&mut self, context: &ReuseContext<'_>) -> Result<(), FederationError> { - if context.fragments.is_empty() { - return Ok(()); - } - - if self.contains_fragment_spread() { - return Err(FederationError::internal("reuse_fragments() must only be used on selection sets that do not contain named fragment spreads")); - } + let mut s = String::new(); - // Calling reuse_fragments() 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.reuse_fragments(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, `reuse_fragments` 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.reuse_fragments_inner( - context, - &mut validator, - &mut FragmentRestrictionAtTypeCache::default(), - )?; + let i = index % NAME_START_CHARS.len(); + s.push(NAME_START_CHARS.as_bytes()[i].into()); + index /= NAME_START_CHARS.len(); - // 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. - *self = match optimized { - FragmentSelection::FragmentSpread(spread) => { - SelectionSet::from_selection(self.type_position.clone(), spread.into()) + while index > 0 { + let i = index % NAME_CHARS.len(); + s.push(NAME_CHARS.as_bytes()[i].into()); + index /= NAME_CHARS.len(); } - FragmentSelection::InlineFragment(inline_fragment) => inline_fragment.selection_set, - }; - Ok(()) - } -} - -impl Operation { - // PORT_NOTE: The JS version of `reuse_fragments` 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; - // `fragments` - rebased fragment definitions for the operation's subgraph - // - `self.selection_set` must be fragment-spread-free. - fn reuse_fragments_inner( - &mut self, - fragments: &NamedFragments, - min_usages_to_optimize: u32, - ) -> 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 - .reuse_fragments(&ReuseContext::for_operation(fragments, &self.variables))?; - 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, min_usages_to_optimize)?; - - self.selection_set = final_selection_set; - self.named_fragments = final_fragments; - Ok(()) - } - - /// Optimize the parsed size of the operation by applying fragment spreads. Fragment spreads - /// are reused from the original user-provided fragments. - /// - /// `fragments` - rebased fragment definitions for the operation's subgraph - /// - // PORT_NOTE: In JS, this function was called "optimize". - pub(crate) fn reuse_fragments( - &mut self, - fragments: &NamedFragments, - ) -> Result<(), FederationError> { - self.reuse_fragments_inner(fragments, Self::DEFAULT_MIN_USAGES_TO_OPTIMIZE) - } - - /// Optimize the parsed size of the operation by generating fragments based on the selections - /// in the operation. - pub(crate) fn generate_fragments(&mut self) -> Result<(), FederationError> { - // Currently, this method simply pulls out every inline fragment into a named fragment. If - // multiple inline fragments are the same, they use the same named fragment. - // - // This method can generate named fragments that are only used once. It's not ideal, but it - // also doesn't seem that bad. Avoiding this is possible but more work, and keeping this - // as simple as possible is a big benefit for now. - // - // When we have more advanced correctness testing, we can add more features to fragment - // generation, like factoring out partial repeated slices of selection sets or only - // introducing named fragments for patterns that occur more than once. - let mut generator = FragmentGenerator::default(); - generator.visit_selection_set(&mut self.selection_set)?; - self.named_fragments = generator.into_inner(); - Ok(()) - } - - /// Used by legacy roundtrip tests. - /// - This lowers `min_usages_to_optimize` to `1` in order to make it easier to write unit tests. - #[cfg(test)] - fn reuse_fragments_for_roundtrip_test( - &mut self, - fragments: &NamedFragments, - ) -> Result<(), FederationError> { - self.reuse_fragments_inner(fragments, /*min_usages_to_optimize*/ 1) - } - - // PORT_NOTE: This mirrors the JS version's `Operation.expandAllFragments`. But this method is - // mainly for unit tests. The actual port of `expandAllFragments` is in `normalize_operation`. - #[cfg(test)] - fn expand_all_fragments_and_normalize(&self) -> Result { - let selection_set = self - .selection_set - .expand_all_fragments()? - .flatten_unnecessary_fragments( - &self.selection_set.type_position, - &self.named_fragments, - &self.schema, - )?; - Ok(Self { - named_fragments: Default::default(), - selection_set, - ..self.clone() - }) - } -} - -#[derive(Debug, Default)] -struct FragmentGenerator { - fragments: NamedFragments, - // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! - names: IndexMap<(String, usize), usize>, -} - -impl FragmentGenerator { - // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! - // In the future, we will just use `.next_name()`. - fn generate_name(&mut self, frag: &InlineFragmentSelection) -> Name { - use std::fmt::Write as _; - - let type_condition = frag - .inline_fragment - .type_condition_position - .as_ref() - .map_or_else( - || "undefined".to_string(), - |condition| condition.to_string(), - ); - let selections = frag.selection_set.selections.len(); - let mut name = format!("_generated_on{type_condition}{selections}"); - - let key = (type_condition, selections); - let index = self - .names - .entry(key) - .and_modify(|index| *index += 1) - .or_default(); - _ = write!(&mut name, "_{index}"); - - Name::new_unchecked(&name) - } - - /// Is a selection set worth using for a newly generated named fragment? - fn is_worth_using(selection_set: &SelectionSet) -> bool { - let mut iter = selection_set.iter(); - let Some(first) = iter.next() else { - // An empty selection is not worth using (and invalid!) - return false; - }; - let Selection::Field(field) = first else { - return true; - }; - // If there's more than one selection, or one selection with a subselection, - // it's probably worth using - iter.next().is_some() || field.selection_set.is_some() - } - - /// Modify the selection set so that eligible inline fragments are moved to named fragment spreads. - fn visit_selection_set( - &mut self, - selection_set: &mut SelectionSet, - ) -> Result<(), FederationError> { - let mut new_selection_set = SelectionSet::empty( - selection_set.schema.clone(), - selection_set.type_position.clone(), - ); - - for selection in Arc::make_mut(&mut selection_set.selections).values_mut() { - match selection { - SelectionValue::Field(mut field) => { - if let Some(selection_set) = field.get_selection_set_mut() { - self.visit_selection_set(selection_set)?; - } - new_selection_set - .add_local_selection(&Selection::Field(Arc::clone(field.get())))?; - } - SelectionValue::FragmentSpread(frag) => { - new_selection_set - .add_local_selection(&Selection::FragmentSpread(Arc::clone(frag.get())))?; - } - SelectionValue::InlineFragment(frag) - if !Self::is_worth_using(&frag.get().selection_set) => - { - new_selection_set - .add_local_selection(&Selection::InlineFragment(Arc::clone(frag.get())))?; - } - SelectionValue::InlineFragment(mut candidate) => { - self.visit_selection_set(candidate.get_selection_set_mut())?; - - // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! - // JS federation does not consider fragments without a type condition. - if candidate - .get() - .inline_fragment - .type_condition_position - .is_none() - { - new_selection_set.add_local_selection(&Selection::InlineFragment( - Arc::clone(candidate.get()), - ))?; - continue; - } - - let directives = &candidate.get().inline_fragment.directives; - let skip_include = directives - .iter() - .map(|directive| match directive.name.as_str() { - "skip" | "include" => Ok(directive.clone()), - _ => Err(()), - }) - .collect::>(); - - // If there are any directives *other* than @skip and @include, - // we can't just transfer them to the generated fragment spread, - // so we have to keep this inline fragment. - let Ok(skip_include) = skip_include else { - new_selection_set.add_local_selection(&Selection::InlineFragment( - Arc::clone(candidate.get()), - ))?; - continue; - }; - - // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! - // JS does not special-case @skip and @include. It never extracts a fragment if - // there's any directives on it. This code duplicates the body from the - // previous condition so it's very easy to remove when we're ready :) - if !skip_include.is_empty() { - new_selection_set.add_local_selection(&Selection::InlineFragment( - Arc::clone(candidate.get()), - ))?; - continue; - } - - let existing = self.fragments.iter().find(|existing| { - existing.type_condition_position - == candidate.get().inline_fragment.casted_type() - && existing.selection_set == candidate.get().selection_set - }); - - let existing = if let Some(existing) = existing { - existing - } else { - // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! - // This should be reverted to `self.next_name();` when we're ready. - let name = self.generate_name(candidate.get()); - self.fragments.insert(Fragment { - schema: selection_set.schema.clone(), - name: name.clone(), - type_condition_position: candidate.get().inline_fragment.casted_type(), - directives: Default::default(), - selection_set: candidate.get().selection_set.clone(), - }); - self.fragments.get(&name).unwrap() - }; - new_selection_set.add_local_selection(&Selection::from( - FragmentSpreadSelection { - spread: FragmentSpread { - schema: selection_set.schema.clone(), - fragment_name: existing.name.clone(), - type_condition_position: existing.type_condition_position.clone(), - directives: skip_include.into(), - fragment_directives: existing.directives.clone(), - selection_id: crate::operation::SelectionId::new(), - }, - selection_set: existing.selection_set.clone(), - }, - ))?; - } - } - } - - *selection_set = new_selection_set; - - Ok(()) - } - - /// Consumes the generator and returns the fragments it generated. - fn into_inner(self) -> NamedFragments { - self.fragments - } -} - -//============================================================================= -// Tests - -#[cfg(test)] -mod tests { - use apollo_compiler::ExecutableDocument; - - use super::*; - use crate::operation::tests::*; - - macro_rules! assert_without_fragments { - ($operation: expr, @$expected: literal) => {{ - let without_fragments = $operation.expand_all_fragments_and_normalize().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.reuse_fragments(&$named_fragments).unwrap(); - validate_operation(&$operation.schema, &optimized.to_string()); - insta::assert_snapshot!(optimized, @$expected) - }}; - } - - /// Returns a consistent GraphQL name for the given index. - fn fragment_name(mut index: usize) -> Name { - /// https://spec.graphql.org/draft/#NameContinue - const NAME_CHARS: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_"; - /// https://spec.graphql.org/draft/#NameStart - const NAME_START_CHARS: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_"; - - if index < NAME_START_CHARS.len() { - Name::new_static_unchecked(&NAME_START_CHARS[index..index + 1]) - } else { - let mut s = String::new(); - - let i = index % NAME_START_CHARS.len(); - s.push(NAME_START_CHARS.as_bytes()[i].into()); - index /= NAME_START_CHARS.len(); - - while index > 0 { - let i = index % NAME_CHARS.len(); - s.push(NAME_CHARS.as_bytes()[i].into()); - index /= NAME_CHARS.len(); - } - - Name::new_unchecked(&s) + Name::new_unchecked(&s) } } @@ -1832,1619 +389,6 @@ mod tests { assert_eq!(fragment_name(usize::MAX), "oS5Uz8g3Iqw"); } - #[test] - fn duplicate_fragment_spreads_after_fragment_expansion() { - // This is a regression test for FED-290, making sure `make_select` method can handle - // duplicate fragment spreads. - // During optimization, `make_selection` may merge multiple fragment spreads with the same - // key. This can happen in the case below where `F1` and `F2` are expanded and generating - // two duplicate `F_shared` spreads in the definition of `fragment F_target`. - let schema_doc = r#" - type Query { - t: T - t2: T - } - - type T { - id: ID! - a: Int! - b: Int! - c: Int! - } - "#; - - let query = r#" - fragment F_shared on T { - id - a - } - fragment F1 on T { - ...F_shared - b - } - - fragment F2 on T { - ...F_shared - c - } - - fragment F_target on T { - ...F1 - ...F2 - } - - query { - t { - ...F_target - } - t2 { - ...F_target - } - } - "#; - - let operation = parse_operation(&parse_schema(schema_doc), query); - let expanded = operation.expand_all_fragments_and_normalize().unwrap(); - assert_optimized!(expanded, operation.named_fragments, @r###" - fragment F_target on T { - id - a - b - c - } - - { - t { - ...F_target - } - t2 { - ...F_target - } - } - "###); - } - - #[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] - 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_and_normalize().unwrap(); - insta::assert_snapshot!(without_fragments, @$expanded); - - let mut optimized = without_fragments; - optimized.reuse_fragments(&operation.named_fragments).unwrap(); - validate_operation(&operation.schema, &optimized.to_string()); - assert_eq!(optimized.to_string(), operation.to_string()); - }}; - } - - /// Tests ported from JS codebase rely on special behavior of - /// `Operation::reuse_fragments_for_roundtrip_test` that is specific for testing, since it makes it - /// easier to write tests. - macro_rules! test_fragments_roundtrip_legacy { - ($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_and_normalize().unwrap(); - insta::assert_snapshot!(without_fragments, @$expanded); - - let mut optimized = without_fragments; - optimized.reuse_fragments_for_roundtrip_test(&operation.named_fragments).unwrap(); - validate_operation(&operation.schema, &optimized.to_string()); - 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 - } - } - } - "###); - } - - #[test] - fn handles_nested_fragments_with_field_intersection() { - let schema_doc = r#" - type Query { - t: T - } - - type T { - a: A - b: Int - } - - type A { - x: String - y: String - z: String - } - "#; - - // The subtlety here is that `FA` contains `__typename` and so after we're reused it, the - // selection will look like: - // { - // t { - // a { - // ...FA - // } - // } - // } - // But to recognize that `FT` can be reused from there, we need to be able to see that - // the `__typename` that `FT` wants is inside `FA` (and since FA applies on the parent type `A` - // directly, it is fine to reuse). - let query = r#" - fragment FA on A { - __typename - x - y - } - - fragment FT on T { - a { - __typename - ...FA - } - } - - query { - t { - ...FT - } - } - "#; - - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - { - t { - a { - __typename - x - y - } - } - } - "###); - } - - #[test] - fn handles_fragment_matching_subset_of_field_selection() { - let schema_doc = r#" - type Query { - t: T - } - - type T { - a: String - b: B - c: Int - d: D - } - - type B { - x: String - y: String - } - - type D { - m: String - n: String - } - "#; - - let query = r#" - fragment FragT on T { - b { - __typename - x - } - c - d { - m - } - } - - { - t { - ...FragT - d { - n - } - a - } - } - "#; - - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - { - t { - b { - __typename - x - } - c - d { - m - n - } - a - } - } - "###); - } - - #[test] - fn handles_fragment_matching_subset_of_inline_fragment_selection() { - // Pretty much the same test than the previous one, but matching inside a fragment selection inside - // of inside a field selection. - // PORT_NOTE: ` implements I` was added in the definition of `type T`, so that validation can pass. - let schema_doc = r#" - type Query { - i: I - } - - interface I { - a: String - } - - type T implements I { - a: String - b: B - c: Int - d: D - } - - type B { - x: String - y: String - } - - type D { - m: String - n: String - } - "#; - - let query = r#" - fragment FragT on T { - b { - __typename - x - } - c - d { - m - } - } - - { - i { - ... on T { - ...FragT - d { - n - } - a - } - } - } - "#; - - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - { - i { - ... on T { - b { - __typename - x - } - c - d { - m - n - } - a - } - } - } - "###); - } - - #[test] - fn intersecting_fragments() { - let schema_doc = r#" - type Query { - t: T - } - - type T { - a: String - b: B - c: Int - d: D - } - - type B { - x: String - y: String - } - - type D { - m: String - n: String - } - "#; - - // Note: the code that reuse fragments iterates on fragments in the order they are defined - // in the document, but when it reuse a fragment, it puts it at the beginning of the - // selection (somewhat random, it just feel often easier to read), so the net effect on - // this example is that `Frag2`, which will be reused after `Frag1` will appear first in - // the re-optimized selection. So we put it first in the input too so that input and output - // actually match (the `testFragmentsRoundtrip` compares strings, so it is sensible to - // ordering; we could theoretically use `Operation.equals` instead of string equality, - // which wouldn't really on ordering, but `Operation.equals` is not entirely trivial and - // comparing strings make problem a bit more obvious). - let query = r#" - fragment Frag1 on T { - b { - x - } - c - d { - m - } - } - - fragment Frag2 on T { - a - b { - __typename - x - } - d { - m - n - } - } - - { - t { - ...Frag1 - ...Frag2 - } - } - "#; - - // PORT_NOTE: `__typename` and `x`'s placements are switched in Rust. - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - { - t { - b { - __typename - x - } - c - d { - m - n - } - a - } - } - "###); - } - - #[test] - fn fragments_application_makes_type_condition_trivial() { - let schema_doc = r#" - type Query { - t: T - } - - interface I { - x: String - } - - type T implements I { - x: String - a: String - } - "#; - - let query = r#" - fragment FragI on I { - x - ... on T { - a - } - } - - { - t { - ...FragI - } - } - "#; - - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - { - t { - x - a - } - } - "###); - } - - #[test] - fn handles_fragment_matching_at_the_top_level_of_another_fragment() { - let schema_doc = r#" - type Query { - t: T - } - - type T { - a: String - u: U - } - - type U { - x: String - y: String - } - "#; - - let query = r#" - fragment Frag1 on T { - a - } - - fragment Frag2 on T { - u { - x - y - } - ...Frag1 - } - - fragment Frag3 on Query { - t { - ...Frag2 - } - } - - { - ...Frag3 - } - "#; - - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - { - t { - u { - x - y - } - a - } - } - "###); - } - - #[test] - fn handles_fragments_used_in_context_where_they_get_trimmed() { - let schema_doc = r#" - type Query { - t1: T1 - } - - interface I { - x: Int - } - - type T1 implements I { - x: Int - y: Int - } - - type T2 implements I { - x: Int - z: Int - } - "#; - - let query = r#" - fragment FragOnI on I { - ... on T1 { - y - } - ... on T2 { - z - } - } - - { - t1 { - ...FragOnI - } - } - "#; - - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - { - t1 { - y - } - } - "###); - } - - #[test] - fn handles_fragments_used_in_the_context_of_non_intersecting_abstract_types() { - let schema_doc = r#" - type Query { - i2: I2 - } - - interface I1 { - x: Int - } - - interface I2 { - y: Int - } - - interface I3 { - z: Int - } - - type T1 implements I1 & I2 { - x: Int - y: Int - } - - type T2 implements I1 & I3 { - x: Int - z: Int - } - "#; - - let query = r#" - fragment FragOnI1 on I1 { - ... on I2 { - y - } - ... on I3 { - z - } - } - - { - i2 { - ...FragOnI1 - } - } - "#; - - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - { - i2 { - ... on I1 { - ... on I2 { - y - } - ... on I3 { - z - } - } - } - } - "###); - } - - #[test] - fn handles_fragments_on_union_in_context_with_limited_intersection() { - let schema_doc = r#" - type Query { - t1: T1 - } - - union U = T1 | T2 - - type T1 { - x: Int - } - - type T2 { - y: Int - } - "#; - - let query = r#" - fragment OnU on U { - ... on T1 { - x - } - ... on T2 { - y - } - } - - { - t1 { - ...OnU - } - } - "#; - - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - { - t1 { - x - } - } - "###); - } - - #[test] - fn off_by_1_error() { - let schema = r#" - type Query { - t: T - } - type T { - id: String! - a: A - v: V - } - type A { - id: String! - } - type V { - t: T! - } - "#; - - let query = r#" - { - t { - ...TFrag - v { - t { - id - a { - __typename - id - } - } - } - } - } - - fragment TFrag on T { - __typename - id - } - "#; - - let operation = parse_operation(&parse_schema(schema), query); - - let expanded = assert_without_fragments!( - operation, - @r###" - { - t { - __typename - id - v { - t { - id - a { - __typename - id - } - } - } - } - } - "### - ); - - assert_optimized!(expanded, operation.named_fragments, @r###" - fragment TFrag on T { - __typename - id - } - - { - t { - ...TFrag - v { - t { - ...TFrag - a { - __typename - id - } - } - } - } - } - "###); - } - - #[test] - fn removes_all_unused_fragments() { - let schema = r#" - type Query { - t1: T1 - } - - union U1 = T1 | T2 | T3 - union U2 = T2 | T3 - - type T1 { - x: Int - } - - type T2 { - y: Int - } - - type T3 { - z: Int - } - "#; - - let query = r#" - query { - t1 { - ...Outer - } - } - - fragment Outer on U1 { - ... on T1 { - x - } - ... on T2 { - ... Inner - } - ... on T3 { - ... Inner - } - } - - fragment Inner on U2 { - ... on T2 { - y - } - } - "#; - - let operation = parse_operation(&parse_schema(schema), query); - - let expanded = assert_without_fragments!( - operation, - @r###" - { - t1 { - x - } - } - "### - ); - - // This is a bit of contrived example, but the reusing code will be able - // to figure out that the `Outer` fragment can be reused and will initially - // do so, but it's only use once, so it will expand it, which yields: - // { - // t1 { - // ... on T1 { - // x - // } - // ... on T2 { - // ... Inner - // } - // ... on T3 { - // ... Inner - // } - // } - // } - // and so `Inner` will not be expanded (it's used twice). Except that - // the `flatten_unnecessary_fragments` code is apply then and will _remove_ both instances - // of `.... Inner`. Which is ok, but we must make sure the fragment - // itself is removed since it is not used now, which this test ensures. - assert_optimized!(expanded, operation.named_fragments, @r###" - { - t1 { - x - } - } - "###); - } - - #[test] - fn removes_fragments_only_used_by_unused_fragments() { - // Similar to the previous test, but we artificially add a - // fragment that is only used by the fragment that is finally - // unused. - let schema = r#" - type Query { - t1: T1 - } - - union U1 = T1 | T2 | T3 - union U2 = T2 | T3 - - type T1 { - x: Int - } - - type T2 { - y1: Y - y2: Y - } - - type T3 { - z: Int - } - - type Y { - v: Int - } - "#; - - let query = r#" - query { - t1 { - ...Outer - } - } - - fragment Outer on U1 { - ... on T1 { - x - } - ... on T2 { - ... Inner - } - ... on T3 { - ... Inner - } - } - - fragment Inner on U2 { - ... on T2 { - y1 { - ...WillBeUnused - } - y2 { - ...WillBeUnused - } - } - } - - fragment WillBeUnused on Y { - v - } - "#; - - let operation = parse_operation(&parse_schema(schema), query); - - let expanded = assert_without_fragments!( - operation, - @r###" - { - t1 { - x - } - } - "### - ); - - assert_optimized!(expanded, operation.named_fragments, @r###" - { - t1 { - x - } - } - "###); - } - - #[test] - fn keeps_fragments_used_by_other_fragments() { - let schema = r#" - type Query { - t1: T - t2: T - } - - type T { - a1: Int - a2: Int - b1: B - b2: B - } - - type B { - x: Int - y: Int - } - "#; - - let query = r#" - query { - t1 { - ...TFields - } - t2 { - ...TFields - } - } - - fragment TFields on T { - ...DirectFieldsOfT - b1 { - ...BFields - } - b2 { - ...BFields - } - } - - fragment DirectFieldsOfT on T { - a1 - a2 - } - - fragment BFields on B { - x - y - } - "#; - - let operation = parse_operation(&parse_schema(schema), query); - - let expanded = assert_without_fragments!( - operation, - @r###" - { - t1 { - a1 - a2 - b1 { - x - y - } - b2 { - x - y - } - } - t2 { - a1 - a2 - b1 { - x - y - } - b2 { - x - y - } - } - } - "### - ); - - // The `DirectFieldsOfT` fragments should not be kept as it is used only once within `TFields`, - // but the `BFields` one should be kept. - assert_optimized!(expanded, operation.named_fragments, @r###" - fragment BFields on B { - x - y - } - - fragment TFields on T { - a1 - a2 - b1 { - ...BFields - } - b2 { - ...BFields - } - } - - { - t1 { - ...TFields - } - t2 { - ...TFields - } - } - "###); - } - - /// - /// applied directives - /// - - #[test] - fn reuse_fragments_with_same_directive_in_the_fragment_selection() { - let schema_doc = r#" - type Query { - t1: T - t2: T - t3: T - } - - type T { - a: Int - b: Int - c: Int - d: Int - } - "#; - - let query = r#" - fragment DirectiveInDef on T { - a @include(if: $cond1) - } - - query myQuery($cond1: Boolean!, $cond2: Boolean!) { - t1 { - a - } - t2 { - ...DirectiveInDef - } - t3 { - a @include(if: $cond2) - } - } - "#; - - test_fragments_roundtrip_legacy!(schema_doc, query, @r###" - query myQuery($cond1: Boolean!, $cond2: Boolean!) { - t1 { - a - } - t2 { - a @include(if: $cond1) - } - t3 { - a @include(if: $cond2) - } - } - "###); - } - - #[test] - fn reuse_fragments_with_directives_on_inline_fragments() { - let schema_doc = r#" - type Query { - t1: T - t2: T - t3: T - } - - type T { - a: Int - b: Int - c: Int - d: Int - } - "#; - - let query = r#" - fragment NoDirectiveDef on T { - a - } - - query myQuery($cond1: Boolean!) { - t1 { - ...NoDirectiveDef - } - t2 { - ...NoDirectiveDef @include(if: $cond1) - } - } - "#; - - test_fragments_roundtrip!(schema_doc, query, @r###" - query myQuery($cond1: Boolean!) { - t1 { - a - } - t2 { - ... on T @include(if: $cond1) { - a - } - } - } - "###); - } - - #[test] - fn reuse_fragments_with_directive_on_typename() { - let schema = r#" - type Query { - t1: T - t2: T - t3: T - } - - type T { - a: Int - b: Int - c: Int - d: Int - } - "#; - let query = r#" - query A ($if: Boolean!) { - t1 { b a ...x } - t2 { ...x } - } - query B { - # Because this inline fragment is exactly the same shape as `x`, - # except for a `__typename` field, it may be tempting to reuse it. - # But `x.__typename` has a directive with a variable, and this query - # does not have that variable declared, so it can't be used. - t3 { ... on T { a c } } - } - fragment x on T { - __typename @include(if: $if) - a - c - } - "#; - let schema = parse_schema(schema); - let query = ExecutableDocument::parse_and_validate(schema.schema(), query, "query.graphql") - .unwrap(); - - let operation_a = - Operation::from_operation_document(schema.clone(), &query, Some("A")).unwrap(); - let operation_b = - Operation::from_operation_document(schema.clone(), &query, Some("B")).unwrap(); - let expanded_b = operation_b.expand_all_fragments_and_normalize().unwrap(); - - assert_optimized!(expanded_b, operation_a.named_fragments, @r###" - query B { - t3 { - a - c - } - } - "###); - } - - #[test] - fn reuse_fragments_with_non_intersecting_types() { - let schema = r#" - type Query { - t: T - s: S - s2: S - i: I - } - - interface I { - a: Int - b: Int - } - - type T implements I { - a: Int - b: Int - - c: Int - d: Int - } - type S implements I { - a: Int - b: Int - - f: Int - g: Int - } - "#; - let query = r#" - query A ($if: Boolean!) { - t { ...x } - s { ...x } - i { ...x } - } - query B { - s { - # this matches fragment x once it is flattened, - # because the `...on T` condition does not intersect with our - # current type `S` - __typename - a b - } - s2 { - # same snippet to get it to use the fragment - __typename - a b - } - } - fragment x on I { - __typename - a - b - ... on T { c d @include(if: $if) } - } - "#; - let schema = parse_schema(schema); - let query = ExecutableDocument::parse_and_validate(schema.schema(), query, "query.graphql") - .unwrap(); - - let operation_a = - Operation::from_operation_document(schema.clone(), &query, Some("A")).unwrap(); - let operation_b = - Operation::from_operation_document(schema.clone(), &query, Some("B")).unwrap(); - let expanded_b = operation_b.expand_all_fragments_and_normalize().unwrap(); - - assert_optimized!(expanded_b, operation_a.named_fragments, @r###" - query B { - s { - __typename - a - b - } - s2 { - __typename - a - b - } - } - "###); - } - /// /// empty branches removal /// diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 09b8799067..9243916327 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -9,7 +9,6 @@ use itertools::Itertools; use super::runtime_types_intersect; use super::Field; use super::FieldSelection; -use super::Fragment; use super::FragmentSpread; use super::FragmentSpreadSelection; use super::InlineFragment; @@ -46,45 +45,23 @@ fn print_possible_runtimes( ) } -/// Options for handling rebasing errors. -#[derive(Clone, Copy, Default)] -enum OnNonRebaseableSelection { - /// Drop the selection that can't be rebased and continue. - Drop, - /// Propagate the rebasing error. - #[default] - Error, -} - impl Selection { fn rebase_inner( &self, parent_type: &CompositeTypeDefinitionPosition, named_fragments: &NamedFragments, schema: &ValidFederationSchema, - on_non_rebaseable_selection: OnNonRebaseableSelection, ) -> Result { match self { Selection::Field(field) => field - .rebase_inner( - parent_type, - named_fragments, - schema, - on_non_rebaseable_selection, - ) + .rebase_inner(parent_type, named_fragments, schema) .map(|field| field.into()), - Selection::FragmentSpread(spread) => spread.rebase_inner( - parent_type, - named_fragments, - schema, - on_non_rebaseable_selection, - ), - Selection::InlineFragment(inline) => inline.rebase_inner( - parent_type, - named_fragments, - schema, - on_non_rebaseable_selection, - ), + Selection::FragmentSpread(spread) => { + spread.rebase_inner(parent_type, named_fragments, schema) + } + Selection::InlineFragment(inline) => { + inline.rebase_inner(parent_type, named_fragments, schema) + } } } @@ -94,7 +71,7 @@ impl Selection { named_fragments: &NamedFragments, schema: &ValidFederationSchema, ) -> Result { - self.rebase_inner(parent_type, named_fragments, schema, Default::default()) + self.rebase_inner(parent_type, named_fragments, schema) } fn can_add_to( @@ -146,18 +123,6 @@ pub(crate) enum RebaseError { }, } -impl FederationError { - fn is_rebase_error(&self) -> bool { - matches!( - self, - crate::error::FederationError::SingleFederationError { - inner: crate::error::SingleFederationError::InternalRebaseError(_), - .. - } - ) - } -} - impl From for FederationError { fn from(value: RebaseError) -> Self { crate::error::SingleFederationError::from(value).into() @@ -311,7 +276,6 @@ impl FieldSelection { parent_type: &CompositeTypeDefinitionPosition, named_fragments: &NamedFragments, schema: &ValidFederationSchema, - on_non_rebaseable_selection: OnNonRebaseableSelection, ) -> Result { if &self.field.schema == schema && &self.field.field_position.parent() == parent_type { // we are rebasing field on the same parent within the same schema - we can just return self @@ -344,12 +308,8 @@ impl FieldSelection { }); } - let rebased_selection_set = selection_set.rebase_inner( - &rebased_base_type, - named_fragments, - schema, - on_non_rebaseable_selection, - )?; + let rebased_selection_set = + selection_set.rebase_inner(&rebased_base_type, named_fragments, schema)?; if rebased_selection_set.selections.is_empty() { Err(RebaseError::EmptySelectionSet.into()) } else { @@ -433,7 +393,6 @@ impl FragmentSpreadSelection { parent_type: &CompositeTypeDefinitionPosition, named_fragments: &NamedFragments, schema: &ValidFederationSchema, - on_non_rebaseable_selection: OnNonRebaseableSelection, ) -> Result { // We preserve the parent type here, to make sure we don't lose context, but we actually don't // want to expand the spread as that would compromise the code that optimize subgraph fetches to re-use named @@ -481,12 +440,9 @@ impl FragmentSpreadSelection { // important because the very logic we're hitting here may need to happen inside the rebase on the // fragment selection, but that logic would not be triggered if we used the rebased `named_fragment` since // `rebase_on_same_schema` would then be 'true'. - let expanded_selection_set = self.selection_set.rebase_inner( - parent_type, - named_fragments, - schema, - on_non_rebaseable_selection, - )?; + let expanded_selection_set = + self.selection_set + .rebase_inner(parent_type, named_fragments, schema)?; // In theory, we could return the selection set directly, but making `SelectionSet.rebase_on` sometimes // return a `SelectionSet` complicate things quite a bit. So instead, we encapsulate the selection set // in an "empty" inline fragment. This make for non-really-optimal selection sets in the (relatively @@ -523,7 +479,7 @@ impl FragmentSpreadSelection { named_fragments: &NamedFragments, schema: &ValidFederationSchema, ) -> Result { - self.rebase_inner(parent_type, named_fragments, schema, Default::default()) + self.rebase_inner(parent_type, named_fragments, schema) } } @@ -619,7 +575,6 @@ impl InlineFragmentSelection { parent_type: &CompositeTypeDefinitionPosition, named_fragments: &NamedFragments, schema: &ValidFederationSchema, - on_non_rebaseable_selection: OnNonRebaseableSelection, ) -> Result { if &self.inline_fragment.schema == schema && self.inline_fragment.parent_type_position == *parent_type @@ -636,12 +591,9 @@ impl InlineFragmentSelection { // we are within the same schema - selection set does not have to be rebased Ok(InlineFragmentSelection::new(rebased_fragment, self.selection_set.clone()).into()) } else { - let rebased_selection_set = self.selection_set.rebase_inner( - &rebased_casted_type, - named_fragments, - schema, - on_non_rebaseable_selection, - )?; + let rebased_selection_set = + self.selection_set + .rebase_inner(&rebased_casted_type, named_fragments, schema)?; if rebased_selection_set.selections.is_empty() { // empty selection set Err(RebaseError::EmptySelectionSet.into()) @@ -710,24 +662,11 @@ impl SelectionSet { parent_type: &CompositeTypeDefinitionPosition, named_fragments: &NamedFragments, schema: &ValidFederationSchema, - on_non_rebaseable_selection: OnNonRebaseableSelection, ) -> Result { let rebased_results = self .selections .values() - .map(|selection| { - selection.rebase_inner( - parent_type, - named_fragments, - schema, - on_non_rebaseable_selection, - ) - }) - // Remove selections with rebase errors if requested - .filter(|result| { - matches!(on_non_rebaseable_selection, OnNonRebaseableSelection::Error) - || !result.as_ref().is_err_and(|err| err.is_rebase_error()) - }); + .map(|selection| selection.rebase_inner(parent_type, named_fragments, schema)); Ok(SelectionSet { schema: schema.clone(), @@ -747,7 +686,7 @@ impl SelectionSet { named_fragments: &NamedFragments, schema: &ValidFederationSchema, ) -> Result { - self.rebase_inner(parent_type, named_fragments, schema, Default::default()) + self.rebase_inner(parent_type, named_fragments, schema) } /// Returns true if the selection set would select cleanly from the given type in the given @@ -762,641 +701,3 @@ impl SelectionSet { .fallible_all(|selection| selection.can_add_to(parent_type, schema)) } } - -impl NamedFragments { - pub(crate) fn rebase_on( - &self, - schema: &ValidFederationSchema, - ) -> Result { - let mut rebased_fragments = NamedFragments::default(); - for fragment in self.fragments.values() { - if let Some(rebased_type) = schema - .get_type(fragment.type_condition_position.type_name().clone()) - .ok() - .and_then(|ty| CompositeTypeDefinitionPosition::try_from(ty).ok()) - { - if let Ok(mut rebased_selection) = fragment.selection_set.rebase_inner( - &rebased_type, - &rebased_fragments, - schema, - OnNonRebaseableSelection::Drop, - ) { - // Rebasing can leave some inefficiencies in some case (particularly when a spread has to be "expanded", see `FragmentSpreadSelection.rebaseOn`), - // so we do a top-level normalization to keep things clean. - rebased_selection = rebased_selection.flatten_unnecessary_fragments( - &rebased_type, - &rebased_fragments, - schema, - )?; - if NamedFragments::is_selection_set_worth_using(&rebased_selection) { - let fragment = Fragment { - schema: schema.clone(), - name: fragment.name.clone(), - type_condition_position: rebased_type.clone(), - directives: fragment.directives.clone(), - selection_set: rebased_selection, - }; - rebased_fragments.insert(fragment); - } - } - } - } - Ok(rebased_fragments) - } -} - -#[cfg(test)] -mod tests { - use apollo_compiler::collections::IndexSet; - use apollo_compiler::name; - - use crate::operation::normalize_operation; - use crate::operation::tests::parse_schema_and_operation; - use crate::operation::tests::parse_subgraph; - use crate::operation::NamedFragments; - use crate::schema::position::InterfaceTypeDefinitionPosition; - - #[test] - fn skips_unknown_fragment_fields() { - let operation_fragments = r#" -query TestQuery { - t { - ...FragOnT - } -} - -fragment FragOnT on T { - v0 - v1 - v2 - u1 { - v3 - v4 - v5 - } - u2 { - v4 - v5 - } -} - -type Query { - t: T -} - -type T { - v0: Int - v1: Int - v2: Int - u1: U - u2: U -} - -type U { - v3: Int - v4: Int - v5: Int -} -"#; - let (schema, mut executable_document) = parse_schema_and_operation(operation_fragments); - assert!( - !executable_document.fragments.is_empty(), - "operation should have some fragments" - ); - - if let Some(operation) = executable_document.operations.named.get_mut("TestQuery") { - let normalized_operation = normalize_operation( - operation, - NamedFragments::new(&executable_document.fragments, &schema), - &schema, - &IndexSet::default(), - ) - .unwrap(); - - let subgraph_schema = r#"type Query { - _: Int -} - -type T { - v1: Int - u1: U -} - -type U { - v3: Int - v5: Int -}"#; - let subgraph = parse_subgraph("A", subgraph_schema); - let rebased_fragments = normalized_operation.named_fragments.rebase_on(&subgraph); - assert!(rebased_fragments.is_ok()); - let rebased_fragments = rebased_fragments.unwrap(); - assert!(!rebased_fragments.is_empty()); - assert!(rebased_fragments.contains(&name!("FragOnT"))); - let rebased_fragment = rebased_fragments.fragments.get("FragOnT").unwrap(); - - insta::assert_snapshot!(rebased_fragment, @r###" - fragment FragOnT on T { - v1 - u1 { - v3 - v5 - } - } - "###); - } - } - - #[test] - fn skips_unknown_fragment_on_condition() { - let operation_fragments = r#" -query TestQuery { - t { - ...FragOnT - } - u { - ...FragOnU - } -} - -fragment FragOnT on T { - x - y -} - -fragment FragOnU on U { - x - y -} - -type Query { - t: T - u: U -} - -type T { - x: Int - y: Int -} - -type U { - x: Int - y: Int -} -"#; - let (schema, mut executable_document) = parse_schema_and_operation(operation_fragments); - assert!( - !executable_document.fragments.is_empty(), - "operation should have some fragments" - ); - assert_eq!(2, executable_document.fragments.len()); - - if let Some(operation) = executable_document.operations.named.get_mut("TestQuery") { - let normalized_operation = normalize_operation( - operation, - NamedFragments::new(&executable_document.fragments, &schema), - &schema, - &IndexSet::default(), - ) - .unwrap(); - - let subgraph_schema = r#"type Query { - t: T -} - -type T { - x: Int - y: Int -}"#; - let subgraph = parse_subgraph("A", subgraph_schema); - let rebased_fragments = normalized_operation.named_fragments.rebase_on(&subgraph); - assert!(rebased_fragments.is_ok()); - let rebased_fragments = rebased_fragments.unwrap(); - assert!(!rebased_fragments.is_empty()); - assert!(rebased_fragments.contains(&name!("FragOnT"))); - assert!(!rebased_fragments.contains(&name!("FragOnU"))); - let rebased_fragment = rebased_fragments.fragments.get("FragOnT").unwrap(); - - let expected = r#"fragment FragOnT on T { - x - y -}"#; - let actual = rebased_fragment.to_string(); - assert_eq!(actual, expected); - } - } - - #[test] - fn skips_unknown_type_within_fragment() { - let operation_fragments = r#" -query TestQuery { - i { - ...FragOnI - } -} - -fragment FragOnI on I { - id - otherId - ... on T1 { - x - } - ... on T2 { - y - } -} - -type Query { - i: I -} - -interface I { - id: ID! - otherId: ID! -} - -type T1 implements I { - id: ID! - otherId: ID! - x: Int -} - -type T2 implements I { - id: ID! - otherId: ID! - y: Int -} -"#; - let (schema, mut executable_document) = parse_schema_and_operation(operation_fragments); - assert!( - !executable_document.fragments.is_empty(), - "operation should have some fragments" - ); - - if let Some(operation) = executable_document.operations.named.get_mut("TestQuery") { - let normalized_operation = normalize_operation( - operation, - NamedFragments::new(&executable_document.fragments, &schema), - &schema, - &IndexSet::default(), - ) - .unwrap(); - - let subgraph_schema = r#"type Query { - i: I -} - -interface I { - id: ID! -} - -type T2 implements I { - id: ID! - y: Int -} -"#; - let subgraph = parse_subgraph("A", subgraph_schema); - let rebased_fragments = normalized_operation.named_fragments.rebase_on(&subgraph); - assert!(rebased_fragments.is_ok()); - let rebased_fragments = rebased_fragments.unwrap(); - assert!(!rebased_fragments.is_empty()); - assert!(rebased_fragments.contains(&name!("FragOnI"))); - let rebased_fragment = rebased_fragments.fragments.get("FragOnI").unwrap(); - - let expected = r#"fragment FragOnI on I { - id - ... on T2 { - y - } -}"#; - let actual = rebased_fragment.to_string(); - assert_eq!(actual, expected); - } - } - - #[test] - fn skips_typename_on_possible_interface_objects_within_fragment() { - let operation_fragments = r#" -query TestQuery { - i { - ...FragOnI - } -} - -fragment FragOnI on I { - __typename - id - x -} - -type Query { - i: I -} - -interface I { - id: ID! - x: String! -} - -type T implements I { - id: ID! - x: String! -} -"#; - - let (schema, mut executable_document) = parse_schema_and_operation(operation_fragments); - assert!( - !executable_document.fragments.is_empty(), - "operation should have some fragments" - ); - - if let Some(operation) = executable_document.operations.named.get_mut("TestQuery") { - let mut interface_objects: IndexSet = - IndexSet::default(); - interface_objects.insert(InterfaceTypeDefinitionPosition { - type_name: name!("I"), - }); - let normalized_operation = normalize_operation( - operation, - NamedFragments::new(&executable_document.fragments, &schema), - &schema, - &interface_objects, - ) - .unwrap(); - - let subgraph_schema = r#"extend schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/federation/v2.5", import: [{ name: "@interfaceObject" }, { name: "@key" }]) - -directive @link(url: String, as: String, import: [link__Import]) repeatable on SCHEMA - -directive @key(fields: federation__FieldSet!, resolvable: Boolean = true) repeatable on OBJECT | INTERFACE - -directive @interfaceObject on OBJECT - -type Query { - i: I -} - -type I @interfaceObject @key(fields: "id") { - id: ID! - x: String! -} - -scalar link__Import - -scalar federation__FieldSet -"#; - let subgraph = parse_subgraph("A", subgraph_schema); - let rebased_fragments = normalized_operation.named_fragments.rebase_on(&subgraph); - assert!(rebased_fragments.is_ok()); - let rebased_fragments = rebased_fragments.unwrap(); - assert!(!rebased_fragments.is_empty()); - assert!(rebased_fragments.contains(&name!("FragOnI"))); - let rebased_fragment = rebased_fragments.fragments.get("FragOnI").unwrap(); - - let expected = r#"fragment FragOnI on I { - id - x -}"#; - let actual = rebased_fragment.to_string(); - assert_eq!(actual, expected); - } - } - - #[test] - fn skips_fragments_with_trivial_selections() { - let operation_fragments = r#" -query TestQuery { - t { - ...F1 - ...F2 - ...F3 - } -} - -fragment F1 on T { - a - b -} - -fragment F2 on T { - __typename - a - b -} - -fragment F3 on T { - __typename - a - b - c - d -} - -type Query { - t: T -} - -type T { - a: Int - b: Int - c: Int - d: Int -} -"#; - let (schema, mut executable_document) = parse_schema_and_operation(operation_fragments); - assert!( - !executable_document.fragments.is_empty(), - "operation should have some fragments" - ); - - if let Some(operation) = executable_document.operations.named.get_mut("TestQuery") { - let normalized_operation = normalize_operation( - operation, - NamedFragments::new(&executable_document.fragments, &schema), - &schema, - &IndexSet::default(), - ) - .unwrap(); - - let subgraph_schema = r#"type Query { - t: T -} - -type T { - c: Int - d: Int -} -"#; - let subgraph = parse_subgraph("A", subgraph_schema); - let rebased_fragments = normalized_operation.named_fragments.rebase_on(&subgraph); - assert!(rebased_fragments.is_ok()); - let rebased_fragments = rebased_fragments.unwrap(); - // F1 reduces to nothing, and F2 reduces to just __typename so we shouldn't keep them. - assert_eq!(1, rebased_fragments.len()); - assert!(rebased_fragments.contains(&name!("F3"))); - let rebased_fragment = rebased_fragments.fragments.get("F3").unwrap(); - - let expected = r#"fragment F3 on T { - __typename - c - d -}"#; - let actual = rebased_fragment.to_string(); - assert_eq!(actual, expected); - } - } - - #[test] - fn handles_skipped_fragments_within_fragments() { - let operation_fragments = r#" -query TestQuery { - ...TheQuery -} - -fragment TheQuery on Query { - t { - x - ... GetU - } -} - -fragment GetU on T { - u { - y - z - } -} - -type Query { - t: T -} - -type T { - x: Int - u: U -} - -type U { - y: Int - z: Int -} -"#; - let (schema, mut executable_document) = parse_schema_and_operation(operation_fragments); - assert!( - !executable_document.fragments.is_empty(), - "operation should have some fragments" - ); - - if let Some(operation) = executable_document.operations.named.get_mut("TestQuery") { - let normalized_operation = normalize_operation( - operation, - NamedFragments::new(&executable_document.fragments, &schema), - &schema, - &IndexSet::default(), - ) - .unwrap(); - - let subgraph_schema = r#"type Query { - t: T -} - -type T { - x: Int -}"#; - let subgraph = parse_subgraph("A", subgraph_schema); - let rebased_fragments = normalized_operation.named_fragments.rebase_on(&subgraph); - assert!(rebased_fragments.is_ok()); - let rebased_fragments = rebased_fragments.unwrap(); - // F1 reduces to nothing, and F2 reduces to just __typename so we shouldn't keep them. - assert_eq!(1, rebased_fragments.len()); - assert!(rebased_fragments.contains(&name!("TheQuery"))); - let rebased_fragment = rebased_fragments.fragments.get("TheQuery").unwrap(); - - let expected = r#"fragment TheQuery on Query { - t { - x - } -}"#; - let actual = rebased_fragment.to_string(); - assert_eq!(actual, expected); - } - } - - #[test] - fn handles_subtypes_within_subgraphs() { - let operation_fragments = r#" -query TestQuery { - ...TQuery -} - -fragment TQuery on Query { - t { - x - y - ... on T { - z - } - } -} - -type Query { - t: I -} - -interface I { - x: Int - y: Int -} - -type T implements I { - x: Int - y: Int - z: Int -} -"#; - let (schema, mut executable_document) = parse_schema_and_operation(operation_fragments); - assert!( - !executable_document.fragments.is_empty(), - "operation should have some fragments" - ); - - if let Some(operation) = executable_document.operations.named.get_mut("TestQuery") { - let normalized_operation = normalize_operation( - operation, - NamedFragments::new(&executable_document.fragments, &schema), - &schema, - &IndexSet::default(), - ) - .unwrap(); - - let subgraph_schema = r#"type Query { - t: T -} - -type T { - x: Int - y: Int - z: Int -} -"#; - - let subgraph = parse_subgraph("A", subgraph_schema); - let rebased_fragments = normalized_operation.named_fragments.rebase_on(&subgraph); - assert!(rebased_fragments.is_ok()); - let rebased_fragments = rebased_fragments.unwrap(); - // F1 reduces to nothing, and F2 reduces to just __typename so we shouldn't keep them. - assert_eq!(1, rebased_fragments.len()); - assert!(rebased_fragments.contains(&name!("TQuery"))); - let rebased_fragment = rebased_fragments.fragments.get("TQuery").unwrap(); - - let expected = r#"fragment TQuery on Query { - t { - x - y - z - } -}"#; - let actual = rebased_fragment.to_string(); - assert_eq!(actual, expected); - } - } -} diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index b8fc736895..477e2548ef 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -248,11 +248,6 @@ impl SelectionMap { self.selections.is_empty() } - /// Returns the first selection in the map, or None if the map is empty. - pub(crate) fn first(&self) -> Option<&Selection> { - self.selections.first() - } - /// Computes the hash of a selection key. fn hash(&self, key: SelectionKey<'_>) -> u64 { self.hash_builder.hash_one(key) diff --git a/apollo-federation/src/operation/tests/mod.rs b/apollo-federation/src/operation/tests/mod.rs index 6988b8e659..b0329cbb3d 100644 --- a/apollo-federation/src/operation/tests/mod.rs +++ b/apollo-federation/src/operation/tests/mod.rs @@ -16,10 +16,18 @@ use crate::query_graph::graph_path::OpPathElement; use crate::schema::position::InterfaceTypeDefinitionPosition; use crate::schema::position::ObjectTypeDefinitionPosition; use crate::schema::ValidFederationSchema; -use crate::subgraph::Subgraph; mod defer; +macro_rules! assert_normalized { + ($schema_doc: expr, $query: expr, @$expected: literal) => {{ + let schema = parse_schema($schema_doc); + let without_fragments = parse_and_expand(&schema, $query).unwrap(); + insta::assert_snapshot!(without_fragments, @$expected); + without_fragments + }}; +} + pub(super) fn parse_schema_and_operation( schema_and_operation: &str, ) -> (ValidFederationSchema, ExecutableDocument) { @@ -30,12 +38,6 @@ pub(super) fn parse_schema_and_operation( (schema, executable_document) } -pub(super) fn parse_subgraph(name: &str, schema: &str) -> ValidFederationSchema { - let parsed_schema = - Subgraph::parse_and_expand(name, &format!("https://{name}"), schema).unwrap(); - ValidFederationSchema::new(parsed_schema.schema).unwrap() -} - pub(super) fn parse_schema(schema_doc: &str) -> ValidFederationSchema { let schema = Schema::parse_and_validate(schema_doc, "schema.graphql").unwrap(); ValidFederationSchema::new(schema).unwrap() @@ -65,17 +67,6 @@ pub(super) fn parse_and_expand( normalize_operation(operation, fragments, schema, &Default::default()) } -/// Parse and validate the query similarly to `parse_operation`, but does not construct the -/// `Operation` struct. -pub(super) fn validate_operation(schema: &ValidFederationSchema, query: &str) { - apollo_compiler::ExecutableDocument::parse_and_validate( - schema.schema(), - query, - "query.graphql", - ) - .unwrap(); -} - #[test] fn expands_named_fragments() { let operation_with_named_fragment = r#" @@ -1672,10 +1663,6 @@ fn directive_propagation() { ) .expect("directive applications to be valid"); insta::assert_snapshot!(query, @r###" - fragment DirectiveOnDef on T @fragDefOnly @fragAll { - a - } - { t2 { ... on T @fragInlineOnly @fragAll { @@ -1704,3 +1691,376 @@ fn directive_propagation() { .expect_err("directive @fragSpreadOnly to be rejected"); insta::assert_snapshot!(err, @"Unsupported custom directive @fragSpreadOnly on fragment spread. Due to query transformations during planning, the router requires directives on fragment spreads to support both the FRAGMENT_SPREAD and INLINE_FRAGMENT locations."); } + +#[test] +fn handles_fragment_matching_at_the_top_level_of_another_fragment() { + let schema_doc = r#" + type Query { + t: T + } + + type T { + a: String + u: U + } + + type U { + x: String + y: String + } + "#; + + let query = r#" + fragment Frag1 on T { + a + } + + fragment Frag2 on T { + u { + x + y + } + ...Frag1 + } + + fragment Frag3 on Query { + t { + ...Frag2 + } + } + + { + ...Frag3 + } + "#; + + assert_normalized!(schema_doc, query, @r###" + { + t { + u { + x + y + } + a + } + } + "###); +} + +#[test] +fn handles_fragments_used_in_context_where_they_get_trimmed() { + let schema_doc = r#" + type Query { + t1: T1 + } + + interface I { + x: Int + } + + type T1 implements I { + x: Int + y: Int + } + + type T2 implements I { + x: Int + z: Int + } + "#; + + let query = r#" + fragment FragOnI on I { + ... on T1 { + y + } + ... on T2 { + z + } + } + + { + t1 { + ...FragOnI + } + } + "#; + + assert_normalized!(schema_doc, query, @r###" + { + t1 { + y + } + } + "###); +} + +#[test] +fn handles_fragments_on_union_in_context_with_limited_intersection() { + let schema_doc = r#" + type Query { + t1: T1 + } + + union U = T1 | T2 + + type T1 { + x: Int + } + + type T2 { + y: Int + } + "#; + + let query = r#" + fragment OnU on U { + ... on T1 { + x + } + ... on T2 { + y + } + } + + { + t1 { + ...OnU + } + } + "#; + + assert_normalized!(schema_doc, query, @r###" + { + t1 { + x + } + } + "###); +} + +#[test] +fn off_by_1_error() { + let schema = r#" + type Query { + t: T + } + type T { + id: String! + a: A + v: V + } + type A { + id: String! + } + type V { + t: T! + } + "#; + + let query = r#" + { + t { + ...TFrag + v { + t { + id + a { + __typename + id + } + } + } + } + } + + fragment TFrag on T { + __typename + id + } + "#; + + assert_normalized!(schema, query,@r###" + { + t { + id + v { + t { + id + a { + id + } + } + } + } + } + "### + ); +} + +/// +/// applied directives +/// + +#[test] +fn reuse_fragments_with_same_directive_in_the_fragment_selection() { + let schema_doc = r#" + type Query { + t1: T + t2: T + t3: T + } + + type T { + a: Int + b: Int + c: Int + d: Int + } + "#; + + let query = r#" + fragment DirectiveInDef on T { + a @include(if: $cond1) + } + + query ($cond1: Boolean!, $cond2: Boolean!) { + t1 { + a + } + t2 { + ...DirectiveInDef + } + t3 { + a @include(if: $cond2) + } + } + "#; + + assert_normalized!(schema_doc, query, @r###" + query($cond1: Boolean!, $cond2: Boolean!) { + t1 { + a + } + t2 { + a @include(if: $cond1) + } + t3 { + a @include(if: $cond2) + } + } + "###); +} + +#[test] +fn reuse_fragments_with_directive_on_typename() { + let schema = r#" + type Query { + t1: T + t2: T + t3: T + } + + type T { + a: Int + b: Int + c: Int + d: Int + } + "#; + let query = r#" + query ($if: Boolean!) { + t1 { b a ...x } + t2 { ...x } + } + fragment x on T { + __typename @include(if: $if) + a + c + } + "#; + + assert_normalized!(schema, query, @r###" + query($if: Boolean!) { + t1 { + b + a + __typename @include(if: $if) + c + } + t2 { + __typename @include(if: $if) + a + c + } + } + "###); +} + +#[test] +fn reuse_fragments_with_non_intersecting_types() { + let schema = r#" + type Query { + t: T + s: S + s2: S + i: I + } + + interface I { + a: Int + b: Int + } + + type T implements I { + a: Int + b: Int + + c: Int + d: Int + } + type S implements I { + a: Int + b: Int + + f: Int + g: Int + } + "#; + let query = r#" + query ($if: Boolean!) { + t { ...x } + s { ...x } + i { ...x } + } + fragment x on I { + __typename + a + b + ... on T { c d @include(if: $if) } + } + "#; + + assert_normalized!(schema, query, @r###" + query($if: Boolean!) { + t { + a + b + c + d @include(if: $if) + } + s { + a + b + } + i { + a + b + ... on T { + c + d @include(if: $if) + } + } + } + "###); +} diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index b91699ecc4..70ddf4d0fa 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -2673,8 +2673,7 @@ impl FetchDependencyGraphNode { &operation_name, )? }; - let operation = - operation_compression.compress(&self.subgraph_name, subgraph_schema, operation)?; + let operation = operation_compression.compress(operation)?; let operation_document = operation.try_into().map_err(|err| match err { FederationError::SingleFederationError { inner: SingleFederationError::InvalidGraphQL { diagnostics }, diff --git a/apollo-federation/src/query_plan/query_planner.rs b/apollo-federation/src/query_plan/query_planner.rs index ed2a4b2589..0c94adde1d 100644 --- a/apollo-federation/src/query_plan/query_planner.rs +++ b/apollo-federation/src/query_plan/query_planner.rs @@ -53,16 +53,6 @@ use crate::Supergraph; #[derive(Debug, Clone, Hash, Serialize)] pub struct QueryPlannerConfig { - /// Whether the query planner should try to reuse the named fragments of the planned query in - /// subgraph fetches. - /// - /// Reusing fragments requires complicated validations, so it can take a long time on large - /// queries with many fragments. This option may be removed in the future in favour of - /// [`generate_query_fragments`][QueryPlannerConfig::generate_query_fragments]. - /// - /// Defaults to false. - pub reuse_query_fragments: bool, - /// If enabled, the query planner will extract inline fragments into fragment /// definitions before sending queries to subgraphs. This can significantly /// reduce the size of the query sent to subgraphs. @@ -104,7 +94,6 @@ pub struct QueryPlannerConfig { impl Default for QueryPlannerConfig { fn default() -> Self { Self { - reuse_query_fragments: false, generate_query_fragments: false, subgraph_graphql_validation: false, incremental_delivery: Default::default(), @@ -451,16 +440,6 @@ impl QueryPlanner { let operation_compression = if self.config.generate_query_fragments { SubgraphOperationCompression::GenerateFragments - } else if self.config.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. - SubgraphOperationCompression::ReuseFragments(RebasedFragments::new( - normalized_operation - .named_fragments - .add_typename_field_for_abstract_types_in_named_fragments()?, - )) } else { SubgraphOperationCompression::Disabled }; @@ -844,57 +823,15 @@ fn generate_condition_nodes<'a>( } } -/// Tracks fragments from the original operation, along with versions rebased on other subgraphs. -pub(crate) struct RebasedFragments { - original_fragments: NamedFragments, - /// Map key: subgraph name - rebased_fragments: IndexMap, NamedFragments>, -} - -impl RebasedFragments { - fn new(fragments: NamedFragments) -> Self { - Self { - original_fragments: fragments, - rebased_fragments: Default::default(), - } - } - - fn for_subgraph( - &mut self, - subgraph_name: impl Into>, - subgraph_schema: &ValidFederationSchema, - ) -> &NamedFragments { - self.rebased_fragments - .entry(subgraph_name.into()) - .or_insert_with(|| { - self.original_fragments - .rebase_on(subgraph_schema) - .unwrap_or_default() - }) - } -} - pub(crate) enum SubgraphOperationCompression { - ReuseFragments(RebasedFragments), GenerateFragments, Disabled, } impl SubgraphOperationCompression { /// Compress a subgraph operation. - pub(crate) fn compress( - &mut self, - subgraph_name: &Arc, - subgraph_schema: &ValidFederationSchema, - operation: Operation, - ) -> Result { + pub(crate) fn compress(&mut self, operation: Operation) -> Result { match self { - Self::ReuseFragments(fragments) => { - let rebased = fragments.for_subgraph(Arc::clone(subgraph_name), subgraph_schema); - let mut operation = operation; - operation.reuse_fragments(rebased)?; - Ok(operation) - } Self::GenerateFragments => { let mut operation = operation; operation.generate_fragments()?; @@ -1324,7 +1261,7 @@ type User } #[test] - fn test_optimize_basic() { + fn test_optimize_no_fragments_generated() { let supergraph = Supergraph::new(TEST_SUPERGRAPH).unwrap(); let api_schema = supergraph.to_api_schema(Default::default()).unwrap(); let document = ExecutableDocument::parse_and_validate( @@ -1350,7 +1287,7 @@ type User .unwrap(); let config = QueryPlannerConfig { - reuse_query_fragments: true, + generate_query_fragments: true, ..Default::default() }; let planner = QueryPlanner::new(&supergraph, config).unwrap(); @@ -1362,149 +1299,14 @@ type User 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 config = QueryPlannerConfig { - reuse_query_fragments: true, - ..Default::default() - }; - let planner = QueryPlanner::new(&supergraph, config).unwrap(); - let plan = planner - .build_query_plan(&document, None, Default::default()) - .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 + another_user: userById(id: 2) { name email - } - "#, - "operation.graphql", - ) - .unwrap(); - - let config = QueryPlannerConfig { - reuse_query_fragments: true, - ..Default::default() - }; - let planner = QueryPlanner::new(&supergraph, config).unwrap(); - let plan = planner - .build_query_plan(&document, None, Default::default()) - .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 c0d85c7b62..4a063565e1 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests.rs @@ -34,6 +34,7 @@ fn some_name() { mod context; mod debug_max_evaluated_plans_configuration; mod defer; +mod entities; mod fetch_operation_names; mod field_merging_with_skip_and_include; mod fragment_autogeneration; @@ -44,8 +45,7 @@ mod interface_type_explosion; mod introspection_typename_handling; mod merged_abstract_types_handling; mod mutations; -mod named_fragments; -mod named_fragments_preservation; +mod named_fragments_expansion; mod overrides; mod provides; mod requires; diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/entities.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/entities.rs new file mode 100644 index 0000000000..53f50aad38 --- /dev/null +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/entities.rs @@ -0,0 +1,142 @@ +// TODO this test shows inefficient QP where we make multiple parallel +// fetches of the same entity from the same subgraph but for different paths +#[test] +fn inefficient_entity_fetches_to_same_subgraph() { + let planner = planner!( + Subgraph1: r#" + type V @shareable { + x: Int + } + + interface I { + v: V + } + + type Outer implements I @key(fields: "id") { + id: ID! + v: V + } + "#, + Subgraph2: r#" + type Query { + outer1: Outer + outer2: Outer + } + + type V @shareable { + x: Int + } + + interface I { + v: V + w: Int + } + + type Inner implements I { + v: V + w: Int + } + + type Outer @key(fields: "id") { + id: ID! + inner: Inner + w: Int + } + "#, + ); + assert_plan!( + &planner, + r#" + query { + outer1 { + ...OuterFrag + } + outer2 { + ...OuterFrag + } + } + + fragment OuterFrag on Outer { + ...IFrag + inner { + ...IFrag + } + } + + fragment IFrag on I { + v { + x + } + w + } + "#, + @r#" + QueryPlan { + Sequence { + Fetch(service: "Subgraph2") { + { + outer1 { + __typename + id + w + inner { + v { + x + } + w + } + } + outer2 { + __typename + id + w + inner { + v { + x + } + w + } + } + } + }, + Parallel { + Flatten(path: "outer2") { + Fetch(service: "Subgraph1") { + { + ... on Outer { + __typename + id + } + } => + { + ... on Outer { + v { + x + } + } + } + }, + }, + Flatten(path: "outer1") { + Fetch(service: "Subgraph1") { + { + ... on Outer { + __typename + id + } + } => + { + ... on Outer { + v { + x + } + } + } + }, + }, + }, + }, + } + "# + ); +} diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/fragment_autogeneration.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/fragment_autogeneration.rs index 03b43c6245..e80f57953f 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/fragment_autogeneration.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/fragment_autogeneration.rs @@ -1,5 +1,12 @@ use apollo_federation::query_plan::query_planner::QueryPlannerConfig; +fn generate_fragments_config() -> QueryPlannerConfig { + QueryPlannerConfig { + generate_query_fragments: true, + ..Default::default() + } +} + const SUBGRAPH: &str = r#" directive @custom on INLINE_FRAGMENT | FRAGMENT_SPREAD @@ -25,7 +32,7 @@ const SUBGRAPH: &str = r#" #[test] fn it_respects_generate_query_fragments_option() { let planner = planner!( - config = QueryPlannerConfig { generate_query_fragments: true, reuse_query_fragments: false, ..Default::default() }, + config = generate_fragments_config(), Subgraph1: SUBGRAPH, ); assert_plan!( @@ -73,7 +80,7 @@ fn it_respects_generate_query_fragments_option() { #[test] fn it_handles_nested_fragment_generation() { let planner = planner!( - config = QueryPlannerConfig { generate_query_fragments: true, reuse_query_fragments: false, ..Default::default() }, + config = generate_fragments_config(), Subgraph1: SUBGRAPH, ); assert_plan!( @@ -131,10 +138,250 @@ fn it_handles_nested_fragment_generation() { ); } +// TODO this test shows a clearly worse plan than reused fragments when fragments +// target concrete types +#[test] +fn it_handles_nested_fragment_generation_from_operation_with_fragments() { + let planner = planner!( + config = generate_fragments_config(), + Subgraph1: r#" + type Query { + a: Anything + } + + union Anything = A1 | A2 | A3 + + interface Foo { + foo: String + child: Foo + child2: Foo + } + + type A1 implements Foo { + foo: String + child: Foo + child2: Foo + } + + type A2 implements Foo { + foo: String + child: Foo + child2: Foo + } + + type A3 implements Foo { + foo: String + child: Foo + child2: Foo + } + "#, + ); + assert_plan!( + &planner, + r#" + query { + a { + ... on A1 { + ...FooSelect + } + ... on A2 { + ...FooSelect + } + ... on A3 { + ...FooSelect + } + } + } + + fragment FooSelect on Foo { + __typename + foo + child { + ...FooChildSelect + } + child2 { + ...FooChildSelect + } + } + + fragment FooChildSelect on Foo { + __typename + foo + child { + child { + child { + foo + } + } + } + } + "#, + + // This is a test case that shows worse result + // QueryPlan { + // Fetch(service: "Subgraph1") { + // { + // a { + // __typename + // ... on A1 { + // ...FooSelect + // } + // ... on A2 { + // ...FooSelect + // } + // ... on A3 { + // ...FooSelect + // } + // } + // } + // + // fragment FooChildSelect on Foo { + // __typename + // foo + // child { + // __typename + // child { + // __typename + // child { + // __typename + // foo + // } + // } + // } + // } + // + // fragment FooSelect on Foo { + // __typename + // foo + // child { + // ...FooChildSelect + // } + // child2 { + // ...FooChildSelect + // } + // } + // }, + // } + @r###" + QueryPlan { + Fetch(service: "Subgraph1") { + { + a { + __typename + ..._generated_onA14_0 + ..._generated_onA24_0 + ..._generated_onA34_0 + } + } + + fragment _generated_onA14_0 on A1 { + __typename + foo + child { + __typename + foo + child { + __typename + child { + __typename + child { + __typename + foo + } + } + } + } + child2 { + __typename + foo + child { + __typename + child { + __typename + child { + __typename + foo + } + } + } + } + } + + fragment _generated_onA24_0 on A2 { + __typename + foo + child { + __typename + foo + child { + __typename + child { + __typename + child { + __typename + foo + } + } + } + } + child2 { + __typename + foo + child { + __typename + child { + __typename + child { + __typename + foo + } + } + } + } + } + + fragment _generated_onA34_0 on A3 { + __typename + foo + child { + __typename + foo + child { + __typename + child { + __typename + child { + __typename + foo + } + } + } + } + child2 { + __typename + foo + child { + __typename + child { + __typename + child { + __typename + foo + } + } + } + } + } + }, + } + "### + ); +} + #[test] fn it_handles_fragments_with_one_non_leaf_field() { let planner = planner!( - config = QueryPlannerConfig { generate_query_fragments: true, reuse_query_fragments: false, ..Default::default() }, + config = generate_fragments_config(), Subgraph1: SUBGRAPH, ); @@ -183,7 +430,7 @@ fn it_handles_fragments_with_one_non_leaf_field() { #[test] fn it_migrates_skip_include() { let planner = planner!( - config = QueryPlannerConfig { generate_query_fragments: true, reuse_query_fragments: false, ..Default::default() }, + config = generate_fragments_config(), Subgraph1: SUBGRAPH, ); assert_plan!( @@ -250,10 +497,11 @@ fn it_migrates_skip_include() { "### ); } + #[test] fn it_identifies_and_reuses_equivalent_fragments_that_arent_identical() { let planner = planner!( - config = QueryPlannerConfig { generate_query_fragments: true, reuse_query_fragments: false, ..Default::default() }, + config = generate_fragments_config(), Subgraph1: SUBGRAPH, ); assert_plan!( @@ -301,7 +549,7 @@ fn it_identifies_and_reuses_equivalent_fragments_that_arent_identical() { #[test] fn fragments_that_share_a_hash_but_are_not_identical_generate_their_own_fragment_definitions() { let planner = planner!( - config = QueryPlannerConfig { generate_query_fragments: true, reuse_query_fragments: false, ..Default::default() }, + config = generate_fragments_config(), Subgraph1: SUBGRAPH, ); assert_plan!( @@ -354,7 +602,7 @@ fn fragments_that_share_a_hash_but_are_not_identical_generate_their_own_fragment #[test] fn same_as_js_router798() { let planner = planner!( - config = QueryPlannerConfig { generate_query_fragments: true, reuse_query_fragments: false, ..Default::default() }, + config = generate_fragments_config(), Subgraph1: r#" interface Interface { a: Int } type Y implements Interface { a: Int b: Int } @@ -398,6 +646,7 @@ fn same_as_js_router798() { #[test] fn works_with_key_chains() { let planner = planner!( + config = generate_fragments_config(), Subgraph1: r#" type Query { t: T @@ -471,10 +720,12 @@ fn works_with_key_chains() { } } => { - ... on T { - x - y - } + ..._generated_onT2_0 + } + + fragment _generated_onT2_0 on T { + x + y } }, }, @@ -483,3 +734,192 @@ fn works_with_key_chains() { "### ); } + +#[test] +fn another_mix_of_fragments_indirection_and_unions() { + // This tests that the issue reported on https://github.com/apollographql/router/issues/3172 is resolved. + let planner = planner!( + config = generate_fragments_config(), + Subgraph1: r#" + type Query { + owner: Owner! + } + + interface OItf { + id: ID! + v0: String! + } + + type Owner implements OItf { + id: ID! + v0: String! + u: [U] + } + + union U = T1 | T2 + + interface I { + id1: ID! + id2: ID! + } + + type T1 implements I { + id1: ID! + id2: ID! + owner: Owner! + } + + type T2 implements I { + id1: ID! + id2: ID! + } + "#, + ); + assert_plan!( + &planner, + r#" + { + owner { + u { + ... on I { + id1 + id2 + } + ...Fragment1 + ...Fragment2 + } + } + } + + fragment Fragment1 on T1 { + owner { + ... on Owner { + ...Fragment3 + } + } + } + + fragment Fragment2 on T2 { + ...Fragment4 + id1 + } + + fragment Fragment3 on OItf { + v0 + } + + fragment Fragment4 on I { + id1 + id2 + __typename + } + "#, + @r###" + QueryPlan { + Fetch(service: "Subgraph1") { + { + owner { + u { + __typename + ..._generated_onI3_0 + ..._generated_onT11_0 + ..._generated_onT23_0 + } + } + } + + fragment _generated_onI3_0 on I { + __typename + id1 + id2 + } + + fragment _generated_onT11_0 on T1 { + owner { + v0 + } + } + + fragment _generated_onT23_0 on T2 { + __typename + id1 + id2 + } + }, + } + "### + ); + + assert_plan!( + &planner, + r#" + { + owner { + u { + ... on I { + id1 + id2 + } + ...Fragment1 + ...Fragment2 + } + } + } + + fragment Fragment1 on T1 { + owner { + ... on Owner { + ...Fragment3 + } + } + } + + fragment Fragment2 on T2 { + ...Fragment4 + id1 + } + + fragment Fragment3 on OItf { + v0 + } + + fragment Fragment4 on I { + id1 + id2 + } + "#, + @r###" + QueryPlan { + Fetch(service: "Subgraph1") { + { + owner { + u { + __typename + ..._generated_onI3_0 + ..._generated_onT11_0 + ..._generated_onT22_0 + } + } + } + + fragment _generated_onI3_0 on I { + __typename + id1 + id2 + } + + fragment _generated_onT11_0 on T1 { + owner { + v0 + } + } + + fragment _generated_onT22_0 on T2 { + id1 + id2 + } + }, + } + "### + ); +} diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments.rs deleted file mode 100644 index 959069588c..0000000000 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments.rs +++ /dev/null @@ -1,563 +0,0 @@ -use apollo_federation::query_plan::query_planner::QueryPlannerConfig; - -fn reuse_fragments_config() -> QueryPlannerConfig { - QueryPlannerConfig { - reuse_query_fragments: true, - ..Default::default() - } -} - -#[test] -fn handles_mix_of_fragments_indirection_and_unions() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - parent: Parent - } - - union CatOrPerson = Cat | Parent | Child - - type Parent { - childs: [Child] - } - - type Child { - id: ID! - } - - type Cat { - name: String - } - "#, - ); - assert_plan!( - &planner, - r#" - query { - parent { - ...F_indirection1_parent - } - } - - fragment F_indirection1_parent on Parent { - ...F_indirection2_catOrPerson - } - - fragment F_indirection2_catOrPerson on CatOrPerson { - ...F_catOrPerson - } - - fragment F_catOrPerson on CatOrPerson { - __typename - ... on Cat { - name - } - ... on Parent { - childs { - __typename - id - } - } - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - parent { - __typename - childs { - __typename - id - } - } - } - }, - } - "### - ); -} - -#[test] -fn another_mix_of_fragments_indirection_and_unions() { - // This tests that the issue reported on https://github.com/apollographql/router/issues/3172 is resolved. - - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - owner: Owner! - } - - interface OItf { - id: ID! - v0: String! - } - - type Owner implements OItf { - id: ID! - v0: String! - u: [U] - } - - union U = T1 | T2 - - interface I { - id1: ID! - id2: ID! - } - - type T1 implements I { - id1: ID! - id2: ID! - owner: Owner! - } - - type T2 implements I { - id1: ID! - id2: ID! - } - "#, - ); - assert_plan!( - &planner, - r#" - { - owner { - u { - ... on I { - id1 - id2 - } - ...Fragment1 - ...Fragment2 - } - } - } - - fragment Fragment1 on T1 { - owner { - ... on Owner { - ...Fragment3 - } - } - } - - fragment Fragment2 on T2 { - ...Fragment4 - id1 - } - - fragment Fragment3 on OItf { - v0 - } - - fragment Fragment4 on I { - id1 - id2 - __typename - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - owner { - u { - __typename - ...Fragment4 - ... on T1 { - owner { - v0 - } - } - ... on T2 { - ...Fragment4 - } - } - } - } - - fragment Fragment4 on I { - __typename - id1 - id2 - } - }, - } - "### - ); - - assert_plan!( - &planner, - r#" - { - owner { - u { - ... on I { - id1 - id2 - } - ...Fragment1 - ...Fragment2 - } - } - } - - fragment Fragment1 on T1 { - owner { - ... on Owner { - ...Fragment3 - } - } - } - - fragment Fragment2 on T2 { - ...Fragment4 - id1 - } - - fragment Fragment3 on OItf { - v0 - } - - fragment Fragment4 on I { - id1 - id2 - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - owner { - u { - __typename - ... on I { - __typename - ...Fragment4 - } - ... on T1 { - owner { - v0 - } - } - ... on T2 { - ...Fragment4 - } - } - } - } - - fragment Fragment4 on I { - id1 - id2 - } - }, - } - "### - ); -} - -#[test] -fn handles_fragments_with_interface_field_subtyping() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - t1: T1! - } - - interface I { - id: ID! - other: I! - } - - type T1 implements I { - id: ID! - other: T1! - } - - type T2 implements I { - id: ID! - other: T2! - } - "#, - ); - assert_plan!( - &planner, - r#" - { - t1 { - ...Fragment1 - } - } - - fragment Fragment1 on I { - other { - ... on T1 { - id - } - ... on T2 { - id - } - } - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - t1 { - other { - __typename - id - } - } - } - }, - } - "### - ); -} - -#[test] -fn can_reuse_fragments_in_subgraph_where_they_only_partially_apply_in_root_fetch() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - t1: T - t2: T - } - - type T @key(fields: "id") { - id: ID! - v0: Int - v1: Int - v2: Int - } - "#, - Subgraph2: r#" - type T @key(fields: "id") { - id: ID! - v3: Int - } - "#, - ); - assert_plan!( - &planner, - r#" - { - t1 { - ...allTFields - } - t2 { - ...allTFields - } - } - - fragment allTFields on T { - v0 - v1 - v2 - v3 - } - "#, - @r###" - QueryPlan { - Sequence { - Fetch(service: "Subgraph1") { - { - t1 { - __typename - ...allTFields - id - } - t2 { - __typename - ...allTFields - id - } - } - - fragment allTFields on T { - v0 - v1 - v2 - } - }, - Parallel { - Flatten(path: "t2") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - } - } => - { - ... on T { - v3 - } - } - }, - }, - Flatten(path: "t1") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - } - } => - { - ... on T { - v3 - } - } - }, - }, - }, - }, - } - "### - ); -} - -#[test] -fn can_reuse_fragments_in_subgraph_where_they_only_partially_apply_in_entity_fetch() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - t: T - } - - type T @key(fields: "id") { - id: ID! - } - "#, - Subgraph2: r#" - type T @key(fields: "id") { - id: ID! - u1: U - u2: U - } - - type U @key(fields: "id") { - id: ID! - v0: Int - v1: Int - } - "#, - Subgraph3: r#" - type U @key(fields: "id") { - id: ID! - v2: Int - v3: Int - } - "#, - ); - - assert_plan!( - &planner, - r#" - { - t { - u1 { - ...allUFields - } - u2 { - ...allUFields - } - } - } - - fragment allUFields on U { - v0 - v1 - v2 - v3 - } - "#, - @r###" - QueryPlan { - Sequence { - Fetch(service: "Subgraph1") { - { - t { - __typename - id - } - } - }, - Flatten(path: "t") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - } - } => - { - ... on T { - u1 { - __typename - ...allUFields - id - } - u2 { - __typename - ...allUFields - id - } - } - } - - fragment allUFields on U { - v0 - v1 - } - }, - }, - Parallel { - Flatten(path: "t.u2") { - Fetch(service: "Subgraph3") { - { - ... on U { - __typename - id - } - } => - { - ... on U { - v2 - v3 - } - } - }, - }, - Flatten(path: "t.u1") { - Fetch(service: "Subgraph3") { - { - ... on U { - __typename - id - } - } => - { - ... on U { - v2 - v3 - } - } - }, - }, - }, - }, - } - "### - ); -} diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_expansion.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_expansion.rs new file mode 100644 index 0000000000..5b68d3e059 --- /dev/null +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_expansion.rs @@ -0,0 +1,369 @@ +#[test] +fn handles_mix_of_fragments_indirection_and_unions() { + let planner = planner!( + Subgraph1: r#" + type Query { + parent: Parent + } + + union CatOrPerson = Cat | Parent | Child + + type Parent { + childs: [Child] + } + + type Child { + id: ID! + } + + type Cat { + name: String + } + "#, + ); + assert_plan!( + &planner, + r#" + query { + parent { + ...F_indirection1_parent + } + } + + fragment F_indirection1_parent on Parent { + ...F_indirection2_catOrPerson + } + + fragment F_indirection2_catOrPerson on CatOrPerson { + ...F_catOrPerson + } + + fragment F_catOrPerson on CatOrPerson { + __typename + ... on Cat { + name + } + ... on Parent { + childs { + __typename + id + } + } + } + "#, + @r###" + QueryPlan { + Fetch(service: "Subgraph1") { + { + parent { + __typename + childs { + __typename + id + } + } + } + }, + } + "### + ); +} + +#[test] +fn handles_fragments_with_interface_field_subtyping() { + let planner = planner!( + Subgraph1: r#" + type Query { + t1: T1! + } + + interface I { + id: ID! + other: I! + } + + type T1 implements I { + id: ID! + other: T1! + } + + type T2 implements I { + id: ID! + other: T2! + } + "#, + ); + + assert_plan!( + &planner, + r#" + { + t1 { + ...Fragment1 + } + } + + fragment Fragment1 on I { + other { + ... on T1 { + id + } + ... on T2 { + id + } + } + } + "#, + @r###" + QueryPlan { + Fetch(service: "Subgraph1") { + { + t1 { + other { + id + } + } + } + }, + } + "### + ); +} + +#[test] +fn it_preserves_directives() { + // (because used only once) + let planner = planner!( + Subgraph1: r#" + type Query { + t: T + } + + type T @key(fields: "id") { + id: ID! + a: Int + b: Int + } + "#, + ); + assert_plan!( + &planner, + r#" + query test($if: Boolean!) { + t { + id + ...OnT @include(if: $if) + } + } + + fragment OnT on T { + a + b + } + "#, + @r###" + QueryPlan { + Fetch(service: "Subgraph1") { + { + t { + id + ... on T @include(if: $if) { + a + b + } + } + } + }, + } + "### + ); +} + +#[test] +fn it_preserves_directives_when_fragment_is_reused() { + let planner = planner!( + Subgraph1: r#" + type Query { + t: T + } + + type T @key(fields: "id") { + id: ID! + a: Int + b: Int + } + "#, + ); + assert_plan!( + &planner, + r#" + query test($test1: Boolean!, $test2: Boolean!) { + t { + id + ...OnT @include(if: $test1) + ...OnT @include(if: $test2) + } + } + + fragment OnT on T { + a + b + } + "#, + @r###" + QueryPlan { + Fetch(service: "Subgraph1") { + { + t { + id + ... on T @include(if: $test1) { + a + b + } + ... on T @include(if: $test2) { + a + b + } + } + } + }, + } + "### + ); +} + +#[test] +fn it_preserves_directives_on_collapsed_fragments() { + let planner = planner!( + Subgraph1: r#" + type Query { + t: T + } + + type T { + id: ID! + t1: V + t2: V + } + + type V { + v1: Int + v2: Int + } + "#, + ); + assert_plan!( + &planner, + r#" + query($test: Boolean!) { + t { + ...OnT + } + } + + fragment OnT on T { + id + ...OnTInner @include(if: $test) + } + + fragment OnTInner on T { + t1 { + ...OnV + } + t2 { + ...OnV + } + } + + fragment OnV on V { + v1 + v2 + } + "#, + @r###" + QueryPlan { + Fetch(service: "Subgraph1") { + { + t { + id + ... on T @include(if: $test) { + t1 { + v1 + v2 + } + t2 { + v1 + v2 + } + } + } + } + }, + } + "### + ); +} + +#[test] +fn it_expands_nested_fragments() { + let planner = planner!( + Subgraph1: r#" + type Query { + t: T + } + + type T @key(fields: "id") { + id: ID! + a: V + b: V + } + + type V { + v1: Int + v2: Int + } + "#, + ); + assert_plan!( + &planner, + r#" + { + t { + ...OnT + } + } + + fragment OnT on T { + a { + ...OnV + } + b { + ...OnV + } + } + + fragment OnV on V { + v1 + v2 + } + "#, + @r###" + QueryPlan { + Fetch(service: "Subgraph1") { + { + t { + a { + v1 + v2 + } + b { + v1 + v2 + } + } + } + }, + } + "### + ); +} 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 deleted file mode 100644 index da90c3edb8..0000000000 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs +++ /dev/null @@ -1,1384 +0,0 @@ -use apollo_federation::query_plan::query_planner::QueryPlannerConfig; - -fn reuse_fragments_config() -> QueryPlannerConfig { - QueryPlannerConfig { - reuse_query_fragments: true, - ..Default::default() - } -} - -#[test] -fn it_works_with_nested_fragments_1() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - a: Anything - } - - union Anything = A1 | A2 | A3 - - interface Foo { - foo: String - child: Foo - child2: Foo - } - - type A1 implements Foo { - foo: String - child: Foo - child2: Foo - } - - type A2 implements Foo { - foo: String - child: Foo - child2: Foo - } - - type A3 implements Foo { - foo: String - child: Foo - child2: Foo - } - "#, - ); - assert_plan!( - &planner, - r#" - query { - a { - ... on A1 { - ...FooSelect - } - ... on A2 { - ...FooSelect - } - ... on A3 { - ...FooSelect - } - } - } - - fragment FooSelect on Foo { - __typename - foo - child { - ...FooChildSelect - } - child2 { - ...FooChildSelect - } - } - - fragment FooChildSelect on Foo { - __typename - foo - child { - child { - child { - foo - } - } - } - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - a { - __typename - ... on A1 { - ...FooSelect - } - ... on A2 { - ...FooSelect - } - ... on A3 { - ...FooSelect - } - } - } - - fragment FooChildSelect on Foo { - __typename - foo - child { - __typename - child { - __typename - child { - __typename - foo - } - } - } - } - - fragment FooSelect on Foo { - __typename - foo - child { - ...FooChildSelect - } - child2 { - ...FooChildSelect - } - } - }, - } - "### - ); -} - -#[test] -fn it_avoid_fragments_usable_only_once() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - t: T - } - - type T @key(fields: "id") { - id: ID! - v1: V - } - - type V @shareable { - a: Int - b: Int - c: Int - } - "#, - Subgraph2: r#" - type T @key(fields: "id") { - id: ID! - v2: V - v3: V - } - - type V @shareable { - a: Int - b: Int - c: Int - } - "#, - ); - - // We use a fragment which does save some on the original query, but as each - // field gets to a different subgraph, the fragment would only be used one - // on each sub-fetch and we make sure the fragment is not used in that case. - assert_plan!( - &planner, - r#" - query { - t { - v1 { - ...OnV - } - v2 { - ...OnV - } - } - } - - fragment OnV on V { - a - b - c - } - "#, - @r###" - QueryPlan { - Sequence { - Fetch(service: "Subgraph1") { - { - t { - __typename - id - v1 { - a - b - c - } - } - } - }, - Flatten(path: "t") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - } - } => - { - ... on T { - v2 { - a - b - c - } - } - } - }, - }, - }, - } - "### - ); - - // But double-check that if we query 2 fields from the same subgraph, then - // the fragment gets used now. - assert_plan!( - &planner, - r#" - query { - t { - v2 { - ...OnV - } - v3 { - ...OnV - } - } - } - - fragment OnV on V { - a - b - c - } - "#, - @r###" - QueryPlan { - Sequence { - Fetch(service: "Subgraph1") { - { - t { - __typename - id - } - } - }, - Flatten(path: "t") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - } - } => - { - ... on T { - v2 { - ...OnV - } - v3 { - ...OnV - } - } - } - - fragment OnV on V { - a - b - c - } - }, - }, - }, - } - "### - ); -} - -mod respects_query_planner_option_reuse_query_fragments { - use super::*; - - const SUBGRAPH1: &str = r#" - type Query { - t: T - } - - type T { - a1: A - a2: A - } - - type A { - x: Int - y: Int - } - "#; - const QUERY: &str = r#" - query { - t { - a1 { - ...Selection - } - a2 { - ...Selection - } - } - } - - fragment Selection on A { - x - y - } - "#; - - #[test] - fn respects_query_planner_option_reuse_query_fragments_true() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: SUBGRAPH1, - ); - let query = QUERY; - - assert_plan!( - &planner, - query, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - t { - a1 { - ...Selection - } - a2 { - ...Selection - } - } - } - - fragment Selection on A { - 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] -fn it_works_with_nested_fragments_when_only_the_nested_fragment_gets_preserved() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - t: T - } - - type T @key(fields: "id") { - id: ID! - a: V - b: V - } - - type V { - v1: Int - v2: Int - } - "#, - ); - assert_plan!( - &planner, - r#" - { - t { - ...OnT - } - } - - fragment OnT on T { - a { - ...OnV - } - b { - ...OnV - } - } - - fragment OnV on V { - v1 - v2 - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - t { - a { - ...OnV - } - b { - ...OnV - } - } - } - - fragment OnV on V { - v1 - v2 - } - }, - } - "### - ); -} - -#[test] -fn it_preserves_directives_when_fragment_not_used() { - // (because used only once) - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - t: T - } - - type T @key(fields: "id") { - id: ID! - a: Int - b: Int - } - "#, - ); - assert_plan!( - &planner, - r#" - query test($if: Boolean!) { - t { - id - ...OnT @include(if: $if) - } - } - - fragment OnT on T { - a - b - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - t { - id - ... on T @include(if: $if) { - a - b - } - } - } - }, - } - "### - ); -} - -#[test] -fn it_preserves_directives_when_fragment_is_reused() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - t: T - } - - type T @key(fields: "id") { - id: ID! - a: Int - b: Int - } - "#, - ); - assert_plan!( - &planner, - r#" - query test($test1: Boolean!, $test2: Boolean!) { - t { - id - ...OnT @include(if: $test1) - ...OnT @include(if: $test2) - } - } - - fragment OnT on T { - a - b - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - t { - id - ...OnT @include(if: $test1) - ...OnT @include(if: $test2) - } - } - - fragment OnT on T { - a - b - } - }, - } - "### - ); -} - -#[test] -fn it_does_not_try_to_apply_fragments_that_are_not_valid_for_the_subgraph() { - // Slightly artificial example for simplicity, but this highlight the problem. - // In that example, the only queried subgraph is the first one (there is in fact - // no way to ever reach the 2nd one), so the plan should mostly simply forward - // the query to the 1st subgraph, but a subtlety is that the named fragment used - // in the query is *not* valid for Subgraph1, because it queries `b` on `I`, but - // there is no `I.b` in Subgraph1. - // So including the named fragment in the fetch would be erroneous: the subgraph - // server would reject it when validating the query, and we must make sure it - // is not reused. - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - i1: I - i2: I - } - - interface I { - a: Int - } - - type T implements I { - a: Int - b: Int - } - "#, - Subgraph2: r#" - interface I { - a: Int - b: Int - } - "#, - ); - assert_plan!( - &planner, - r#" - query { - i1 { - ... on T { - ...Frag - } - } - i2 { - ... on T { - ...Frag - } - } - } - - fragment Frag on I { - b - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - i1 { - __typename - ... on T { - b - } - } - i2 { - __typename - ... on T { - b - } - } - } - }, - } - "### - ); -} - -#[test] -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 - // there). Further, the operations we test have a fragment on I (`IFrag` below) that is - // used "in the context of `Outer`" (at the top-level of fragment `OuterFrag`). - // - // What this all means is that `IFrag` can be rebased in `Subgraph2` "as is" because `I` - // exists there with all its fields, but as we rebase `OuterFrag` on `Subgraph2`, we - // cannot use `...IFrag` inside it (at the top-level), because `I` and `Outer` do - // no intersect in `Subgraph2` and this would be an invalid selection. - // - // Previous versions of the code were not handling this case and were error out by - // creating the invalid selection (#2721), and this test ensures this is fixed. - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type V @shareable { - x: Int - } - - interface I { - v: V - } - - type Outer implements I @key(fields: "id") { - id: ID! - v: V - } - "#, - Subgraph2: r#" - type Query { - outer1: Outer - outer2: Outer - } - - type V @shareable { - x: Int - } - - interface I { - v: V - w: Int - } - - type Inner implements I { - v: V - w: Int - } - - type Outer @key(fields: "id") { - id: ID! - inner: Inner - w: Int - } - "#, - ); - assert_plan!( - &planner, - r#" - query { - outer1 { - ...OuterFrag - } - outer2 { - ...OuterFrag - } - } - - fragment OuterFrag on Outer { - ...IFrag - inner { - ...IFrag - } - } - - fragment IFrag on I { - v { - x - } - } - "#, - @r#" - QueryPlan { - Sequence { - Fetch(service: "Subgraph2") { - { - outer1 { - __typename - ...OuterFrag - id - } - outer2 { - __typename - ...OuterFrag - id - } - } - - fragment OuterFrag on Outer { - inner { - v { - x - } - } - } - }, - Parallel { - Flatten(path: "outer2") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v { - x - } - } - } - }, - }, - Flatten(path: "outer1") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v { - x - } - } - } - }, - }, - }, - }, - } - "# - ); - - // We very slighly modify the operation to add an artificial indirection within `IFrag`. - // This does not really change the query, and should result in the same plan, but - // ensure the code handle correctly such indirection. - assert_plan!( - &planner, - r#" - query { - outer1 { - ...OuterFrag - } - outer2 { - ...OuterFrag - } - } - - fragment OuterFrag on Outer { - ...IFrag - inner { - ...IFrag - } - } - - fragment IFrag on I { - ...IFragDelegate - } - - fragment IFragDelegate on I { - v { - x - } - } - "#, - @r#" - QueryPlan { - Sequence { - Fetch(service: "Subgraph2") { - { - outer1 { - __typename - ...OuterFrag - id - } - outer2 { - __typename - ...OuterFrag - id - } - } - - fragment OuterFrag on Outer { - inner { - v { - x - } - } - } - }, - Parallel { - Flatten(path: "outer2") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v { - x - } - } - } - }, - }, - Flatten(path: "outer1") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v { - x - } - } - } - }, - }, - }, - }, - } - "# - ); - - // The previous cases tests the cases where nothing in the `...IFrag` spread at the - // top-level of `OuterFrag` applied at all: it all gets eliminated in the plan. But - // in the schema of `Subgraph2`, while `Outer` does not implement `I` (and does not - // have `v` in particular), it does contains field `w` that `I` also have, so we - // add that field to `IFrag` and make sure we still correctly query that field. - - assert_plan!( - &planner, - r#" - query { - outer1 { - ...OuterFrag - } - outer2 { - ...OuterFrag - } - } - - fragment OuterFrag on Outer { - ...IFrag - inner { - ...IFrag - } - } - - fragment IFrag on I { - v { - x - } - w - } - "#, - @r#" - QueryPlan { - Sequence { - Fetch(service: "Subgraph2") { - { - outer1 { - __typename - ...OuterFrag - id - } - outer2 { - __typename - ...OuterFrag - id - } - } - - fragment OuterFrag on Outer { - w - inner { - v { - x - } - w - } - } - }, - Parallel { - Flatten(path: "outer2") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v { - x - } - } - } - }, - }, - Flatten(path: "outer1") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v { - x - } - } - } - }, - }, - }, - }, - } - "# - ); -} - -#[test] -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. - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type V @shareable { - x: Int - } - - union U = Outer - - type Outer @key(fields: "id") { - id: ID! - v: Int - } - "#, - Subgraph2: r#" - type Query { - outer1: Outer - outer2: Outer - } - - union U = Inner - - type Inner { - v: Int - w: Int - } - - type Outer @key(fields: "id") { - id: ID! - inner: Inner - w: Int - } - "#, - ); - assert_plan!( - &planner, - r#" - query { - outer1 { - ...OuterFrag - } - outer2 { - ...OuterFrag - } - } - - fragment OuterFrag on Outer { - ...UFrag - inner { - ...UFrag - } - } - - fragment UFrag on U { - ... on Outer { - v - } - ... on Inner { - v - } - } - "#, - @r#" - QueryPlan { - Sequence { - Fetch(service: "Subgraph2") { - { - outer1 { - __typename - ...OuterFrag - id - } - outer2 { - __typename - ...OuterFrag - id - } - } - - fragment OuterFrag on Outer { - inner { - v - } - } - }, - Parallel { - Flatten(path: "outer2") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v - } - } - }, - }, - Flatten(path: "outer1") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v - } - } - }, - }, - }, - }, - } - "# - ); - - // We very slighly modify the operation to add an artificial indirection within `IFrag`. - // This does not really change the query, and should result in the same plan, but - // ensure the code handle correctly such indirection. - assert_plan!( - &planner, - r#" - query { - outer1 { - ...OuterFrag - } - outer2 { - ...OuterFrag - } - } - - fragment OuterFrag on Outer { - ...UFrag - inner { - ...UFrag - } - } - - fragment UFrag on U { - ...UFragDelegate - } - - fragment UFragDelegate on U { - ... on Outer { - v - } - ... on Inner { - v - } - } - "#, - @r#" - QueryPlan { - Sequence { - Fetch(service: "Subgraph2") { - { - outer1 { - __typename - ...OuterFrag - id - } - outer2 { - __typename - ...OuterFrag - id - } - } - - fragment OuterFrag on Outer { - inner { - v - } - } - }, - Parallel { - Flatten(path: "outer2") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v - } - } - }, - }, - Flatten(path: "outer1") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v - } - } - }, - }, - }, - }, - } - "# - ); - - // The previous cases tests the cases where nothing in the `...IFrag` spread at the - // top-level of `OuterFrag` applied at all: it all gets eliminated in the plan. But - // in the schema of `Subgraph2`, while `Outer` does not implement `I` (and does not - // have `v` in particular), it does contains field `w` that `I` also have, so we - // add that field to `IFrag` and make sure we still correctly query that field. - assert_plan!( - &planner, - r#" - query { - outer1 { - ...OuterFrag - } - outer2 { - ...OuterFrag - } - } - - fragment OuterFrag on Outer { - ...UFrag - inner { - ...UFrag - } - } - - fragment UFrag on U { - ... on Outer { - v - w - } - ... on Inner { - v - } - } - "#, - @r#" - QueryPlan { - Sequence { - Fetch(service: "Subgraph2") { - { - outer1 { - __typename - ...OuterFrag - id - } - outer2 { - __typename - ...OuterFrag - id - } - } - - fragment OuterFrag on Outer { - w - inner { - v - } - } - }, - Parallel { - Flatten(path: "outer2") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v - } - } - }, - }, - Flatten(path: "outer1") { - Fetch(service: "Subgraph1") { - { - ... on Outer { - __typename - id - } - } => - { - ... on Outer { - v - } - } - }, - }, - }, - }, - } - "# - ); -} - -#[test] -fn it_preserves_nested_fragments_when_outer_one_has_directives_and_is_eliminated() { - let planner = planner!( - config = reuse_fragments_config(), - Subgraph1: r#" - type Query { - t: T - } - - type T { - id: ID! - t1: V - t2: V - } - - type V { - v1: Int - v2: Int - } - "#, - ); - assert_plan!( - &planner, - r#" - query($test: Boolean!) { - t { - ...OnT @include(if: $test) - } - } - - fragment OnT on T { - t1 { - ...OnV - } - t2 { - ...OnV - } - } - - fragment OnV on V { - v1 - v2 - } - "#, - @r###" - QueryPlan { - Fetch(service: "Subgraph1") { - { - t { - ... on T @include(if: $test) { - t1 { - ...OnV - } - t2 { - ...OnV - } - } - } - } - - fragment OnV on V { - v1 - v2 - } - }, - } - "### - ); -} diff --git a/apollo-federation/tests/query_plan/supergraphs/inefficient_entity_fetches_to_same_subgraph.graphql b/apollo-federation/tests/query_plan/supergraphs/inefficient_entity_fetches_to_same_subgraph.graphql new file mode 100644 index 0000000000..8aaa3f274f --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/inefficient_entity_fetches_to_same_subgraph.graphql @@ -0,0 +1,97 @@ +# Composed from subgraphs with hash: b2221050efb89f6e4df71823675d2ea1fbe66a31 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) +{ + query: Query +} + +directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +interface I + @join__type(graph: SUBGRAPH1) + @join__type(graph: SUBGRAPH2) +{ + v: V + w: Int @join__field(graph: SUBGRAPH2) +} + +type Inner implements I + @join__implements(graph: SUBGRAPH2, interface: "I") + @join__type(graph: SUBGRAPH2) +{ + v: V + w: Int +} + +input join__ContextArgument { + name: String! + type: String! + context: String! + selection: join__FieldValue! +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +scalar join__FieldValue + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") + SUBGRAPH2 @join__graph(name: "Subgraph2", url: "none") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Outer implements I + @join__implements(graph: SUBGRAPH1, interface: "I") + @join__type(graph: SUBGRAPH1, key: "id") + @join__type(graph: SUBGRAPH2, key: "id") +{ + id: ID! + v: V @join__field(graph: SUBGRAPH1) + inner: Inner @join__field(graph: SUBGRAPH2) + w: Int @join__field(graph: SUBGRAPH2) +} + +type Query + @join__type(graph: SUBGRAPH1) + @join__type(graph: SUBGRAPH2) +{ + outer1: Outer @join__field(graph: SUBGRAPH2) + outer2: Outer @join__field(graph: SUBGRAPH2) +} + +type V + @join__type(graph: SUBGRAPH1) + @join__type(graph: SUBGRAPH2) +{ + x: Int +} diff --git a/apollo-federation/tests/query_plan/supergraphs/it_expands_nested_fragments.graphql b/apollo-federation/tests/query_plan/supergraphs/it_expands_nested_fragments.graphql new file mode 100644 index 0000000000..0d1594dcca --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/it_expands_nested_fragments.graphql @@ -0,0 +1,75 @@ +# Composed from subgraphs with hash: af8642bd2cc335a2823e7c95f48ce005d3c809f0 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) +{ + query: Query +} + +directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +input join__ContextArgument { + name: String! + type: String! + context: String! + selection: join__FieldValue! +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +scalar join__FieldValue + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Query + @join__type(graph: SUBGRAPH1) +{ + t: T +} + +type T + @join__type(graph: SUBGRAPH1, key: "id") +{ + id: ID! + a: V + b: V +} + +type V + @join__type(graph: SUBGRAPH1) +{ + v1: Int + v2: Int +} diff --git a/apollo-federation/tests/query_plan/supergraphs/it_handles_nested_fragment_generation_from_operation_with_fragments.graphql b/apollo-federation/tests/query_plan/supergraphs/it_handles_nested_fragment_generation_from_operation_with_fragments.graphql new file mode 100644 index 0000000000..bf45161fb0 --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/it_handles_nested_fragment_generation_from_operation_with_fragments.graphql @@ -0,0 +1,102 @@ +# Composed from subgraphs with hash: 7cb80bbad99a03ca0bb30082bd6f9eb6f7c1beff +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) +{ + query: Query +} + +directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +type A1 implements Foo + @join__implements(graph: SUBGRAPH1, interface: "Foo") + @join__type(graph: SUBGRAPH1) +{ + foo: String + child: Foo + child2: Foo +} + +type A2 implements Foo + @join__implements(graph: SUBGRAPH1, interface: "Foo") + @join__type(graph: SUBGRAPH1) +{ + foo: String + child: Foo + child2: Foo +} + +type A3 implements Foo + @join__implements(graph: SUBGRAPH1, interface: "Foo") + @join__type(graph: SUBGRAPH1) +{ + foo: String + child: Foo + child2: Foo +} + +union Anything + @join__type(graph: SUBGRAPH1) + @join__unionMember(graph: SUBGRAPH1, member: "A1") + @join__unionMember(graph: SUBGRAPH1, member: "A2") + @join__unionMember(graph: SUBGRAPH1, member: "A3") + = A1 | A2 | A3 + +interface Foo + @join__type(graph: SUBGRAPH1) +{ + foo: String + child: Foo + child2: Foo +} + +input join__ContextArgument { + name: String! + type: String! + context: String! + selection: join__FieldValue! +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +scalar join__FieldValue + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Query + @join__type(graph: SUBGRAPH1) +{ + a: Anything +} diff --git a/apollo-federation/tests/query_plan/supergraphs/it_preserves_directives.graphql b/apollo-federation/tests/query_plan/supergraphs/it_preserves_directives.graphql new file mode 100644 index 0000000000..95316d4353 --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/it_preserves_directives.graphql @@ -0,0 +1,68 @@ +# Composed from subgraphs with hash: 136ac120ab3c0a9b8ea4cb22cb440886a1b4a961 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) +{ + query: Query +} + +directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +input join__ContextArgument { + name: String! + type: String! + context: String! + selection: join__FieldValue! +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +scalar join__FieldValue + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Query + @join__type(graph: SUBGRAPH1) +{ + t: T +} + +type T + @join__type(graph: SUBGRAPH1, key: "id") +{ + id: ID! + a: Int + b: Int +} diff --git a/apollo-federation/tests/query_plan/supergraphs/it_preserves_directives_on_collapsed_fragments.graphql b/apollo-federation/tests/query_plan/supergraphs/it_preserves_directives_on_collapsed_fragments.graphql new file mode 100644 index 0000000000..7b9af26713 --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/it_preserves_directives_on_collapsed_fragments.graphql @@ -0,0 +1,75 @@ +# Composed from subgraphs with hash: fd162a5fc982fc2cd0a8d33e271831822b681137 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) +{ + query: Query +} + +directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +input join__ContextArgument { + name: String! + type: String! + context: String! + selection: join__FieldValue! +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +scalar join__FieldValue + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Query + @join__type(graph: SUBGRAPH1) +{ + t: T +} + +type T + @join__type(graph: SUBGRAPH1) +{ + id: ID! + t1: V + t2: V +} + +type V + @join__type(graph: SUBGRAPH1) +{ + v1: Int + v2: Int +} diff --git a/apollo-router/src/configuration/migrations/0031-reuse-query-fragments.yaml b/apollo-router/src/configuration/migrations/0031-reuse-query-fragments.yaml new file mode 100644 index 0000000000..97ea457053 --- /dev/null +++ b/apollo-router/src/configuration/migrations/0031-reuse-query-fragments.yaml @@ -0,0 +1,6 @@ +description: supergraph.experimental_reuse_query_fragments is deprecated +actions: + - type: log + level: warn + path: supergraph.experimental_reuse_query_fragments + log: "'supergraph.experimental_reuse_query_fragments' is not supported by the native query planner and this configuration option will be removed in the next release. Use 'supergraph.generate_query_fragments' instead." \ No newline at end of file diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 2df5ad8c62..e6892e0087 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -417,7 +417,6 @@ impl Configuration { &self, ) -> apollo_federation::query_plan::query_planner::QueryPlannerConfig { apollo_federation::query_plan::query_planner::QueryPlannerConfig { - reuse_query_fragments: self.supergraph.reuse_query_fragments.unwrap_or(true), subgraph_graphql_validation: false, generate_query_fragments: self.supergraph.generate_query_fragments, incremental_delivery: @@ -684,7 +683,8 @@ pub(crate) struct Supergraph { pub(crate) introspection: bool, /// Enable reuse of query fragments - /// Default: depends on the federation version + /// This feature is deprecated and will be removed in next release. + /// The config can only be set when the legacy query planner is explicitly enabled. #[serde(rename = "experimental_reuse_query_fragments")] pub(crate) reuse_query_fragments: Option, @@ -765,7 +765,8 @@ impl Supergraph { Some(false) } else { reuse_query_fragments } ), - generate_query_fragments: generate_query_fragments.unwrap_or_else(default_generate_query_fragments), + generate_query_fragments: generate_query_fragments + .unwrap_or_else(default_generate_query_fragments), early_cancel: early_cancel.unwrap_or_default(), experimental_log_on_broken_pipe: experimental_log_on_broken_pipe.unwrap_or_default(), } @@ -802,7 +803,8 @@ impl Supergraph { Some(false) } else { reuse_query_fragments } ), - generate_query_fragments: generate_query_fragments.unwrap_or_else(default_generate_query_fragments), + generate_query_fragments: generate_query_fragments + .unwrap_or_else(default_generate_query_fragments), early_cancel: early_cancel.unwrap_or_default(), experimental_log_on_broken_pipe: experimental_log_on_broken_pipe.unwrap_or_default(), } diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap index 6dd57e693f..db8f3f8851 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap @@ -6644,7 +6644,7 @@ expression: "&schema" }, "experimental_reuse_query_fragments": { "default": null, - "description": "Enable reuse of query fragments Default: depends on the federation version", + "description": "Enable reuse of query fragments This feature is deprecated and will be removed in next release. The config can only be set when the legacy query planner is explicitly enabled.", "nullable": true, "type": "boolean" }, diff --git a/apollo-router/src/configuration/tests.rs b/apollo-router/src/configuration/tests.rs index 21cb5fdb50..4a93e496cc 100644 --- a/apollo-router/src/configuration/tests.rs +++ b/apollo-router/src/configuration/tests.rs @@ -1096,19 +1096,3 @@ fn find_struct_name(lines: &[&str], line_number: usize) -> Option { }) .next() } - -#[test] -fn it_prevents_reuse_and_generate_query_fragments_simultaneously() { - let conf = Configuration::builder() - .supergraph( - Supergraph::builder() - .generate_query_fragments(true) - .reuse_query_fragments(true) - .build(), - ) - .build() - .unwrap(); - - assert!(conf.supergraph.generate_query_fragments); - assert_eq!(conf.supergraph.reuse_query_fragments, Some(false)); -} diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index cc058b5555..98a8832347 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -167,10 +167,6 @@ impl PlannerMode { configuration: &Configuration, ) -> Result, ServiceBuildError> { let config = apollo_federation::query_plan::query_planner::QueryPlannerConfig { - reuse_query_fragments: configuration - .supergraph - .reuse_query_fragments - .unwrap_or(true), subgraph_graphql_validation: false, generate_query_fragments: configuration.supergraph.generate_query_fragments, incremental_delivery: diff --git a/apollo-router/src/services/supergraph/tests.rs b/apollo-router/src/services/supergraph/tests.rs index ac2dbab21a..059d7baa09 100644 --- a/apollo-router/src/services/supergraph/tests.rs +++ b/apollo-router/src/services/supergraph/tests.rs @@ -3342,119 +3342,6 @@ async fn interface_object_typename() { insta::assert_json_snapshot!(stream.next_response().await.unwrap()); } -#[tokio::test] -async fn fragment_reuse() { - const SCHEMA: &str = r#"schema - @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) - { - query: Query - } - directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA - directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE - directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION - directive @join__graph(name: String!, url: String!) on ENUM_VALUE - directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR - directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION - directive @join__implements( graph: join__Graph! interface: String!) repeatable on OBJECT | INTERFACE - - scalar link__Import - - enum link__Purpose { - SECURITY - EXECUTION - } - scalar join__FieldSet - - enum join__Graph { - USER @join__graph(name: "user", url: "http://localhost:4001/graphql") - ORGA @join__graph(name: "orga", url: "http://localhost:4002/graphql") - } - - type Query - @join__type(graph: ORGA) - @join__type(graph: USER) - { - me: User @join__field(graph: USER) - } - - type User - @join__type(graph: ORGA, key: "id") - @join__type(graph: USER, key: "id") - { - id: ID! - name: String - organizations: [Organization] @join__field(graph: ORGA) - } - type Organization - @join__type(graph: ORGA, key: "id") - { - id: ID - name: String @join__field(graph: ORGA) - }"#; - - let subgraphs = MockedSubgraphs([ - ("user", MockSubgraph::builder().with_json( - serde_json::json!{{ - "query":"query Query__user__0($a:Boolean!=true$b:Boolean!=true){me{name ...on User@include(if:$a){__typename id}...on User@include(if:$b){__typename id}}}", - "operationName": "Query__user__0" - }}, - serde_json::json!{{"data": {"me": { "name": "Ada", "__typename": "User", "id": "1" }}}} - ).build()), - ("orga", MockSubgraph::builder().with_json( - serde_json::json!{{ - "query":"query Query__orga__1($representations:[_Any!]!$a:Boolean!=true$b:Boolean!=true){_entities(representations:$representations){...F@include(if:$a)...F@include(if:$b)}}fragment F on User{organizations{id name}}", - "operationName": "Query__orga__1", - "variables":{"representations":[{"__typename":"User","id":"1"}]} - }}, - serde_json::json!{{"data": {"_entities": [{ "organizations": [{"id": "2", "name": "Apollo"}] }]}}} - ).build()) - ].into_iter().collect()); - - let service = TestHarness::builder() - .configuration_json(serde_json::json!({ - "include_subgraph_errors": { "all": true }, - "supergraph": { - "generate_query_fragments": false, - "experimental_reuse_query_fragments": true, - } - })) - .unwrap() - .schema(SCHEMA) - .extra_plugin(subgraphs) - .build_supergraph() - .await - .unwrap(); - - let request = supergraph::Request::fake_builder() - .query( - r#"query Query($a: Boolean! = true, $b: Boolean! = true) { - me { - name - ...F @include(if: $a) - ...F @include(if: $b) - } - } - fragment F on User { - organizations { - id - name - } - }"#, - ) - .build() - .unwrap(); - let response = service - .oneshot(request) - .await - .unwrap() - .next_response() - .await - .unwrap(); - - insta::assert_json_snapshot!(response); -} - #[tokio::test] async fn abstract_types_in_requires() { let schema = r#"schema diff --git a/apollo-router/tests/integration/fixtures/query_planner_redis_config_update_reuse_query_fragments.router.yaml b/apollo-router/tests/integration/fixtures/query_planner_redis_config_update_reuse_query_fragments.router.yaml index 14136a0268..6f24cebc9a 100644 --- a/apollo-router/tests/integration/fixtures/query_planner_redis_config_update_reuse_query_fragments.router.yaml +++ b/apollo-router/tests/integration/fixtures/query_planner_redis_config_update_reuse_query_fragments.router.yaml @@ -8,4 +8,5 @@ supergraph: - redis://localhost:6379 ttl: 10s experimental_reuse_query_fragments: true - + generate_query_fragments: false +experimental_query_planner_mode: legacy \ No newline at end of file diff --git a/apollo-router/tests/integration/redis.rs b/apollo-router/tests/integration/redis.rs index 34e6b11093..8ff18e9cd4 100644 --- a/apollo-router/tests/integration/redis.rs +++ b/apollo-router/tests/integration/redis.rs @@ -51,7 +51,7 @@ async fn query_planner_cache() -> Result<(), BoxError> { } // If this test fails and the cache key format changed you'll need to update the key here. // Look at the top of the file for instructions on getting the new cache key. - let known_cache_key = "plan:cache:1:federation:v2.9.3:8c0b4bfb4630635c2b5748c260d686ddb301d164e5818c63d6d9d77e13631676:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:1cfc840090ac76a98f8bd51442f41fd6ca4c8d918b3f8d87894170745acf0734"; + let known_cache_key = "plan:cache:1:federation:v2.9.3:8c0b4bfb4630635c2b5748c260d686ddb301d164e5818c63d6d9d77e13631676:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:8f8ce6ad09f15c3d567a05f1c3d7230ab71b3366fcaebc9cc3bbfa356d55ac12"; let config = RedisConfig::from_url("redis://127.0.0.1:6379").unwrap(); let client = RedisClient::new(config, None, None, None); @@ -974,10 +974,21 @@ async fn connection_failure_blocks_startup() { #[tokio::test(flavor = "multi_thread")] async fn query_planner_redis_update_query_fragments() { + // If this test fails and the cache key format changed you'll need to update + // the key here. Look at the top of the file for instructions on getting + // the new cache key. + // + // You first need to follow the process and update the key in + // `test_redis_query_plan_config_update`, and then update the key in this + // test. + // + // This test requires graphos license, so make sure you have + // "TEST_APOLLO_KEY" and "TEST_APOLLO_GRAPH_REF" env vars set, otherwise the + // test just passes locally. test_redis_query_plan_config_update( // This configuration turns the fragment generation option *off*. include_str!("fixtures/query_planner_redis_config_update_query_fragments.router.yaml"), - "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:0ade8e18db172d9d51b36a2112513c15032d103100644df418a50596de3adfba", + "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:b030b297e8cc0fb51de5b683162be9a4a5a0023844597253e580f99672bdf2b4", ) .await; } @@ -1007,7 +1018,7 @@ async fn query_planner_redis_update_defer() { // test just passes locally. test_redis_query_plan_config_update( include_str!("fixtures/query_planner_redis_config_update_defer.router.yaml"), - "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:066f41523274aed2428e0f08c9de077ee748a1d8470ec31edb5224030a198f3b", + "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:ab8143af84859ddbed87fc3ac3b1f9c1e2271ffc8e58b58a666619ffc90bfc29", ) .await; } @@ -1029,11 +1040,12 @@ async fn query_planner_redis_update_type_conditional_fetching() { include_str!( "fixtures/query_planner_redis_config_update_type_conditional_fetching.router.yaml" ), - "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:b31d320db1af4015998cc89027f0ede2305dcc61724365e9b76d4252f90c7677", + "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:285740e3d6ca7533144f54f8395204d7c19c44ed16e48f22a3ea41195d60180b", ) .await; } +// TODO drop this test once we remove the JS QP #[tokio::test(flavor = "multi_thread")] async fn query_planner_redis_update_reuse_query_fragments() { // If this test fails and the cache key format changed you'll need to update @@ -1051,7 +1063,7 @@ async fn query_planner_redis_update_reuse_query_fragments() { include_str!( "fixtures/query_planner_redis_config_update_reuse_query_fragments.router.yaml" ), - "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:d54414eeede3a1bf631d88a84a1e3a354683be87746e79a69769cf18d919cc01", + "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:9af18c8afd568c197050fc1a60c52a8c98656f1775016110516fabfbedc135fe", ) .await; } @@ -1076,7 +1088,7 @@ async fn test_redis_query_plan_config_update(updated_config: &str, new_cache_key router.clear_redis_cache().await; // If the tests above are failing, this is the key that needs to be changed first. - let starting_key = "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:1cfc840090ac76a98f8bd51442f41fd6ca4c8d918b3f8d87894170745acf0734"; + let starting_key = "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:8f8ce6ad09f15c3d567a05f1c3d7230ab71b3366fcaebc9cc3bbfa356d55ac12"; assert_ne!(starting_key, new_cache_key, "starting_key (cache key for the initial config) and new_cache_key (cache key with the updated config) should not be equal. This either means that the cache key is not being generated correctly, or that the test is not actually checking the updated key."); router.execute_default_query().await; diff --git a/docs/source/reference/router/configuration.mdx b/docs/source/reference/router/configuration.mdx index 144fc63edf..ebf01cc9cb 100644 --- a/docs/source/reference/router/configuration.mdx +++ b/docs/source/reference/router/configuration.mdx @@ -1257,37 +1257,37 @@ example: ``` - -### Fragment generation and reuse + +### Automatic fragment generation By default, the router compresses subgraph requests by generating fragment definitions based on the shape of the subgraph operation. In many cases this significantly reduces the size of the query sent to subgraphs. -The router also supports an experimental algorithm that attempts to reuse fragments -from the original operation while forming subgraph requests. This experimental feature -used to be enabled by default, but is still available to support subgraphs that rely -on the specific shape of fragments in an operation: +You can explicitly opt-out of this behavior by specifying: ```yaml supergraph: generate_query_fragments: false - experimental_reuse_query_fragments: true ``` -Note that `generate_query_fragments` and `experimental_reuse_query_fragments` are -mutually exclusive; if both are explicitly set to `true`, `generate_query_fragments` -will take precedence. - -In the future, the `generate_query_fragments` option will be the only option for handling fragments. +The legacy query planner still supports an experimental algorithm that attempts to +reuse fragments from the original operation while forming subgraph requests. The +legacy query planner has to be explicitly enabled. This experimental feature used to +be enabled by default, but is still available to support subgraphs that rely on the +specific shape of fragments in an operation: - - - +```yaml +supergraph: + generate_query_fragments: false + experimental_reuse_query_fragments: true +``` -In the future, the `generate_query_fragments` option will be the only option for handling fragments. +Note that `generate_query_fragments` and `experimental_reuse_query_fragments` are +mutually exclusive; if both are explicitly set to `true`, `generate_query_fragments` +will take precedence.