From ca7627c83ab619d3f7aac6125791ac724bf626ef Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 10:47:21 -0600 Subject: [PATCH 01/18] [federation] drop experimental reuse fragment optimization Drop support for the experimental reuse fragment query optimization. This implementation was very slow and buggy due to its complexity. Auto generation of fragments is much simpler algorithm that in most cases produces better results (and much faster). --- apollo-federation/src/operation/optimize.rs | 395 ----- .../src/query_plan/fetch_dependency_graph.rs | 2 +- .../src/query_plan/query_planner.rs | 30 - .../query_plan/build_query_plan_tests.rs | 4 +- .../build_query_plan_tests/entities.rs | 142 ++ .../fragment_autogeneration.rs | 462 +++++- .../build_query_plan_tests/named_fragments.rs | 563 ------- .../named_fragments_expansion.rs | 369 +++++ .../named_fragments_preservation.rs | 1384 ----------------- ...nt_entity_fetches_to_same_subgraph.graphql | 97 ++ .../it_expands_nested_fragments.graphql | 75 + ...tion_from_operation_with_fragments.graphql | 102 ++ .../it_preserves_directives.graphql | 68 + ..._directives_on_collapsed_fragments.graphql | 75 + 14 files changed, 1382 insertions(+), 2386 deletions(-) create mode 100644 apollo-federation/tests/query_plan/build_query_plan_tests/entities.rs delete mode 100644 apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments.rs create mode 100644 apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_expansion.rs delete mode 100644 apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs create mode 100644 apollo-federation/tests/query_plan/supergraphs/inefficient_entity_fetches_to_same_subgraph.graphql create mode 100644 apollo-federation/tests/query_plan/supergraphs/it_expands_nested_fragments.graphql create mode 100644 apollo-federation/tests/query_plan/supergraphs/it_handles_nested_fragment_generation_from_operation_with_fragments.graphql create mode 100644 apollo-federation/tests/query_plan/supergraphs/it_preserves_directives.graphql create mode 100644 apollo-federation/tests/query_plan/supergraphs/it_preserves_directives_on_collapsed_fragments.graphql diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index c7e54b23f7..232389a729 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -48,7 +48,6 @@ use super::Containment; use super::ContainmentOptions; use super::DirectiveList; use super::Field; -use super::FieldSelection; use super::Fragment; use super::FragmentSpreadSelection; use super::HasSelectionKey; @@ -90,125 +89,6 @@ impl<'a> ReuseContext<'a> { } } -//============================================================================= -// 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 @@ -1264,68 +1144,6 @@ impl NamedFragments { //============================================================================= // `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 { @@ -1343,211 +1161,8 @@ impl From for Selection { } } -impl InlineFragmentSelection { - fn reuse_fragments_inner( - &self, - context: &ReuseContext<'_>, - validator: &mut FieldsConflictMultiBranchValidator, - fragments_at_type: &mut FragmentRestrictionAtTypeCache, - ) -> Result { - let optimized; - - 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, - }, - )?; - - match opt { - SelectionSetOrFragment::Fragment(fragment) => { - // We're fully matching the sub-selection. If the fragment condition is also - // this element condition, then we can replace the whole element by the spread - // (not just the sub-selection). - if *type_condition_position == fragment.type_condition_position { - // Optimized as `...`, dropping the original inline spread (`self`). - - // Note that `FullMatchingFragmentCondition::ForInlineFragmentSelection` - // above guarantees that this element directives are a superset of the - // fragment directives. But there can be additional directives, and in that - // case they should be kept on the spread. - // PORT_NOTE: We are assuming directives on fragment definitions are - // carried over to their spread sites as JS version does, which - // is handled differently in Rust version (see `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; - } - } - } 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")); - } - - // 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(), - )?; - - // 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()) - } - 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> { @@ -1567,16 +1182,6 @@ impl Operation { 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)] diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index b91699ecc4..3b9475dcea 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -2674,7 +2674,7 @@ impl FetchDependencyGraphNode { )? }; let operation = - operation_compression.compress(&self.subgraph_name, subgraph_schema, 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..fcbe67cb67 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 }; @@ -875,7 +854,6 @@ impl RebasedFragments { } pub(crate) enum SubgraphOperationCompression { - ReuseFragments(RebasedFragments), GenerateFragments, Disabled, } @@ -884,17 +862,9 @@ impl SubgraphOperationCompression { /// Compress a subgraph operation. pub(crate) fn compress( &mut self, - subgraph_name: &Arc, - subgraph_schema: &ValidFederationSchema, 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()?; 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..3bafc09ed4 --- /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 + } + } + } + }, + }, + }, + }, + } + "# + ); +} \ No newline at end of file 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 +} From a7df507867fd611d110e79e6208aa956f144cb25 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 12:33:23 -0600 Subject: [PATCH 02/18] drop fragment rebasing logic --- apollo-federation/src/operation/optimize.rs | 1 - apollo-federation/src/operation/rebase.rs | 672 +----------------- .../src/query_plan/fetch_dependency_graph.rs | 3 +- .../src/query_plan/query_planner.rs | 35 +- .../build_query_plan_tests/entities.rs | 2 +- 5 files changed, 6 insertions(+), 707 deletions(-) diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 232389a729..f0d969a2ef 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -1161,7 +1161,6 @@ impl From for Selection { } } - impl Operation { /// Optimize the parsed size of the operation by generating fragments based on the selections /// in the operation. diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 09b8799067..60988d5e21 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,23 +45,12 @@ 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 @@ -70,20 +58,17 @@ impl Selection { parent_type, named_fragments, schema, - on_non_rebaseable_selection, ) .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, ), } } @@ -94,7 +79,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( @@ -311,7 +296,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 @@ -348,7 +332,6 @@ impl FieldSelection { &rebased_base_type, named_fragments, schema, - on_non_rebaseable_selection, )?; if rebased_selection_set.selections.is_empty() { Err(RebaseError::EmptySelectionSet.into()) @@ -433,7 +416,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 @@ -485,7 +467,6 @@ impl FragmentSpreadSelection { parent_type, named_fragments, schema, - on_non_rebaseable_selection, )?; // 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 @@ -523,7 +504,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 +600,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 @@ -640,7 +620,6 @@ impl InlineFragmentSelection { &rebased_casted_type, named_fragments, schema, - on_non_rebaseable_selection, )?; if rebased_selection_set.selections.is_empty() { // empty selection set @@ -710,7 +689,6 @@ impl SelectionSet { parent_type: &CompositeTypeDefinitionPosition, named_fragments: &NamedFragments, schema: &ValidFederationSchema, - on_non_rebaseable_selection: OnNonRebaseableSelection, ) -> Result { let rebased_results = self .selections @@ -720,13 +698,7 @@ impl SelectionSet { 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()) }); Ok(SelectionSet { @@ -747,7 +719,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 +734,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/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index 3b9475dcea..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(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 fcbe67cb67..104d55be99 100644 --- a/apollo-federation/src/query_plan/query_planner.rs +++ b/apollo-federation/src/query_plan/query_planner.rs @@ -823,36 +823,6 @@ 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 { GenerateFragments, Disabled, @@ -860,10 +830,7 @@ pub(crate) enum SubgraphOperationCompression { impl SubgraphOperationCompression { /// Compress a subgraph operation. - pub(crate) fn compress( - &mut self, - operation: Operation, - ) -> Result { + pub(crate) fn compress(&mut self, operation: Operation) -> Result { match self { Self::GenerateFragments => { let mut operation = operation; 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 index 3bafc09ed4..53f50aad38 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/entities.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/entities.rs @@ -139,4 +139,4 @@ fn inefficient_entity_fetches_to_same_subgraph() { } "# ); -} \ No newline at end of file +} From 5f9228c2b1640808ffe8aff65bf94d10f56e3c45 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 12:43:55 -0600 Subject: [PATCH 03/18] default to empty fragments for normalized operation --- apollo-federation/src/operation/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index f93f805661..30d2d3afd8 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -3905,7 +3905,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) } From 980706b526d22c4ef3ef0b63f8c67e170c29d7f0 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 12:44:34 -0600 Subject: [PATCH 04/18] remove unused rebasing code --- apollo-federation/src/operation/optimize.rs | 278 -------------------- apollo-federation/src/operation/rebase.rs | 12 - 2 files changed, 290 deletions(-) diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index f0d969a2ef..f156085176 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -44,8 +44,6 @@ 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::Fragment; @@ -190,76 +188,6 @@ impl SelectionSet { } } -//============================================================================= -// 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); - } - - // 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)) - } -} - -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() - }) - } -} - //============================================================================= // Field validation @@ -686,212 +614,6 @@ enum SelectionSetOrFragment { 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`. - 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)?; - - // 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())?; - } - - optimized.add_local_selection_set(¬_covered_so_far)?; - Ok(optimized.into()) - } -} - //============================================================================= // Retain fragments in selection sets while expanding the rest diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 60988d5e21..efe0ee5d31 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -131,18 +131,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() From 37bf548de26c9252d00952484d078059b59db2f0 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 13:05:22 -0600 Subject: [PATCH 05/18] remove even more dead code.... --- apollo-federation/src/operation/mod.rs | 53 --- apollo-federation/src/operation/optimize.rs | 368 -------------------- 2 files changed, 421 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 30d2d3afd8..946c185ed4 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -3393,49 +3393,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 +3490,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. diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index f156085176..70b4b06bf2 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -439,24 +439,6 @@ struct FragmentRestrictionAtTypeCache { map: IndexMap<(Name, CompositeTypeDefinitionPosition), Arc>, } -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)?)), - )), - } - } -} - impl FragmentRestrictionAtType { fn new(selections: SelectionSet, validator: Option) -> Self { Self { @@ -492,121 +474,6 @@ impl FragmentRestrictionAtType { } } -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 - ) - }) - } -} - -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(), - - // To be able to use a matching inline fragment, it needs to have either no directives, - // or if it has some, then: - // 1. All it's directives should also be on the current element. - // 2. The type condition of this element should be the fragment's condition. because - // If those 2 conditions are true, we can replace the whole current inline fragment - // with the match spread and directives will still match. - Self::ForInlineFragmentSelection { - type_condition_position, - directives, - } => { - if fragment.directives.is_empty() { - return true; - } - - // PORT_NOTE: The JS version handles `@defer` directive differently. However, Rust - // version can't have `@defer` at this point (see comments on `enum SelectionKey` - // definition) - fragment.type_condition_position == **type_condition_position - && fragment - .directives - .iter() - .all(|d1| directives.iter().any(|d2| d1 == d2)) - } - } - } -} - /// The return type for `SelectionSet::try_optimize_with_fragments`. #[derive(derive_more::From)] enum SelectionSetOrFragment { @@ -614,55 +481,6 @@ enum SelectionSetOrFragment { Fragment(Node), } -//============================================================================= -// Retain fragments in selection sets while expanding the rest - -impl Selection { - /// Expand fragments that are not in the `fragments_to_keep`. - // PORT_NOTE: The JS version's name was `expandFragments`, which was confusing with - // `expand_all_fragments`. So, it was renamed to `retain_fragments`. - fn retain_fragments( - &self, - parent_type: &CompositeTypeDefinitionPosition, - fragments_to_keep: &NamedFragments, - ) -> Result { - match self { - Selection::FragmentSpread(fragment) => { - if fragments_to_keep.contains(&fragment.spread.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()) - } 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()) - } - } - } - - // 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 { @@ -677,192 +495,6 @@ impl From for SelectionMapperReturn { } } -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)?; - - // 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, - ) - } - - 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; - } - } -} - //============================================================================= // `reuse_fragments` methods (putting everything together) From d74803057c88411ae82a7aa75bc88765240743bd Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 13:08:52 -0600 Subject: [PATCH 06/18] I see dead code.... --- apollo-federation/src/operation/optimize.rs | 123 -------------------- 1 file changed, 123 deletions(-) diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 70b4b06bf2..9631731792 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -44,7 +44,6 @@ use apollo_compiler::executable::VariableDefinition; use apollo_compiler::Name; use apollo_compiler::Node; -use super::DirectiveList; use super::Field; use super::Fragment; use super::FragmentSpreadSelection; @@ -59,7 +58,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> { @@ -338,87 +336,6 @@ impl FieldsConflictValidator { } } -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) - } -} - //============================================================================= // Matching fragments with selection set (`try_optimize_with_fragments`) @@ -434,46 +351,6 @@ struct FragmentRestrictionAtType { validator: Option>, } -#[derive(Default)] -struct FragmentRestrictionAtTypeCache { - map: IndexMap<(Name, CompositeTypeDefinitionPosition), Arc>, -} - -impl FragmentRestrictionAtType { - fn new(selections: SelectionSet, validator: Option) -> Self { - Self { - selections, - validator: validator.map(Arc::new), - } - } - - // 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(); - let Some(first) = iter.next() else { - return true; - }; - iter.next().is_none() && first.is_typename_field() - } -} - /// The return type for `SelectionSet::try_optimize_with_fragments`. #[derive(derive_more::From)] enum SelectionSetOrFragment { From 2cda75183509bf85764bc0827eb64c89cf6aa64c Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 13:12:13 -0600 Subject: [PATCH 07/18] dead code... dead code everywhere --- apollo-federation/src/operation/optimize.rs | 191 -------------------- 1 file changed, 191 deletions(-) diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 9631731792..4d27737f3a 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -38,13 +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::Field; use super::Fragment; use super::FragmentSpreadSelection; use super::HasSelectionKey; @@ -59,32 +56,6 @@ use crate::error::FederationError; use crate::operation::FragmentSpread; use crate::operation::SelectionValue; -#[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()), - } - } -} - //============================================================================= // Selection/SelectionSet intersection/minus operations @@ -186,171 +157,9 @@ impl SelectionSet { } } -//============================================================================= -// Field validation - -// 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>>>, -} - -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); - } - } - } - } - } - } - 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)?)) - } -} - //============================================================================= // 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>, -} - /// The return type for `SelectionSet::try_optimize_with_fragments`. #[derive(derive_more::From)] enum SelectionSetOrFragment { From 21df2595e7decffe478246af4517773186576735 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:12:14 -0600 Subject: [PATCH 08/18] final test cleanup --- apollo-federation/src/operation/mod.rs | 124 -- apollo-federation/src/operation/optimize.rs | 1700 ----------------- apollo-federation/src/operation/rebase.rs | 53 +- .../src/operation/selection_map.rs | 5 - apollo-federation/src/operation/tests/mod.rs | 404 +++- .../src/query_plan/query_planner.rs | 141 +- 6 files changed, 401 insertions(+), 2026 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 946c185ed4..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 diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 4d27737f3a..7bdd0842a2 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -81,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 { @@ -126,35 +106,6 @@ 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, - )) - } } //============================================================================= @@ -220,25 +171,6 @@ impl Operation { self.named_fragments = generator.into_inner(); Ok(()) } - - // 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)] @@ -421,28 +353,9 @@ impl FragmentGenerator { #[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 @@ -476,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 efe0ee5d31..9243916327 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -54,22 +54,14 @@ impl Selection { ) -> Result { match self { Selection::Field(field) => field - .rebase_inner( - parent_type, - named_fragments, - schema, - ) + .rebase_inner(parent_type, named_fragments, schema) .map(|field| field.into()), - Selection::FragmentSpread(spread) => spread.rebase_inner( - parent_type, - named_fragments, - schema, - ), - Selection::InlineFragment(inline) => inline.rebase_inner( - parent_type, - named_fragments, - schema, - ), + Selection::FragmentSpread(spread) => { + spread.rebase_inner(parent_type, named_fragments, schema) + } + Selection::InlineFragment(inline) => { + inline.rebase_inner(parent_type, named_fragments, schema) + } } } @@ -316,11 +308,8 @@ impl FieldSelection { }); } - let rebased_selection_set = selection_set.rebase_inner( - &rebased_base_type, - named_fragments, - schema, - )?; + 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 { @@ -451,11 +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, - )?; + 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 @@ -604,11 +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, - )?; + 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()) @@ -681,13 +666,7 @@ impl SelectionSet { let rebased_results = self .selections .values() - .map(|selection| { - selection.rebase_inner( - parent_type, - named_fragments, - schema, - ) - }); + .map(|selection| selection.rebase_inner(parent_type, named_fragments, schema)); Ok(SelectionSet { schema: schema.clone(), 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/query_planner.rs b/apollo-federation/src/query_plan/query_planner.rs index 104d55be99..0c94adde1d 100644 --- a/apollo-federation/src/query_plan/query_planner.rs +++ b/apollo-federation/src/query_plan/query_planner.rs @@ -1261,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( @@ -1287,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(); @@ -1299,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 } }, } From 2af32cbcc245dd880d9f7309632daad71ed73b45 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:57:20 -0600 Subject: [PATCH 09/18] fix router config --- apollo-router/src/configuration/metrics.rs | 13 -- apollo-router/src/configuration/mod.rs | 34 +----- ...nfiguration__tests__schema_generation.snap | 6 - apollo-router/src/configuration/tests.rs | 16 --- .../src/query_planner/bridge_query_planner.rs | 4 - .../src/services/supergraph/tests.rs | 113 ------------------ ...g_update_reuse_query_fragments.router.yaml | 1 - 7 files changed, 5 insertions(+), 182 deletions(-) diff --git a/apollo-router/src/configuration/metrics.rs b/apollo-router/src/configuration/metrics.rs index 6ea8121a69..ed8263aecd 100644 --- a/apollo-router/src/configuration/metrics.rs +++ b/apollo-router/src/configuration/metrics.rs @@ -49,7 +49,6 @@ impl Metrics { data.populate_user_plugins_instrument(configuration); data.populate_query_planner_experimental_parallelism(configuration); data.populate_deno_or_rust_mode_instruments(configuration); - data.populate_legacy_fragment_usage(configuration); data.into() } @@ -498,18 +497,6 @@ impl InstrumentData { ); } - pub(crate) fn populate_legacy_fragment_usage(&mut self, configuration: &Configuration) { - // Fragment generation takes precedence over fragment reuse. Only report when fragment reuse is *actually active*. - if configuration.supergraph.reuse_query_fragments == Some(true) - && !configuration.supergraph.generate_query_fragments - { - self.data.insert( - "apollo.router.config.reuse_query_fragments".to_string(), - (1, HashMap::new()), - ); - } - } - pub(crate) fn populate_query_planner_experimental_parallelism( &mut self, configuration: &Configuration, diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 2df5ad8c62..985c1d6702 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -394,7 +394,7 @@ impl Configuration { pub(crate) fn js_query_planner_config(&self) -> router_bridge::planner::QueryPlannerConfig { router_bridge::planner::QueryPlannerConfig { - reuse_query_fragments: self.supergraph.reuse_query_fragments, + reuse_query_fragments: None, generate_query_fragments: Some(self.supergraph.generate_query_fragments), incremental_delivery: Some(router_bridge::planner::IncrementalDeliverySupport { enable_defer: Some(self.supergraph.defer_support), @@ -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: @@ -683,11 +682,6 @@ pub(crate) struct Supergraph { /// Default: false pub(crate) introspection: bool, - /// Enable reuse of query fragments - /// Default: depends on the federation version - #[serde(rename = "experimental_reuse_query_fragments")] - pub(crate) reuse_query_fragments: Option, - /// Enable QP generation of fragments for subgraph requests /// Default: true pub(crate) generate_query_fragments: bool, @@ -745,7 +739,6 @@ impl Supergraph { introspection: Option, defer_support: Option, query_planning: Option, - reuse_query_fragments: Option, generate_query_fragments: Option, early_cancel: Option, experimental_log_on_broken_pipe: Option, @@ -756,16 +749,8 @@ impl Supergraph { introspection: introspection.unwrap_or_else(default_graphql_introspection), defer_support: defer_support.unwrap_or_else(default_defer_support), query_planning: query_planning.unwrap_or_default(), - reuse_query_fragments: generate_query_fragments.and_then(|v| - if v { - if reuse_query_fragments.is_some_and(|v| v) { - // warn the user that both are enabled and it's overridden - tracing::warn!("Both 'generate_query_fragments' and 'experimental_reuse_query_fragments' are explicitly enabled, 'experimental_reuse_query_fragments' will be overridden to false"); - } - 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(), } @@ -782,7 +767,6 @@ impl Supergraph { introspection: Option, defer_support: Option, query_planning: Option, - reuse_query_fragments: Option, generate_query_fragments: Option, early_cancel: Option, experimental_log_on_broken_pipe: Option, @@ -793,16 +777,8 @@ impl Supergraph { introspection: introspection.unwrap_or_else(default_graphql_introspection), defer_support: defer_support.unwrap_or_else(default_defer_support), query_planning: query_planning.unwrap_or_default(), - reuse_query_fragments: generate_query_fragments.and_then(|v| - if v { - if reuse_query_fragments.is_some_and(|v| v) { - // warn the user that both are enabled and it's overridden - tracing::warn!("Both 'generate_query_fragments' and 'experimental_reuse_query_fragments' are explicitly enabled, 'experimental_reuse_query_fragments' will be overridden to false"); - } - 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..63490420e5 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 @@ -6642,12 +6642,6 @@ expression: "&schema" "description": "Log a message if the client closes the connection before the response is sent. Default: false.", "type": "boolean" }, - "experimental_reuse_query_fragments": { - "default": null, - "description": "Enable reuse of query fragments Default: depends on the federation version", - "nullable": true, - "type": "boolean" - }, "generate_query_fragments": { "default": true, "description": "Enable QP generation of fragments for subgraph requests Default: true", 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..ccf317cbbc 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 @@ -7,5 +7,4 @@ supergraph: urls: - redis://localhost:6379 ttl: 10s - experimental_reuse_query_fragments: true From c3cfac9acb9690abdc7ef626d26336e498610dbe Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 16:03:24 -0600 Subject: [PATCH 10/18] add changesets --- .changesets/breaking_drop_reuse_fragment.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changesets/breaking_drop_reuse_fragment.md 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 From 4cff666b4cf666d6b0d2f8e231ad04cfe6e12bee Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 27 Nov 2024 16:12:26 -0600 Subject: [PATCH 11/18] fix federation cli --- apollo-federation/cli/src/main.rs | 5 ----- 1 file changed, 5 deletions(-) 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 { From 7716029dec5bf634a54f02421a35238ea33e38d3 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Mon, 2 Dec 2024 14:37:56 -0600 Subject: [PATCH 12/18] re-add support for reuse fragments for JS QP --- apollo-router/src/configuration/metrics.rs | 13 +++++++ apollo-router/src/configuration/mod.rs | 37 ++++++++++++++++++- ...nfiguration__tests__schema_generation.snap | 6 +++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/apollo-router/src/configuration/metrics.rs b/apollo-router/src/configuration/metrics.rs index ed8263aecd..6ea8121a69 100644 --- a/apollo-router/src/configuration/metrics.rs +++ b/apollo-router/src/configuration/metrics.rs @@ -49,6 +49,7 @@ impl Metrics { data.populate_user_plugins_instrument(configuration); data.populate_query_planner_experimental_parallelism(configuration); data.populate_deno_or_rust_mode_instruments(configuration); + data.populate_legacy_fragment_usage(configuration); data.into() } @@ -497,6 +498,18 @@ impl InstrumentData { ); } + pub(crate) fn populate_legacy_fragment_usage(&mut self, configuration: &Configuration) { + // Fragment generation takes precedence over fragment reuse. Only report when fragment reuse is *actually active*. + if configuration.supergraph.reuse_query_fragments == Some(true) + && !configuration.supergraph.generate_query_fragments + { + self.data.insert( + "apollo.router.config.reuse_query_fragments".to_string(), + (1, HashMap::new()), + ); + } + } + pub(crate) fn populate_query_planner_experimental_parallelism( &mut self, configuration: &Configuration, diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 985c1d6702..724237ece5 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -394,7 +394,7 @@ impl Configuration { pub(crate) fn js_query_planner_config(&self) -> router_bridge::planner::QueryPlannerConfig { router_bridge::planner::QueryPlannerConfig { - reuse_query_fragments: None, + reuse_query_fragments: self.supergraph.reuse_query_fragments, generate_query_fragments: Some(self.supergraph.generate_query_fragments), incremental_delivery: Some(router_bridge::planner::IncrementalDeliverySupport { enable_defer: Some(self.supergraph.defer_support), @@ -416,6 +416,14 @@ impl Configuration { pub(crate) fn rust_query_planner_config( &self, ) -> apollo_federation::query_plan::query_planner::QueryPlannerConfig { + if self + .supergraph + .reuse_query_fragments + .is_some_and(|flag| flag) + { + // warn the user that reuse query fragments is unsupported for RS QP + tracing::warn!("'experimental_reuse_query_fragments' is not supported by the Rust QP and this configuration option will be removed in the next release. Use 'generate_query_fragments' instead."); + } apollo_federation::query_plan::query_planner::QueryPlannerConfig { subgraph_graphql_validation: false, generate_query_fragments: self.supergraph.generate_query_fragments, @@ -682,6 +690,13 @@ pub(crate) struct Supergraph { /// Default: false pub(crate) introspection: bool, + /// Enable reuse of query fragments + /// + /// This feature is deprecated and will be removed in next release. It is only available in JS QP. + /// Use generate_query_fragments instead. + #[serde(rename = "experimental_reuse_query_fragments")] + pub(crate) reuse_query_fragments: Option, + /// Enable QP generation of fragments for subgraph requests /// Default: true pub(crate) generate_query_fragments: bool, @@ -739,6 +754,7 @@ impl Supergraph { introspection: Option, defer_support: Option, query_planning: Option, + reuse_query_fragments: Option, generate_query_fragments: Option, early_cancel: Option, experimental_log_on_broken_pipe: Option, @@ -749,6 +765,15 @@ impl Supergraph { introspection: introspection.unwrap_or_else(default_graphql_introspection), defer_support: defer_support.unwrap_or_else(default_defer_support), query_planning: query_planning.unwrap_or_default(), + reuse_query_fragments: generate_query_fragments.and_then(|v| + if v { + if reuse_query_fragments.is_some_and(|v| v) { + // warn the user that both are enabled and it's overridden + tracing::warn!("Both 'generate_query_fragments' and 'experimental_reuse_query_fragments' are explicitly enabled, 'experimental_reuse_query_fragments' will be overridden to false"); + } + Some(false) + } else { reuse_query_fragments } + ), generate_query_fragments: generate_query_fragments .unwrap_or_else(default_generate_query_fragments), early_cancel: early_cancel.unwrap_or_default(), @@ -767,6 +792,7 @@ impl Supergraph { introspection: Option, defer_support: Option, query_planning: Option, + reuse_query_fragments: Option, generate_query_fragments: Option, early_cancel: Option, experimental_log_on_broken_pipe: Option, @@ -777,6 +803,15 @@ impl Supergraph { introspection: introspection.unwrap_or_else(default_graphql_introspection), defer_support: defer_support.unwrap_or_else(default_defer_support), query_planning: query_planning.unwrap_or_default(), + reuse_query_fragments: generate_query_fragments.and_then(|v| + if v { + if reuse_query_fragments.is_some_and(|v| v) { + // warn the user that both are enabled and it's overridden + tracing::warn!("Both 'generate_query_fragments' and 'experimental_reuse_query_fragments' are explicitly enabled, 'experimental_reuse_query_fragments' will be overridden to false"); + } + Some(false) + } else { reuse_query_fragments } + ), generate_query_fragments: generate_query_fragments .unwrap_or_else(default_generate_query_fragments), early_cancel: early_cancel.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 63490420e5..3dc4b1b134 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 @@ -6642,6 +6642,12 @@ expression: "&schema" "description": "Log a message if the client closes the connection before the response is sent. Default: false.", "type": "boolean" }, + "experimental_reuse_query_fragments": { + "default": null, + "description": "Enable reuse of query fragments\n\nThis feature is deprecated and will be removed in next release. It is only available in JS QP. Use generate_query_fragments instead.", + "nullable": true, + "type": "boolean" + }, "generate_query_fragments": { "default": true, "description": "Enable QP generation of fragments for subgraph requests Default: true", From cf1611039230e5a8d7e01f4115e43d0b54b94c1d Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Mon, 2 Dec 2024 14:49:30 -0600 Subject: [PATCH 13/18] update docs --- .../source/reference/router/configuration.mdx | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/docs/source/reference/router/configuration.mdx b/docs/source/reference/router/configuration.mdx index 144fc63edf..186f5d2bdf 100644 --- a/docs/source/reference/router/configuration.mdx +++ b/docs/source/reference/router/configuration.mdx @@ -1257,37 +1257,36 @@ 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 JavaScript query planner 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: - - - +```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. From 0a7777bbd7e22391957a054256b51417c2dda897 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Mon, 2 Dec 2024 16:52:58 -0600 Subject: [PATCH 14/18] fix unit test --- ...nner_redis_config_update_reuse_query_fragments.router.yaml | 4 +++- apollo-router/tests/integration/redis.rs | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) 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 ccf317cbbc..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 @@ -7,4 +7,6 @@ supergraph: urls: - 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..2e52e2434b 100644 --- a/apollo-router/tests/integration/redis.rs +++ b/apollo-router/tests/integration/redis.rs @@ -1034,6 +1034,7 @@ async fn query_planner_redis_update_type_conditional_fetching() { .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 From 946998ed90861a4902a0aadabefc684e73776dcb Mon Sep 17 00:00:00 2001 From: Iryna Shestak Date: Wed, 4 Dec 2024 16:55:22 +0100 Subject: [PATCH 15/18] update redis integration tests after removal of experimental_reuse_query_fragments (#6393) --- apollo-router/tests/integration/redis.rs | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/apollo-router/tests/integration/redis.rs b/apollo-router/tests/integration/redis.rs index 2e52e2434b..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,7 +1040,7 @@ 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; } @@ -1052,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; } @@ -1077,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; From fa59de1d72ff58c6f2ccb76205be53ed7bf303c9 Mon Sep 17 00:00:00 2001 From: Dariusz Kuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 4 Dec 2024 11:09:13 -0600 Subject: [PATCH 16/18] Apply suggestions from code review Co-authored-by: Iryna Shestak --- apollo-router/src/configuration/mod.rs | 5 ++--- ..._router__configuration__tests__schema_generation.snap | 2 +- docs/source/reference/router/configuration.mdx | 9 +++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 724237ece5..2a45561247 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -691,9 +691,8 @@ pub(crate) struct Supergraph { pub(crate) introspection: bool, /// Enable reuse of query fragments - /// - /// This feature is deprecated and will be removed in next release. It is only available in JS QP. - /// Use generate_query_fragments instead. + /// 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, 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 3dc4b1b134..d7eb44671f 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\n\nThis feature is deprecated and will be removed in next release. It is only available in JS QP. Use generate_query_fragments instead.", + "description": "Enable reuse of query fragments\n\nThis feature is deprecated and will be removed in next release.\nThe config can only be set when the legacy query planner is explicitly enabled.", "nullable": true, "type": "boolean" }, diff --git a/docs/source/reference/router/configuration.mdx b/docs/source/reference/router/configuration.mdx index 186f5d2bdf..ebf01cc9cb 100644 --- a/docs/source/reference/router/configuration.mdx +++ b/docs/source/reference/router/configuration.mdx @@ -1273,10 +1273,11 @@ supergraph: -The JavaScript query planner 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: +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: From 6689c02e143b0b5b2f3312f30d1355c6c54aad7d Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 4 Dec 2024 11:50:50 -0600 Subject: [PATCH 17/18] add migration config --- .../migrations/0031-reuse-query-fragments.yaml | 6 ++++++ apollo-router/src/configuration/mod.rs | 8 -------- 2 files changed, 6 insertions(+), 8 deletions(-) create mode 100644 apollo-router/src/configuration/migrations/0031-reuse-query-fragments.yaml 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 2a45561247..e6892e0087 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -416,14 +416,6 @@ impl Configuration { pub(crate) fn rust_query_planner_config( &self, ) -> apollo_federation::query_plan::query_planner::QueryPlannerConfig { - if self - .supergraph - .reuse_query_fragments - .is_some_and(|flag| flag) - { - // warn the user that reuse query fragments is unsupported for RS QP - tracing::warn!("'experimental_reuse_query_fragments' is not supported by the Rust QP and this configuration option will be removed in the next release. Use 'generate_query_fragments' instead."); - } apollo_federation::query_plan::query_planner::QueryPlannerConfig { subgraph_graphql_validation: false, generate_query_fragments: self.supergraph.generate_query_fragments, From efb8a4fa0f66247df2ff6a161ac0df02a191d293 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 4 Dec 2024 14:13:28 -0600 Subject: [PATCH 18/18] fix snapshot test --- .../apollo_router__configuration__tests__schema_generation.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d7eb44671f..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\n\nThis feature is deprecated and will be removed in next release.\nThe config can only be set when the legacy query planner is explicitly enabled.", + "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" },