From 74afe96e5c611e401cce9ed7adf82e17531a9119 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Tue, 3 Sep 2024 21:41:15 +0300 Subject: [PATCH 1/2] chore(tests): Update flaky tests retry list from CircleCI API (#5944) --- .config/nextest.toml | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/.config/nextest.toml b/.config/nextest.toml index 9deb55fd49..f2c4ef3618 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -24,14 +24,27 @@ or ( binary_id(=apollo-router) & test(=axum_factory::tests::response_with_custom or ( binary_id(=apollo-router) & test(=axum_factory::tests::response_with_custom_endpoint_wildcard) ) or ( binary_id(=apollo-router) & test(=axum_factory::tests::response_with_custom_prefix_endpoint) ) or ( binary_id(=apollo-router) & test(=axum_factory::tests::response_with_root_wildcard) ) +or ( binary_id(=apollo-router) & test(=layers::map_first_graphql_response::tests::test_map_first_graphql_response) ) or ( binary_id(=apollo-router) & test(=notification::tests::it_test_ttl) ) or ( binary_id(=apollo-router) & test(=plugins::authentication::subgraph::test::test_credentials_provider_refresh_on_stale) ) +or ( binary_id(=apollo-router) & test(=plugins::expose_query_plan::tests::it_expose_query_plan) ) +or ( binary_id(=apollo-router) & test(=plugins::include_subgraph_errors::test::it_does_not_redact_all_explicit_allow_account_explict_redact_for_product_query) ) +or ( binary_id(=apollo-router) & test(=plugins::include_subgraph_errors::test::it_does_not_redact_all_explicit_allow_review_explict_redact_for_product_query) ) +or ( binary_id(=apollo-router) & test(=plugins::include_subgraph_errors::test::it_does_not_redact_all_implicit_redact_product_explict_allow_for_product_query) ) +or ( binary_id(=apollo-router) & test(=plugins::include_subgraph_errors::test::it_does_redact_all_explicit_allow_account_explict_redact_for_account_query) ) +or ( binary_id(=apollo-router) & test(=plugins::include_subgraph_errors::test::it_does_redact_all_explicit_allow_product_explict_redact_for_product_query) ) +or ( binary_id(=apollo-router) & test(=plugins::include_subgraph_errors::test::it_redacts_all_subgraphs_implicit_redact) ) +or ( binary_id(=apollo-router) & test(=plugins::include_subgraph_errors::test::it_returns_valid_response) ) or ( binary_id(=apollo-router) & test(=plugins::telemetry::config_new::instruments::tests::test_instruments) ) or ( binary_id(=apollo-router) & test(=plugins::telemetry::metrics::apollo::test::apollo_metrics_enabled) ) or ( binary_id(=apollo-router) & test(=plugins::telemetry::tests::it_test_prometheus_metrics) ) or ( binary_id(=apollo-router) & test(=router::tests::basic_event_stream_test) ) or ( binary_id(=apollo-router) & test(=router::tests::schema_update_test) ) or ( binary_id(=apollo-router) & test(=services::subgraph_service::tests::test_subgraph_service_websocket_with_error) ) +or ( binary_id(=apollo-router) & test(=services::supergraph::tests::aliased_subgraph_data_rewrites_on_non_root_fetch) ) +or ( binary_id(=apollo-router) & test(=services::supergraph::tests::interface_object_typename_rewrites) ) +or ( binary_id(=apollo-router) & test(=services::supergraph::tests::only_query_interface_object_subgraph) ) +or ( binary_id(=apollo-router) & test(=uplink::license_stream::test::license_expander_claim_no_claim) ) or ( binary_id(=apollo-router) & test(=uplink::license_stream::test::license_expander_claim_pause_claim) ) or ( binary_id(=apollo-router) & test(=uplink::persisted_queries_manifest_stream::test::integration_test) ) or ( binary_id(=apollo-router) & test(=uplink::schema_stream::test::integration_test) ) @@ -96,21 +109,33 @@ or ( binary_id(=apollo-router::integration_tests) & test(=integration::lifecycle or ( binary_id(=apollo-router::integration_tests) & test(=integration::lifecycle::test_reload_config_valid) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::lifecycle::test_reload_config_with_broken_plugin) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::lifecycle::test_reload_config_with_broken_plugin_recovery) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::lifecycle::test_shutdown_with_idle_connection) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::query_planner::progressive_override_with_legacy_qp_reload_to_both_best_effort_keep_previous_config) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::apq) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::connection_failure_blocks_startup) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::entity_cache) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::entity_cache_authorization) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::query_planner_redis_update_defer) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::query_planner_redis_update_introspection) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::query_planner_redis_update_query_fragments) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::query_planner_redis_update_reuse_query_fragments) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::query_planner_redis_update_type_conditional_fetching) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::redis::test::connection_failure_blocks_startup) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::subgraph_response::test_invalid_error_locations_contains_negative_one_location) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::datadog::test_basic) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::datadog::test_resource_mapping_default) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::datadog::test_resource_mapping_override) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::datadog::test_span_metrics) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::datadog::test_with_parent_span) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::jaeger::test_decimal_trace_id) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::jaeger::test_default_operation) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::jaeger::test_local_root) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::jaeger::test_remote_root) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::jaeger::test_selected_operation) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::logging::test_json) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::logging::test_json_promote_span_attributes) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::logging::test_json_sampler_off) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::logging::test_json_uuid_format) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::logging::test_text) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::logging::test_text_sampler_off) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::metrics::test_bad_queries) ) @@ -118,6 +143,10 @@ or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::metrics::test_metrics_bad_query) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::metrics::test_metrics_reloading) ) or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::metrics::test_subgraph_auth_metrics) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::telemetry::propagation::test_trace_id_via_header) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::traffic_shaping::test_router_rate_limit) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::traffic_shaping::test_subgraph_rate_limit) ) +or ( binary_id(=apollo-router::integration_tests) & test(=integration::traffic_shaping::test_subgraph_timeout) ) or ( binary_id(=apollo-router::integration_tests) & test(=normal_query_with_defer_accept_header) ) or ( binary_id(=apollo-router::integration_tests) & test(=persisted_queries) ) or ( binary_id(=apollo-router::integration_tests) & test(=queries_should_work_over_get) ) @@ -129,6 +158,7 @@ or ( binary_id(=apollo-router::samples) & test(=/basic/query1) ) or ( binary_id(=apollo-router::samples) & test(=/basic/query2) ) or ( binary_id(=apollo-router::samples) & test(=/enterprise/entity-cache/invalidation) ) or ( binary_id(=apollo-router::samples) & test(=/enterprise/entity-cache/invalidation-subgraph) ) +or ( binary_id(=apollo-router::samples) & test(=/enterprise/entity-cache/invalidation-subgraph-name) ) or ( binary_id(=apollo-router::samples) & test(=/enterprise/entity-cache/invalidation-subgraph-type) ) or ( binary_id(=apollo-router::samples) & test(=/enterprise/query-planning-redis) ) or ( binary_id(=apollo-router::set_context) & test(=test_set_context) ) From f331c89985b779ac28d0a05466479fe4a5f46609 Mon Sep 17 00:00:00 2001 From: Tyler Bloom Date: Tue, 3 Sep 2024 22:58:23 -0400 Subject: [PATCH 2/2] Refactor SelectionSet::merge_selections_into and remove unnecessary directives in Selection::from_element (#5919) --- apollo-federation/src/operation/merging.rs | 259 ++++++++---------- apollo-federation/src/operation/mod.rs | 53 +++- apollo-federation/src/operation/optimize.rs | 165 +++++------ apollo-federation/src/operation/simplify.rs | 2 +- apollo-federation/src/operation/tests/mod.rs | 4 +- .../src/query_graph/graph_path.rs | 23 +- .../src/query_plan/conditions.rs | 31 ++- .../src/query_plan/fetch_dependency_graph.rs | 4 +- .../requires/include_skip.rs | 10 +- ...res_triggered_within_a_conditional.graphql | 2 +- ...t_requires_triggered_conditionally.graphql | 2 +- ..._multiple_conditional_are_involved.graphql | 2 +- 12 files changed, 283 insertions(+), 274 deletions(-) diff --git a/apollo-federation/src/operation/merging.rs b/apollo-federation/src/operation/merging.rs index 4c2b31cbd3..c938f2e564 100644 --- a/apollo-federation/src/operation/merging.rs +++ b/apollo-federation/src/operation/merging.rs @@ -1,14 +1,13 @@ //! Provides methods for recursively merging selections and selection sets. use std::sync::Arc; -use apollo_compiler::collections::IndexMap; +use selection_map::SelectionMap; use super::selection_map; use super::FieldSelection; use super::FieldSelectionValue; use super::FragmentSpreadSelection; use super::FragmentSpreadSelectionValue; -use super::HasSelectionKey as _; use super::InlineFragmentSelection; use super::InlineFragmentSelectionValue; use super::NamedFragments; @@ -16,6 +15,8 @@ use super::Selection; use super::SelectionSet; use super::SelectionValue; use crate::error::FederationError; +use crate::operation::HasSelectionKey; +use crate::schema::position::CompositeTypeDefinitionPosition; impl<'a> FieldSelectionValue<'a> { /// Merges the given field selections into this one. @@ -28,43 +29,38 @@ impl<'a> FieldSelectionValue<'a> { /// Returns an error if: /// - The parent type or schema of any selection does not match `self`'s. /// - Any selection does not select the same field position as `self`. - fn merge_into<'op>( - &mut self, - others: impl Iterator, - ) -> Result<(), FederationError> { + fn merge_into(&mut self, other: &FieldSelection) -> Result<(), FederationError> { let self_field = &self.get().field; - let mut selection_sets = vec![]; - for other in others { - let other_field = &other.field; - if other_field.schema != self_field.schema { - return Err(FederationError::internal( - "Cannot merge field selections from different schemas", - )); - } - if other_field.field_position != self_field.field_position { - return Err(FederationError::internal(format!( + let mut selection_set = None; + let other_field = &other.field; + if other_field.schema != self_field.schema { + return Err(FederationError::internal( + "Cannot merge field selections from different schemas", + )); + } + if other_field.field_position != self_field.field_position { + return Err(FederationError::internal(format!( "Cannot merge field selection for field \"{}\" into a field selection for field \"{}\"", other_field.field_position, self_field.field_position, ))); - } - if self.get().selection_set.is_some() { - let Some(other_selection_set) = &other.selection_set else { - return Err(FederationError::internal(format!( - "Field \"{}\" has composite type but not a selection set", - other_field.field_position, - ))); - }; - selection_sets.push(other_selection_set); - } else if other.selection_set.is_some() { + } + if self.get().selection_set.is_some() { + let Some(other_selection_set) = &other.selection_set else { return Err(FederationError::internal(format!( - "Field \"{}\" has non-composite type but also has a selection set", + "Field \"{}\" has composite type but not a selection set", other_field.field_position, ))); - } + }; + selection_set = Some(other_selection_set); + } else if other.selection_set.is_some() { + return Err(FederationError::internal(format!( + "Field \"{}\" has non-composite type but also has a selection set", + other_field.field_position, + ))); } if let Some(self_selection_set) = self.get_selection_set_mut() { - self_selection_set.merge_into(selection_sets.into_iter())?; + self_selection_set.merge_into(selection_set.into_iter())?; } Ok(()) } @@ -79,35 +75,26 @@ impl<'a> InlineFragmentSelectionValue<'a> { /// /// # Errors /// Returns an error if the parent type or schema of any selection does not match `self`'s. - fn merge_into<'op>( - &mut self, - others: impl Iterator, - ) -> Result<(), FederationError> { + fn merge_into(&mut self, other: &InlineFragmentSelection) -> Result<(), FederationError> { let self_inline_fragment = &self.get().inline_fragment; - let mut selection_sets = vec![]; - for other in others { - let other_inline_fragment = &other.inline_fragment; - if other_inline_fragment.schema != self_inline_fragment.schema { - return Err(FederationError::internal( - "Cannot merge inline fragment from different schemas", - )); - } - if other_inline_fragment.parent_type_position - != self_inline_fragment.parent_type_position - { - return Err(FederationError::internal( - format!( - "Cannot merge inline fragment of parent type \"{}\" into an inline fragment of parent type \"{}\"", - other_inline_fragment.parent_type_position, - self_inline_fragment.parent_type_position, - ), - )); - } - selection_sets.push(&other.selection_set); + let other_inline_fragment = &other.inline_fragment; + if other_inline_fragment.schema != self_inline_fragment.schema { + return Err(FederationError::internal( + "Cannot merge inline fragment from different schemas", + )); + } + if other_inline_fragment.parent_type_position != self_inline_fragment.parent_type_position { + return Err(FederationError::internal( + format!( + "Cannot merge inline fragment of parent type \"{}\" into an inline fragment of parent type \"{}\"", + other_inline_fragment.parent_type_position, + self_inline_fragment.parent_type_position, + ), + )); } + self.get_selection_set_mut() - .merge_into(selection_sets.into_iter())?; - Ok(()) + .merge_into(std::iter::once(&other.selection_set)) } } @@ -120,24 +107,19 @@ impl<'a> FragmentSpreadSelectionValue<'a> { /// /// # Errors /// Returns an error if the parent type or schema of any selection does not match `self`'s. - fn merge_into<'op>( - &mut self, - others: impl Iterator, - ) -> Result<(), FederationError> { + fn merge_into(&mut self, other: &FragmentSpreadSelection) -> Result<(), FederationError> { let self_fragment_spread = &self.get().spread; - for other in others { - let other_fragment_spread = &other.spread; - if other_fragment_spread.schema != self_fragment_spread.schema { - return Err(FederationError::internal( - "Cannot merge fragment spread from different schemas", - )); - } - // Nothing to do since the fragment spread is already part of the selection set. - // Fragment spreads are uniquely identified by fragment name and applied directives. - // Since there is already an entry for the same fragment spread, there is no point - // in attempting to merge its sub-selections, as the underlying entry should be - // exactly the same as the currently processed one. + let other_fragment_spread = &other.spread; + if other_fragment_spread.schema != self_fragment_spread.schema { + return Err(FederationError::internal( + "Cannot merge fragment spread from different schemas", + )); } + // Nothing to do since the fragment spread is already part of the selection set. + // Fragment spreads are uniquely identified by fragment name and applied directives. + // Since there is already an entry for the same fragment spread, there is no point + // in attempting to merge its sub-selections, as the underlying entry should be + // exactly the same as the currently processed one. Ok(()) } } @@ -173,68 +155,65 @@ impl SelectionSet { } selections_to_merge.extend(other.selections.values()); } - self.merge_selections_into(selections_to_merge.into_iter()) + self.merge_selections_into(selections_to_merge.into_iter(), false) } /// NOTE: This is a private API and should be used with care, use `add_selection` instead. /// /// A helper function for merging the given selections into this one. /// + /// The `do_fragment_inlining` flag enables a check to see if any inline fragments yielded from + /// `others` can be recursively merged into the selection set instead of just merging in the + /// fragment. This requires that the fragment has no directives and either has no type + /// condition or the type condition matches this selection set's type position. + /// /// # Errors /// Returns an error if the parent type or schema of any selection does not match `self`'s. /// /// Returns an error if any selection contains invalid GraphQL that prevents the merge. + #[allow(unreachable_code)] pub(super) fn merge_selections_into<'op>( &mut self, - others: impl Iterator, + mut others: impl Iterator, + do_fragment_inlining: bool, ) -> Result<(), FederationError> { - let mut fields = IndexMap::default(); - let mut fragment_spreads = IndexMap::default(); - let mut inline_fragments = IndexMap::default(); - let target = Arc::make_mut(&mut self.selections); - for other_selection in others { - let other_key = other_selection.key(); - match target.entry(other_key.clone()) { - selection_map::Entry::Occupied(existing) => match existing.get() { - Selection::Field(self_field_selection) => { - let Selection::Field(other_field_selection) = other_selection else { - return Err(FederationError::internal( - format!( - "Field selection key for field \"{}\" references non-field selection", - self_field_selection.field.field_position, - ), - )); + fn insert_selection( + target: &mut SelectionMap, + selection: &Selection, + ) -> Result<(), FederationError> { + match target.entry(selection.key()) { + selection_map::Entry::Vacant(vacant) => { + vacant.insert(selection.clone())?; + Ok(()) + } + selection_map::Entry::Occupied(mut entry) => match entry.get_mut() { + SelectionValue::Field(mut field) => { + let Selection::Field(other_field) = selection else { + return Err(FederationError::internal(format!( + "Field selection key for field \"{}\" references non-field selection", + field.get().field.field_position, + ))); }; - fields - .entry(other_key) - .or_insert_with(Vec::new) - .push(other_field_selection); + field.merge_into(other_field) } - Selection::FragmentSpread(self_fragment_spread_selection) => { - let Selection::FragmentSpread(other_fragment_spread_selection) = - other_selection - else { + SelectionValue::FragmentSpread(mut spread) => { + let Selection::FragmentSpread(other_spread) = selection else { return Err(FederationError::internal( format!( "Fragment spread selection key for fragment \"{}\" references non-field selection", - self_fragment_spread_selection.spread.fragment_name, + spread.get().spread.fragment_name, ), )); }; - fragment_spreads - .entry(other_key) - .or_insert_with(Vec::new) - .push(other_fragment_spread_selection); + spread.merge_into(other_spread) } - Selection::InlineFragment(self_inline_fragment_selection) => { - let Selection::InlineFragment(other_inline_fragment_selection) = - other_selection - else { + SelectionValue::InlineFragment(mut inline) => { + let Selection::InlineFragment(other_inline) = selection else { return Err(FederationError::internal( format!( "Inline fragment selection key under parent type \"{}\" {}references non-field selection", - self_inline_fragment_selection.inline_fragment.parent_type_position, - self_inline_fragment_selection.inline_fragment.type_condition_position.clone() + inline.get().inline_fragment.parent_type_position, + inline.get().inline_fragment.type_condition_position.clone() .map_or_else( String::new, |cond| format!("(type condition: {}) ", cond), @@ -242,53 +221,36 @@ impl SelectionSet { ), )); }; - inline_fragments - .entry(other_key) - .or_insert_with(Vec::new) - .push(other_inline_fragment_selection); + inline.merge_into(other_inline) } }, - selection_map::Entry::Vacant(vacant) => { - vacant.insert(other_selection.clone())?; - } } } - for (key, self_selection) in target.iter_mut() { - match self_selection { - SelectionValue::Field(mut self_field_selection) => { - if let Some(other_field_selections) = fields.shift_remove(key) { - self_field_selection.merge_into( - other_field_selections.iter().map(|selection| &***selection), - )?; - } - } - SelectionValue::FragmentSpread(mut self_fragment_spread_selection) => { - if let Some(other_fragment_spread_selections) = - fragment_spreads.shift_remove(key) - { - self_fragment_spread_selection.merge_into( - other_fragment_spread_selections - .iter() - .map(|selection| &***selection), - )?; - } - } - SelectionValue::InlineFragment(mut self_inline_fragment_selection) => { - if let Some(other_inline_fragment_selections) = - inline_fragments.shift_remove(key) - { - self_inline_fragment_selection.merge_into( - other_inline_fragment_selections - .iter() - .map(|selection| &***selection), - )?; + let target = Arc::make_mut(&mut self.selections); + + if do_fragment_inlining { + fn recurse_on_inline_fragment<'a>( + target: &mut SelectionMap, + type_pos: &CompositeTypeDefinitionPosition, + mut others: impl Iterator, + ) -> Result<(), FederationError> { + others.try_for_each(|selection| match selection { + Selection::InlineFragment(inline) if inline.is_unnecessary(type_pos) => { + recurse_on_inline_fragment( + target, + type_pos, + inline.selection_set.selections.values(), + ) } - } + selection => insert_selection(target, selection), + }) } - } - Ok(()) + recurse_on_inline_fragment(target, &self.type_position, others) + } else { + others.try_for_each(|selection| insert_selection(target, selection)) + } } /// Inserts a `Selection` into the inner map. Should a selection with the same key already @@ -305,13 +267,14 @@ impl SelectionSet { pub(crate) fn add_local_selection( &mut self, selection: &Selection, + do_fragment_inlining: bool, ) -> Result<(), FederationError> { debug_assert_eq!( &self.schema, selection.schema(), "In order to add selection it needs to point to the same schema" ); - self.merge_selections_into(std::iter::once(selection)) + self.merge_selections_into(std::iter::once(selection), do_fragment_inlining) } /// Inserts a `SelectionSet` into the inner map. Should any sub selection with the same key already diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index bd70e4698b..0b04790556 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -34,6 +34,7 @@ use crate::compat::coerce_executable_values; use crate::error::FederationError; use crate::error::SingleFederationError; use crate::error::SingleFederationError::Internal; +use crate::query_graph::graph_path::op_slice_condition_directives; use crate::query_graph::graph_path::OpPathElement; use crate::query_plan::conditions::Conditions; use crate::query_plan::FetchDataKeyRenamer; @@ -788,6 +789,7 @@ impl Selection { pub(crate) fn from_element( element: OpPathElement, sub_selections: Option, + unnecessary_directives: Option<&DirectiveList>, ) -> Result { // PORT_NOTE: This is TODO item is copied from the JS `selectionOfElement` function. // TODO: validate that the subSelection is ok for the element @@ -799,7 +801,21 @@ impl Selection { "unexpected inline fragment without sub-selections", )); }; - Ok(InlineFragmentSelection::new(inline_fragment, sub_selections).into()) + if let Some(unnecessary_directives) = unnecessary_directives { + let directives = inline_fragment + .directives + .iter() + .filter(|dir| !unnecessary_directives.contains(dir)) + .cloned() + .collect::(); + Ok(InlineFragmentSelection::new( + inline_fragment.with_updated_directives(directives), + sub_selections, + ) + .into()) + } else { + Ok(InlineFragmentSelection::new(inline_fragment, sub_selections).into()) + } } } } @@ -1670,6 +1686,16 @@ mod inline_fragment_selection { selection_set: self.selection_set.clone(), } } + pub(crate) fn with_updated_directives_and_selection_set( + &self, + directives: impl Into, + selection_set: SelectionSet, + ) -> Self { + Self { + inline_fragment: self.inline_fragment.with_updated_directives(directives), + selection_set, + } + } } impl HasSelectionKey for InlineFragmentSelection { @@ -1905,7 +1931,7 @@ impl SelectionSet { .flat_map(SelectionSet::split_top_level_fields) .filter_map(move |set| { let parent_type = ele.parent_type_position(); - Selection::from_element(ele.clone(), Some(set)) + Selection::from_element(ele.clone(), Some(set), None) .ok() .map(|sel| SelectionSet::from_selection(parent_type, sel)) }), @@ -2025,7 +2051,7 @@ impl SelectionSet { type_position, selections: Arc::new(SelectionMap::new()), }; - merged.merge_selections_into(normalized_selections.iter())?; + merged.merge_selections_into(normalized_selections.iter(), false)?; Ok(merged) } @@ -2122,7 +2148,7 @@ impl SelectionSet { type_position: self.type_position.clone(), selections: Arc::new(SelectionMap::new()), }; - expanded.merge_selections_into(expanded_selections.iter())?; + expanded.merge_selections_into(expanded_selections.iter(), false)?; Ok(expanded) } @@ -2496,7 +2522,7 @@ impl SelectionSet { sibling_typename.alias().cloned(), ); let typename_selection = - Selection::from_element(field_element.into(), /*subselection*/ None)?; + Selection::from_element(field_element.into(), /*subselection*/ None, None)?; Ok([typename_selection, updated].into_iter().collect()) }) } @@ -2613,11 +2639,13 @@ impl SelectionSet { let mut selection = Arc::make_mut(&mut self.selections) .entry(ele.key()) .or_insert(|| { + let unnecessary_directives = op_slice_condition_directives(path); Selection::from_element( element, // We immediately add a selection afterward to make this selection set // valid. Some(SelectionSet::empty(self.schema.clone(), sub_selection_type)), + Some(&unnecessary_directives), ) })?; match &mut selection { @@ -2649,9 +2677,11 @@ impl SelectionSet { if !ele.is_terminal()? { return Ok(()); } else { + let unneeded_directives = op_slice_condition_directives(path); // add leaf - let selection = Selection::from_element(element, None)?; - self.add_local_selection(&selection)? + let selection = + Selection::from_element(element, None, Some(&unneeded_directives))?; + self.add_local_selection(&selection, true)? } } else { let selection_set = selection_set @@ -2666,8 +2696,13 @@ impl SelectionSet { }) .transpose()? .map(|selection_set| selection_set.without_unnecessary_fragments()); - let selection = Selection::from_element(element, selection_set)?; - self.add_local_selection(&selection)? + let unneeded_directives = op_slice_condition_directives(path); + let selection = Selection::from_element( + element, + selection_set, + Some(&unneeded_directives), + )?; + self.add_local_selection(&selection, true)?; } } // If we don't have any path, we rebase and merge in the given sub selections at the root. diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 79fe71c8ef..ce2802bf4e 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -1007,7 +1007,7 @@ impl SelectionSet { &fragment, /*directives*/ &Default::default(), ); - optimized.add_local_selection(&fragment_selection.into())?; + optimized.add_local_selection(&fragment_selection.into(), false)?; } optimized.add_local_selection_set(¬_covered_so_far)?; @@ -1697,17 +1697,21 @@ impl FragmentGenerator { self.visit_selection_set(selection_set)?; } new_selection_set - .add_local_selection(&Selection::Field(Arc::clone(field.get())))?; + .add_local_selection(&Selection::Field(Arc::clone(field.get())), false)?; } SelectionValue::FragmentSpread(frag) => { - new_selection_set - .add_local_selection(&Selection::FragmentSpread(Arc::clone(frag.get())))?; + new_selection_set.add_local_selection( + &Selection::FragmentSpread(Arc::clone(frag.get())), + false, + )?; } SelectionValue::InlineFragment(frag) if !Self::is_worth_using(&frag.get().selection_set) => { - new_selection_set - .add_local_selection(&Selection::InlineFragment(Arc::clone(frag.get())))?; + new_selection_set.add_local_selection( + &Selection::InlineFragment(Arc::clone(frag.get())), + false, + )?; } SelectionValue::InlineFragment(mut candidate) => { self.visit_selection_set(candidate.get_selection_set_mut())?; @@ -1725,9 +1729,10 @@ impl FragmentGenerator { // we can't just transfer them to the generated fragment spread, // so we have to keep this inline fragment. let Ok(skip_include) = skip_include else { - new_selection_set.add_local_selection(&Selection::InlineFragment( - Arc::clone(candidate.get()), - ))?; + new_selection_set.add_local_selection( + &Selection::InlineFragment(Arc::clone(candidate.get())), + false, + )?; continue; }; @@ -1736,9 +1741,10 @@ impl FragmentGenerator { // there's any directives on it. This code duplicates the body from the // previous condition so it's very easy to remove when we're ready :) if !skip_include.is_empty() { - new_selection_set.add_local_selection(&Selection::InlineFragment( - Arc::clone(candidate.get()), - ))?; + new_selection_set.add_local_selection( + &Selection::InlineFragment(Arc::clone(candidate.get())), + false, + )?; continue; } @@ -1763,8 +1769,8 @@ impl FragmentGenerator { }); self.fragments.get(&name).unwrap() }; - new_selection_set.add_local_selection(&Selection::from( - FragmentSpreadSelection { + new_selection_set.add_local_selection( + &Selection::from(FragmentSpreadSelection { spread: FragmentSpread::new(FragmentSpreadData { schema: selection_set.schema.clone(), fragment_name: existing.name.clone(), @@ -1774,8 +1780,9 @@ impl FragmentGenerator { selection_id: crate::operation::SelectionId::new(), }), selection_set: existing.selection_set.clone(), - }, - ))?; + }), + false, + )?; } } } @@ -2281,12 +2288,12 @@ mod tests { type Query { t: T } - + type T { a: A b: Int } - + type A { x: String y: String @@ -2312,14 +2319,14 @@ mod tests { x y } - + fragment FT on T { a { __typename ...FA } } - + query { t { ...FT @@ -2346,19 +2353,19 @@ mod tests { type Query { t: T } - + type T { a: String b: B c: Int d: D } - + type B { x: String y: String } - + type D { m: String n: String @@ -2376,7 +2383,7 @@ mod tests { m } } - + { t { ...FragT @@ -2415,23 +2422,23 @@ mod tests { type Query { i: I } - + interface I { a: String } - + type T implements I { a: String b: B c: Int d: D } - + type B { x: String y: String } - + type D { m: String n: String @@ -2449,7 +2456,7 @@ mod tests { m } } - + { i { ... on T { @@ -2489,19 +2496,19 @@ mod tests { type Query { t: T } - + type T { a: String b: B c: Int d: D } - + type B { x: String y: String } - + type D { m: String n: String @@ -2527,7 +2534,7 @@ mod tests { m } } - + fragment Frag2 on T { a b { @@ -2539,7 +2546,7 @@ mod tests { n } } - + { t { ...Frag1 @@ -2573,11 +2580,11 @@ mod tests { type Query { t: T } - + interface I { x: String } - + type T implements I { x: String a: String @@ -2591,7 +2598,7 @@ mod tests { a } } - + { t { ...FragI @@ -2615,12 +2622,12 @@ mod tests { type Query { t: T } - + type T { a: String u: U } - + type U { x: String y: String @@ -2631,7 +2638,7 @@ mod tests { fragment Frag1 on T { a } - + fragment Frag2 on T { u { x @@ -2639,13 +2646,13 @@ mod tests { } ...Frag1 } - + fragment Frag3 on Query { t { ...Frag2 } } - + { ...Frag3 } @@ -2670,16 +2677,16 @@ mod tests { type Query { t1: T1 } - + interface I { x: Int } - + type T1 implements I { x: Int y: Int } - + type T2 implements I { x: Int z: Int @@ -2695,7 +2702,7 @@ mod tests { z } } - + { t1 { ...FragOnI @@ -2718,24 +2725,24 @@ mod tests { type Query { i2: I2 } - + interface I1 { x: Int } - + interface I2 { y: Int } - + interface I3 { z: Int } - + type T1 implements I1 & I2 { x: Int y: Int } - + type T2 implements I1 & I3 { x: Int z: Int @@ -2751,7 +2758,7 @@ mod tests { z } } - + { i2 { ...FragOnI1 @@ -2781,13 +2788,13 @@ mod tests { type Query { t1: T1 } - + union U = T1 | T2 - + type T1 { x: Int } - + type T2 { y: Int } @@ -2802,7 +2809,7 @@ mod tests { y } } - + { t1 { ...OnU @@ -2912,18 +2919,18 @@ mod tests { type Query { t1: T1 } - + union U1 = T1 | T2 | T3 union U2 = T2 | T3 - + type T1 { x: Int } - + type T2 { y: Int } - + type T3 { z: Int } @@ -2935,7 +2942,7 @@ mod tests { ...Outer } } - + fragment Outer on U1 { ... on T1 { x @@ -2947,7 +2954,7 @@ mod tests { ... Inner } } - + fragment Inner on U2 { ... on T2 { y @@ -3006,23 +3013,23 @@ mod tests { type Query { t1: T1 } - + union U1 = T1 | T2 | T3 union U2 = T2 | T3 - + type T1 { x: Int } - + type T2 { y1: Y y2: Y } - + type T3 { z: Int } - + type Y { v: Int } @@ -3034,7 +3041,7 @@ mod tests { ...Outer } } - + fragment Outer on U1 { ... on T1 { x @@ -3046,7 +3053,7 @@ mod tests { ... Inner } } - + fragment Inner on U2 { ... on T2 { y1 { @@ -3057,7 +3064,7 @@ mod tests { } } } - + fragment WillBeUnused on Y { v } @@ -3092,14 +3099,14 @@ mod tests { t1: T t2: T } - + type T { a1: Int a2: Int b1: B b2: B } - + type B { x: Int y: Int @@ -3115,7 +3122,7 @@ mod tests { ...TFields } } - + fragment TFields on T { ...DirectFieldsOfT b1 { @@ -3125,12 +3132,12 @@ mod tests { ...BFields } } - + fragment DirectFieldsOfT on T { a1 a2 } - + fragment BFields on B { x y @@ -3213,7 +3220,7 @@ mod tests { t2: T t3: T } - + type T { a: Int b: Int @@ -3226,7 +3233,7 @@ mod tests { fragment DirectiveInDef on T { a @include(if: $cond1) } - + query myQuery($cond1: Boolean!, $cond2: Boolean!) { t1 { a @@ -3263,7 +3270,7 @@ mod tests { t2: T t3: T } - + type T { a: Int b: Int @@ -3276,7 +3283,7 @@ mod tests { fragment NoDirectiveDef on T { a } - + query myQuery($cond1: Boolean!) { t1 { ...NoDirectiveDef diff --git a/apollo-federation/src/operation/simplify.rs b/apollo-federation/src/operation/simplify.rs index 8555b241a2..802bbfcab1 100644 --- a/apollo-federation/src/operation/simplify.rs +++ b/apollo-federation/src/operation/simplify.rs @@ -460,7 +460,7 @@ impl SelectionSet { { match selection_or_set { SelectionOrSet::Selection(normalized_selection) => { - normalized_selections.add_local_selection(&normalized_selection)?; + normalized_selections.add_local_selection(&normalized_selection, false)?; } SelectionOrSet::SelectionSet(normalized_set) => { // Since the `selection` has been expanded/lifted, we use diff --git a/apollo-federation/src/operation/tests/mod.rs b/apollo-federation/src/operation/tests/mod.rs index 89c9817587..276b0a579f 100644 --- a/apollo-federation/src/operation/tests/mod.rs +++ b/apollo-federation/src/operation/tests/mod.rs @@ -1204,7 +1204,7 @@ mod make_selection_tests { base_selection_set.type_position.clone(), selection.clone(), ); - Selection::from_element(base.element().unwrap(), Some(subselections)).unwrap() + Selection::from_element(base.element().unwrap(), Some(subselections), None).unwrap() }; let foo_with_a = clone_selection_at_path(foo, &[name!("a")]); @@ -1331,7 +1331,7 @@ mod lazy_map_tests { let field_element = Field::new_introspection_typename(s.schema(), &parent_type_pos, None); let typename_selection = - Selection::from_element(field_element.into(), /*subselection*/ None)?; + Selection::from_element(field_element.into(), /*subselection*/ None, None)?; // return `updated` and `typename_selection` Ok([updated, typename_selection].into_iter().collect()) }) diff --git a/apollo-federation/src/query_graph/graph_path.rs b/apollo-federation/src/query_graph/graph_path.rs index b9486f8434..1cc8984509 100644 --- a/apollo-federation/src/query_graph/graph_path.rs +++ b/apollo-federation/src/query_graph/graph_path.rs @@ -3656,6 +3656,18 @@ impl ClosedBranch { } } +pub fn op_slice_condition_directives(path: &[Arc]) -> DirectiveList { + path.iter() + .flat_map(|path_element| { + path_element + .directives() + .iter() + .filter(|d| d.name == "include" || d.name == "skip") + }) + .cloned() + .collect() +} + impl OpPath { pub fn len(&self) -> usize { self.0.len() @@ -3678,16 +3690,7 @@ impl OpPath { } pub(crate) fn conditional_directives(&self) -> DirectiveList { - self.0 - .iter() - .flat_map(|path_element| { - path_element - .directives() - .iter() - .filter(|d| d.name == "include" || d.name == "skip") - }) - .cloned() - .collect() + op_slice_condition_directives(&self.0) } /// Filter any fragment element in the provided path whose type condition does not exist in the provided schema. diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs index d4a84b9f49..a91681de68 100644 --- a/apollo-federation/src/query_plan/conditions.rs +++ b/apollo-federation/src/query_plan/conditions.rs @@ -223,12 +223,12 @@ pub(crate) fn remove_conditions_from_selection_set( selection.with_updated_selection_set(Some(updated_selection_set))? } } else { - Selection::from_element(updated_element, Some(updated_selection_set))? + Selection::from_element(updated_element, Some(updated_selection_set), None)? } } else if updated_element == element { selection.clone() } else { - Selection::from_element(updated_element, None)? + Selection::from_element(updated_element, None, None)? }; selection_map.insert(new_selection); } @@ -262,14 +262,12 @@ pub(crate) fn remove_unneeded_top_level_fragment_directives( // if there is no type condition we should preserve the directive info selection_map.insert(selection.clone()); } else { - let mut needed_directives: Vec> = Vec::new(); - if fragment.directives.len() > 0 { - for directive in fragment.directives.iter() { - if !unneded_directives.contains(directive) { - needed_directives.push(directive.clone()); - } - } - } + let needed_directives: Vec> = fragment + .directives + .iter() + .filter(|directive| !unneded_directives.contains(directive)) + .cloned() + .collect(); // We recurse, knowing that we'll stop as soon as we hit field selections, so this only cover the fragments // at the "top-level" of the set. @@ -282,12 +280,15 @@ pub(crate) fn remove_unneeded_top_level_fragment_directives( let final_selection = inline_fragment.with_updated_selection_set(updated_selections); selection_map.insert(Selection::InlineFragment(Arc::new(final_selection))); + } else { + // We can skip some of the fragment directives directive. + let final_selection = inline_fragment + .with_updated_directives_and_selection_set( + DirectiveList::from_iter(needed_directives), + updated_selections, + ); + selection_map.insert(Selection::InlineFragment(Arc::new(final_selection))); } - - // We can skip some of the fragment directives directive. - let final_selection = inline_fragment - .with_updated_directives(DirectiveList::from_iter(needed_directives)); - selection_map.insert(Selection::InlineFragment(Arc::new(final_selection))); } } _ => { diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index 456dd9b03b..de9a3a87c3 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -2759,6 +2759,7 @@ fn operation_for_entities_fetch( sibling_typename: None, })), Some(selection_set), + None, )?; let type_position: CompositeTypeDefinitionPosition = subgraph_schema @@ -2870,7 +2871,8 @@ impl FetchSelectionSet { path_in_node: &OpPath, selection_set: Option<&Arc>, ) -> Result<(), FederationError> { - Arc::make_mut(&mut self.selection_set).add_at_path(path_in_node, selection_set)?; + let target = Arc::make_mut(&mut self.selection_set); + target.add_at_path(path_in_node, selection_set)?; // TODO: when calling this multiple times, maybe only re-compute conditions at the end? // Or make it lazily-initialized and computed on demand? self.conditions = self.selection_set.conditions()?; diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs index b981592f68..7e5ad05772 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs @@ -1,13 +1,11 @@ #[test] -#[should_panic(expected = "snapshot assertion")] -// TODO: investigate this failure (redundant inline spread) fn it_handles_a_simple_at_requires_triggered_within_a_conditional() { let planner = planner!( Subgraph1: r#" type Query { t: T } - + type T @key(fields: "id") { id: ID! a: Int @@ -73,7 +71,7 @@ fn it_handles_an_at_requires_triggered_conditionally() { type Query { t: T } - + type T @key(fields: "id") { id: ID! a: Int @@ -143,7 +141,7 @@ fn it_handles_an_at_requires_where_multiple_conditional_are_involved() { type Query { a: A } - + type A @key(fields: "idA") { idA: ID! } @@ -153,7 +151,7 @@ fn it_handles_an_at_requires_where_multiple_conditional_are_involved() { idA: ID! b: [B] } - + type B @key(fields: "idB") { idB: ID! required: Int diff --git a/apollo-federation/tests/query_plan/supergraphs/it_handles_a_simple_at_requires_triggered_within_a_conditional.graphql b/apollo-federation/tests/query_plan/supergraphs/it_handles_a_simple_at_requires_triggered_within_a_conditional.graphql index 60ae71d098..49e3180b5b 100644 --- a/apollo-federation/tests/query_plan/supergraphs/it_handles_a_simple_at_requires_triggered_within_a_conditional.graphql +++ b/apollo-federation/tests/query_plan/supergraphs/it_handles_a_simple_at_requires_triggered_within_a_conditional.graphql @@ -1,4 +1,4 @@ -# Composed from subgraphs with hash: d6db359dab5222fd4d4da957d4ece13e5dee392f +# Composed from subgraphs with hash: 0e4eac66a889724333f86a54a0647cc4f694f511 schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) diff --git a/apollo-federation/tests/query_plan/supergraphs/it_handles_an_at_requires_triggered_conditionally.graphql b/apollo-federation/tests/query_plan/supergraphs/it_handles_an_at_requires_triggered_conditionally.graphql index 60ae71d098..49e3180b5b 100644 --- a/apollo-federation/tests/query_plan/supergraphs/it_handles_an_at_requires_triggered_conditionally.graphql +++ b/apollo-federation/tests/query_plan/supergraphs/it_handles_an_at_requires_triggered_conditionally.graphql @@ -1,4 +1,4 @@ -# Composed from subgraphs with hash: d6db359dab5222fd4d4da957d4ece13e5dee392f +# Composed from subgraphs with hash: 0e4eac66a889724333f86a54a0647cc4f694f511 schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) diff --git a/apollo-federation/tests/query_plan/supergraphs/it_handles_an_at_requires_where_multiple_conditional_are_involved.graphql b/apollo-federation/tests/query_plan/supergraphs/it_handles_an_at_requires_where_multiple_conditional_are_involved.graphql index 0760e3ad79..020d3b8da0 100644 --- a/apollo-federation/tests/query_plan/supergraphs/it_handles_an_at_requires_where_multiple_conditional_are_involved.graphql +++ b/apollo-federation/tests/query_plan/supergraphs/it_handles_an_at_requires_where_multiple_conditional_are_involved.graphql @@ -1,4 +1,4 @@ -# Composed from subgraphs with hash: cc5702c4822eb337c8ae95605bcb51812750cff8 +# Composed from subgraphs with hash: 2ea9dba0e4a9f205821228485cdd112ca0b0c17a schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION)