From a1a2e50b78ddf5b13326ee9e8e09614829f0eedd 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 1/4] [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 19710d112fde2557f80aa2801d1c0a9828842184 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 2/4] 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 e32fa6b6eae60b4ee935e9d0c58bc0250fc62ca2 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 3/4] 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 1fe1f16287..855b4b3006 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -3911,7 +3911,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 abd7fcedb469b3fa187312a968f3e7c756f7af7f 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 4/4] 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()