From 870f6c5c494e10598fda1da539b9d6c45bb6350b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 25 Sep 2024 12:51:57 +0200 Subject: [PATCH 01/17] Move selection map to a file --- apollo-federation/src/operation/mod.rs | 426 +---------------- .../src/operation/selection_map.rs | 436 ++++++++++++++++++ 2 files changed, 440 insertions(+), 422 deletions(-) create mode 100644 apollo-federation/src/operation/selection_map.rs diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 2eaacfd4e4..e12afe1cb2 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -259,434 +259,16 @@ impl PartialEq for SelectionSet { impl Eq for SelectionSet {} -mod selection_map { - use std::borrow::Cow; - use std::iter::Map; - use std::ops::Deref; - use std::sync::Arc; - - use apollo_compiler::collections::IndexMap; - use serde::Serialize; - - use crate::error::FederationError; - use crate::error::SingleFederationError::Internal; - use crate::operation::field_selection::FieldSelection; - use crate::operation::fragment_spread_selection::FragmentSpreadSelection; - use crate::operation::inline_fragment_selection::InlineFragmentSelection; - use crate::operation::HasSelectionKey; - use crate::operation::Selection; - use crate::operation::SelectionKey; - use crate::operation::SelectionSet; - use crate::operation::SiblingTypename; - - /// A "normalized" selection map is an optimized representation of a selection set which does - /// not contain selections with the same selection "key". Selections that do have the same key - /// are merged during the normalization process. By storing a selection set as a map, we can - /// efficiently merge/join multiple selection sets. - /// - /// Because the key depends strictly on the value, we expose the underlying map's API in a - /// read-only capacity, while mutations use an API closer to `IndexSet`. We don't just use an - /// `IndexSet` since key computation is expensive (it involves sorting). This type is in its own - /// module to prevent code from accidentally mutating the underlying map outside the mutation - /// API. - #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize)] - pub(crate) struct SelectionMap(IndexMap); - - impl Deref for SelectionMap { - type Target = IndexMap; - - fn deref(&self) -> &Self::Target { - &self.0 - } - } - - impl SelectionMap { - pub(crate) fn new() -> Self { - SelectionMap(IndexMap::default()) - } - - #[cfg(test)] - pub(crate) fn clear(&mut self) { - self.0.clear(); - } - - pub(crate) fn insert(&mut self, value: Selection) -> Option { - self.0.insert(value.key(), value) - } - - /// Remove a selection from the map. Returns the selection and its numeric index. - pub(crate) fn remove(&mut self, key: &SelectionKey) -> Option<(usize, Selection)> { - // We specifically use shift_remove() instead of swap_remove() to maintain order. - self.0 - .shift_remove_full(key) - .map(|(index, _key, selection)| (index, selection)) - } - - pub(crate) fn retain( - &mut self, - mut predicate: impl FnMut(&SelectionKey, &Selection) -> bool, - ) { - self.0.retain(|k, v| predicate(k, v)) - } - - pub(crate) fn get_mut(&mut self, key: &SelectionKey) -> Option { - self.0.get_mut(key).map(SelectionValue::new) - } - - pub(crate) fn iter_mut(&mut self) -> IterMut { - self.0.iter_mut().map(|(k, v)| (k, SelectionValue::new(v))) - } - - pub(super) fn entry(&mut self, key: SelectionKey) -> Entry { - match self.0.entry(key) { - indexmap::map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry(entry)), - indexmap::map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry(entry)), - } - } - - pub(crate) fn extend(&mut self, other: SelectionMap) { - self.0.extend(other.0) - } - - pub(crate) fn extend_ref(&mut self, other: &SelectionMap) { - self.0 - .extend(other.iter().map(|(k, v)| (k.clone(), v.clone()))) - } - - /// Returns the selection set resulting from "recursively" filtering any selection - /// that does not match the provided predicate. - /// This method calls `predicate` on every selection of the selection set, - /// not just top-level ones, and apply a "depth-first" strategy: - /// when the predicate is called on a given selection it is guaranteed that - /// filtering has happened on all the selections of its sub-selection. - pub(crate) fn filter_recursive_depth_first( - &self, - predicate: &mut dyn FnMut(&Selection) -> Result, - ) -> Result, FederationError> { - fn recur_sub_selections<'sel>( - selection: &'sel Selection, - predicate: &mut dyn FnMut(&Selection) -> Result, - ) -> Result, FederationError> { - Ok(match selection { - Selection::Field(field) => { - if let Some(sub_selections) = &field.selection_set { - match sub_selections.filter_recursive_depth_first(predicate)? { - Cow::Borrowed(_) => Cow::Borrowed(selection), - Cow::Owned(new) => Cow::Owned(Selection::from_field( - field.field.clone(), - Some(new), - )), - } - } else { - Cow::Borrowed(selection) - } - } - Selection::InlineFragment(fragment) => match fragment - .selection_set - .filter_recursive_depth_first(predicate)? - { - Cow::Borrowed(_) => Cow::Borrowed(selection), - Cow::Owned(selection_set) => Cow::Owned(Selection::InlineFragment( - Arc::new(InlineFragmentSelection::new( - fragment.inline_fragment.clone(), - selection_set, - )), - )), - }, - Selection::FragmentSpread(_) => { - return Err(FederationError::internal("unexpected fragment spread")) - } - }) - } - let mut iter = self.0.iter(); - let mut enumerated = (&mut iter).enumerate(); - let mut new_map: IndexMap<_, _>; - loop { - let Some((index, (key, selection))) = enumerated.next() else { - return Ok(Cow::Borrowed(self)); - }; - let filtered = recur_sub_selections(selection, predicate)?; - let keep = predicate(&filtered)?; - if keep && matches!(filtered, Cow::Borrowed(_)) { - // Nothing changed so far, continue without cloning - continue; - } - - // Clone the map so far - new_map = self.0.as_slice()[..index] - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(); - - if keep { - new_map.insert(key.clone(), filtered.into_owned()); - } - break; - } - for (key, selection) in iter { - let filtered = recur_sub_selections(selection, predicate)?; - if predicate(&filtered)? { - new_map.insert(key.clone(), filtered.into_owned()); - } - } - Ok(Cow::Owned(Self(new_map))) - } - } - - impl FromIterator for SelectionMap - where - A: Into, - { - fn from_iter>(iter: T) -> Self { - let mut map = Self::new(); - for selection in iter { - map.insert(selection.into()); - } - map - } - } - - type IterMut<'a> = Map< - indexmap::map::IterMut<'a, SelectionKey, Selection>, - fn((&'a SelectionKey, &'a mut Selection)) -> (&'a SelectionKey, SelectionValue<'a>), - >; - - /// A mutable reference to a `Selection` value in a `SelectionMap`, which - /// also disallows changing key-related data (to maintain the invariant that a value's key is - /// the same as it's map entry's key). - #[derive(Debug)] - pub(crate) enum SelectionValue<'a> { - Field(FieldSelectionValue<'a>), - FragmentSpread(FragmentSpreadSelectionValue<'a>), - InlineFragment(InlineFragmentSelectionValue<'a>), - } - - impl<'a> SelectionValue<'a> { - pub(crate) fn new(selection: &'a mut Selection) -> Self { - match selection { - Selection::Field(field_selection) => { - SelectionValue::Field(FieldSelectionValue::new(field_selection)) - } - Selection::FragmentSpread(fragment_spread_selection) => { - SelectionValue::FragmentSpread(FragmentSpreadSelectionValue::new( - fragment_spread_selection, - )) - } - Selection::InlineFragment(inline_fragment_selection) => { - SelectionValue::InlineFragment(InlineFragmentSelectionValue::new( - inline_fragment_selection, - )) - } - } - } - - #[cfg(test)] - pub(super) fn get_selection_set_mut(&mut self) -> Option<&mut SelectionSet> { - match self { - Self::Field(field) => field.get_selection_set_mut().as_mut(), - Self::FragmentSpread(spread) => Some(spread.get_selection_set_mut()), - Self::InlineFragment(inline) => Some(inline.get_selection_set_mut()), - } - } - } - - #[derive(Debug)] - pub(crate) struct FieldSelectionValue<'a>(&'a mut Arc); - - impl<'a> FieldSelectionValue<'a> { - pub(crate) fn new(field_selection: &'a mut Arc) -> Self { - Self(field_selection) - } - - pub(crate) fn get(&self) -> &Arc { - self.0 - } - - pub(crate) fn get_sibling_typename_mut(&mut self) -> &mut Option { - Arc::make_mut(self.0).field.sibling_typename_mut() - } - - pub(crate) fn get_selection_set_mut(&mut self) -> &mut Option { - &mut Arc::make_mut(self.0).selection_set - } - } - - #[derive(Debug)] - pub(crate) struct FragmentSpreadSelectionValue<'a>(&'a mut Arc); - - impl<'a> FragmentSpreadSelectionValue<'a> { - pub(crate) fn new(fragment_spread_selection: &'a mut Arc) -> Self { - Self(fragment_spread_selection) - } - - #[cfg(test)] - pub(crate) fn get_selection_set_mut(&mut self) -> &mut SelectionSet { - &mut Arc::make_mut(self.0).selection_set - } - - pub(crate) fn get(&self) -> &Arc { - self.0 - } - } - - #[derive(Debug)] - pub(crate) struct InlineFragmentSelectionValue<'a>(&'a mut Arc); - - impl<'a> InlineFragmentSelectionValue<'a> { - pub(crate) fn new(inline_fragment_selection: &'a mut Arc) -> Self { - Self(inline_fragment_selection) - } - - pub(crate) fn get(&self) -> &Arc { - self.0 - } - - pub(crate) fn get_selection_set_mut(&mut self) -> &mut SelectionSet { - &mut Arc::make_mut(self.0).selection_set - } - } - - pub(crate) enum Entry<'a> { - Occupied(OccupiedEntry<'a>), - Vacant(VacantEntry<'a>), - } - - impl<'a> Entry<'a> { - pub(crate) fn or_insert( - self, - produce: impl FnOnce() -> Result, - ) -> Result, FederationError> { - match self { - Self::Occupied(entry) => Ok(entry.into_mut()), - Self::Vacant(entry) => entry.insert(produce()?), - } - } - } - - pub(crate) struct OccupiedEntry<'a>(indexmap::map::OccupiedEntry<'a, SelectionKey, Selection>); - - impl<'a> OccupiedEntry<'a> { - pub(crate) fn get(&self) -> &Selection { - self.0.get() - } - - pub(crate) fn into_mut(self) -> SelectionValue<'a> { - SelectionValue::new(self.0.into_mut()) - } - } - - pub(crate) struct VacantEntry<'a>(indexmap::map::VacantEntry<'a, SelectionKey, Selection>); - - impl<'a> VacantEntry<'a> { - pub(crate) fn key(&self) -> &SelectionKey { - self.0.key() - } - - pub(crate) fn insert( - self, - value: Selection, - ) -> Result, FederationError> { - if *self.key() != value.key() { - return Err(Internal { - message: format!( - "Key mismatch when inserting selection {} into vacant entry ", - value - ), - } - .into()); - } - Ok(SelectionValue::new(self.0.insert(value))) - } - } - - impl IntoIterator for SelectionMap { - type Item = as IntoIterator>::Item; - type IntoIter = as IntoIterator>::IntoIter; - - fn into_iter(self) -> Self::IntoIter { - as IntoIterator>::into_iter(self.0) - } - } - - impl<'a> IntoIterator for &'a SelectionMap { - type Item = <&'a IndexMap as IntoIterator>::Item; - type IntoIter = <&'a IndexMap as IntoIterator>::IntoIter; - - fn into_iter(self) -> Self::IntoIter { - self.0.iter() - } - } -} +mod selection_map; pub(crate) use selection_map::FieldSelectionValue; pub(crate) use selection_map::FragmentSpreadSelectionValue; +pub(crate) use selection_map::HasSelectionKey; pub(crate) use selection_map::InlineFragmentSelectionValue; +pub(crate) use selection_map::SelectionKey; pub(crate) use selection_map::SelectionMap; pub(crate) use selection_map::SelectionValue; -/// A selection "key" (unrelated to the federation `@key` directive) is an identifier of a selection -/// (field, inline fragment, or fragment spread) that is used to determine whether two selections -/// can be merged. -/// -/// In order to merge two selections they need to -/// * reference the same field/inline fragment -/// * specify the same directives -/// * directives have to be applied in the same order -/// * directive arguments order does not matter (they get automatically sorted by their names). -/// * selection cannot specify @defer directive -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] -pub(crate) enum SelectionKey { - Field { - /// The field alias (if specified) or field name in the resulting selection set. - response_name: Name, - /// directives applied on the field - #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] - directives: DirectiveList, - }, - FragmentSpread { - /// The name of the fragment. - fragment_name: Name, - /// Directives applied on the fragment spread (does not contain @defer). - #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] - directives: DirectiveList, - }, - InlineFragment { - /// The optional type condition of the fragment. - type_condition: Option, - /// Directives applied on the fragment spread (does not contain @defer). - #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] - directives: DirectiveList, - }, - Defer { - /// Unique selection ID used to distinguish deferred fragment spreads that cannot be merged. - #[cfg_attr(not(feature = "snapshot_tracing"), serde(skip))] - deferred_id: SelectionId, - }, -} - -impl SelectionKey { - /// Returns true if the selection key is `__typename` *without directives*. - pub(crate) fn is_typename_field(&self) -> bool { - matches!(self, SelectionKey::Field { response_name, directives } if *response_name == TYPENAME_FIELD && directives.is_empty()) - } - - /// Create a selection key for a specific field name. - /// - /// This is available for tests only as selection keys should not normally be created outside of - /// `HasSelectionKey::key`. - #[cfg(test)] - pub(crate) fn field_name(name: &str) -> Self { - SelectionKey::Field { - response_name: Name::new(name).unwrap(), - directives: Default::default(), - } - } -} - -pub(crate) trait HasSelectionKey { - fn key(&self) -> SelectionKey; -} - /// An analogue of the apollo-compiler type `Selection` that stores our other selection analogues /// instead of the apollo-compiler types. #[derive(Debug, Clone, PartialEq, Eq, derive_more::IsVariant, Serialize)] @@ -2844,7 +2426,7 @@ impl<'a> IntoIterator for &'a SelectionSet { type IntoIter = <&'a IndexMap as IntoIterator>::IntoIter; fn into_iter(self) -> Self::IntoIter { - self.selections.as_ref().into_iter() + self.selections.as_ref().iter() } } diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs new file mode 100644 index 0000000000..19948febf8 --- /dev/null +++ b/apollo-federation/src/operation/selection_map.rs @@ -0,0 +1,436 @@ +use std::borrow::Cow; +use std::iter::Map; +use std::ops::Deref; +use std::sync::Arc; + +use apollo_compiler::collections::IndexMap; +use apollo_compiler::Name; +use serde::Serialize; + +use crate::error::FederationError; +use crate::error::SingleFederationError::Internal; +use crate::operation::field_selection::FieldSelection; +use crate::operation::fragment_spread_selection::FragmentSpreadSelection; +use crate::operation::inline_fragment_selection::InlineFragmentSelection; +use crate::operation::DirectiveList; +use crate::operation::Selection; +use crate::operation::SelectionId; +use crate::operation::SelectionSet; +use crate::operation::SiblingTypename; + +/// A selection "key" (unrelated to the federation `@key` directive) is an identifier of a selection +/// (field, inline fragment, or fragment spread) that is used to determine whether two selections +/// can be merged. +/// +/// In order to merge two selections they need to +/// * reference the same field/inline fragment +/// * specify the same directives +/// * directives have to be applied in the same order +/// * directive arguments order does not matter (they get automatically sorted by their names). +/// * selection cannot specify @defer directive +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] +pub(crate) enum SelectionKey { + Field { + /// The field alias (if specified) or field name in the resulting selection set. + response_name: Name, + /// directives applied on the field + #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] + directives: DirectiveList, + }, + FragmentSpread { + /// The name of the fragment. + fragment_name: Name, + /// Directives applied on the fragment spread (does not contain @defer). + #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] + directives: DirectiveList, + }, + InlineFragment { + /// The optional type condition of the fragment. + type_condition: Option, + /// Directives applied on the fragment spread (does not contain @defer). + #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] + directives: DirectiveList, + }, + Defer { + /// Unique selection ID used to distinguish deferred fragment spreads that cannot be merged. + #[cfg_attr(not(feature = "snapshot_tracing"), serde(skip))] + deferred_id: SelectionId, + }, +} + +impl SelectionKey { + /// Returns true if the selection key is `__typename` *without directives*. + #[deprecated = "Use the Selection type instead"] + pub(crate) fn is_typename_field(&self) -> bool { + matches!(self, SelectionKey::Field { response_name, directives } if *response_name == super::TYPENAME_FIELD && directives.is_empty()) + } + + /// Create a selection key for a specific field name. + /// + /// This is available for tests only as selection keys should not normally be created outside of + /// `HasSelectionKey::key`. + #[cfg(test)] + pub(crate) fn field_name(name: &str) -> Self { + SelectionKey::Field { + response_name: Name::new(name).unwrap(), + directives: Default::default(), + } + } +} + +pub(crate) trait HasSelectionKey { + fn key(&self) -> SelectionKey; +} + +/// A "normalized" selection map is an optimized representation of a selection set which does +/// not contain selections with the same selection "key". Selections that do have the same key +/// are merged during the normalization process. By storing a selection set as a map, we can +/// efficiently merge/join multiple selection sets. +/// +/// Because the key depends strictly on the value, we expose the underlying map's API in a +/// read-only capacity, while mutations use an API closer to `IndexSet`. We don't just use an +/// `IndexSet` since key computation is expensive (it involves sorting). This type is in its own +/// module to prevent code from accidentally mutating the underlying map outside the mutation +/// API. +#[derive(Debug, Clone, PartialEq, Eq, Default, Serialize)] +pub(crate) struct SelectionMap(IndexMap); + +impl Deref for SelectionMap { + type Target = IndexMap; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl SelectionMap { + pub(crate) fn new() -> Self { + SelectionMap(IndexMap::default()) + } + + pub(crate) fn clear(&mut self) { + self.0.clear(); + } + + pub(crate) fn insert(&mut self, value: Selection) -> Option { + self.0.insert(value.key(), value) + } + + /// Insert a selection at a specific index. + pub(crate) fn insert_at(&mut self, index: usize, value: Selection) -> Option { + self.0.shift_insert(index, value.key(), value) + } + + /// Remove a selection from the map. Returns the selection and its numeric index. + pub(crate) fn remove(&mut self, key: &SelectionKey) -> Option<(usize, Selection)> { + // We specifically use shift_remove() instead of swap_remove() to maintain order. + self.0 + .shift_remove_full(key) + .map(|(index, _key, selection)| (index, selection)) + } + + pub(crate) fn retain( + &mut self, + mut predicate: impl FnMut(&SelectionKey, &Selection) -> bool, + ) { + self.0.retain(|k, v| predicate(k, v)) + } + + pub(crate) fn get_mut(&mut self, key: &SelectionKey) -> Option { + self.0.get_mut(key).map(SelectionValue::new) + } + + pub(crate) fn iter_mut(&mut self) -> IterMut { + self.0.iter_mut().map(|(k, v)| (k, SelectionValue::new(v))) + } + + pub(super) fn entry(&mut self, key: SelectionKey) -> Entry { + match self.0.entry(key) { + indexmap::map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry(entry)), + indexmap::map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry(entry)), + } + } + + pub(crate) fn extend(&mut self, other: SelectionMap) { + self.0.extend(other.0) + } + + pub(crate) fn extend_ref(&mut self, other: &SelectionMap) { + self.0 + .extend(other.iter().map(|(k, v)| (k.clone(), v.clone()))) + } + + /// Returns the selection set resulting from "recursively" filtering any selection + /// that does not match the provided predicate. + /// This method calls `predicate` on every selection of the selection set, + /// not just top-level ones, and apply a "depth-first" strategy: + /// when the predicate is called on a given selection it is guaranteed that + /// filtering has happened on all the selections of its sub-selection. + pub(crate) fn filter_recursive_depth_first( + &self, + predicate: &mut dyn FnMut(&Selection) -> Result, + ) -> Result, FederationError> { + fn recur_sub_selections<'sel>( + selection: &'sel Selection, + predicate: &mut dyn FnMut(&Selection) -> Result, + ) -> Result, FederationError> { + Ok(match selection { + Selection::Field(field) => { + if let Some(sub_selections) = &field.selection_set { + match sub_selections.filter_recursive_depth_first(predicate)? { + Cow::Borrowed(_) => Cow::Borrowed(selection), + Cow::Owned(new) => Cow::Owned(Selection::from_field( + field.field.clone(), + Some(new), + )), + } + } else { + Cow::Borrowed(selection) + } + } + Selection::InlineFragment(fragment) => match fragment + .selection_set + .filter_recursive_depth_first(predicate)? + { + Cow::Borrowed(_) => Cow::Borrowed(selection), + Cow::Owned(selection_set) => Cow::Owned(Selection::InlineFragment( + Arc::new(InlineFragmentSelection::new( + fragment.inline_fragment.clone(), + selection_set, + )), + )), + }, + Selection::FragmentSpread(_) => { + return Err(FederationError::internal("unexpected fragment spread")) + } + }) + } + let mut iter = self.0.iter(); + let mut enumerated = (&mut iter).enumerate(); + let mut new_map: IndexMap<_, _>; + loop { + let Some((index, (key, selection))) = enumerated.next() else { + return Ok(Cow::Borrowed(self)); + }; + let filtered = recur_sub_selections(selection, predicate)?; + let keep = predicate(&filtered)?; + if keep && matches!(filtered, Cow::Borrowed(_)) { + // Nothing changed so far, continue without cloning + continue; + } + + // Clone the map so far + new_map = self.0.as_slice()[..index] + .iter() + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); + + if keep { + new_map.insert(key.clone(), filtered.into_owned()); + } + break; + } + for (key, selection) in iter { + let filtered = recur_sub_selections(selection, predicate)?; + if predicate(&filtered)? { + new_map.insert(key.clone(), filtered.into_owned()); + } + } + Ok(Cow::Owned(Self(new_map))) + } +} + +impl FromIterator for SelectionMap +where + A: Into, +{ + fn from_iter>(iter: T) -> Self { + let mut map = Self::new(); + for selection in iter { + map.insert(selection.into()); + } + map + } +} + +type IterMut<'a> = Map< + indexmap::map::IterMut<'a, SelectionKey, Selection>, + fn((&'a SelectionKey, &'a mut Selection)) -> (&'a SelectionKey, SelectionValue<'a>), +>; + +/// A mutable reference to a `Selection` value in a `SelectionMap`, which +/// also disallows changing key-related data (to maintain the invariant that a value's key is +/// the same as it's map entry's key). +#[derive(Debug)] +pub(crate) enum SelectionValue<'a> { + Field(FieldSelectionValue<'a>), + FragmentSpread(FragmentSpreadSelectionValue<'a>), + InlineFragment(InlineFragmentSelectionValue<'a>), +} + +impl<'a> SelectionValue<'a> { + pub(crate) fn new(selection: &'a mut Selection) -> Self { + match selection { + Selection::Field(field_selection) => { + SelectionValue::Field(FieldSelectionValue::new(field_selection)) + } + Selection::FragmentSpread(fragment_spread_selection) => { + SelectionValue::FragmentSpread(FragmentSpreadSelectionValue::new( + fragment_spread_selection, + )) + } + Selection::InlineFragment(inline_fragment_selection) => { + SelectionValue::InlineFragment(InlineFragmentSelectionValue::new( + inline_fragment_selection, + )) + } + } + } + + pub(super) fn directives(&self) -> &'_ DirectiveList { + match self { + Self::Field(field) => &field.get().field.directives, + Self::FragmentSpread(frag) => &frag.get().spread.directives, + Self::InlineFragment(frag) => &frag.get().inline_fragment.directives, + } + } + + pub(super) fn get_selection_set_mut(&mut self) -> Option<&mut SelectionSet> { + match self { + Self::Field(field) => field.get_selection_set_mut().as_mut(), + Self::FragmentSpread(spread) => Some(spread.get_selection_set_mut()), + Self::InlineFragment(inline) => Some(inline.get_selection_set_mut()), + } + } +} + +#[derive(Debug)] +pub(crate) struct FieldSelectionValue<'a>(&'a mut Arc); + +impl<'a> FieldSelectionValue<'a> { + pub(crate) fn new(field_selection: &'a mut Arc) -> Self { + Self(field_selection) + } + + pub(crate) fn get(&self) -> &Arc { + self.0 + } + + pub(crate) fn get_sibling_typename_mut(&mut self) -> &mut Option { + Arc::make_mut(self.0).field.sibling_typename_mut() + } + + pub(crate) fn get_selection_set_mut(&mut self) -> &mut Option { + &mut Arc::make_mut(self.0).selection_set + } +} + +#[derive(Debug)] +pub(crate) struct FragmentSpreadSelectionValue<'a>(&'a mut Arc); + +impl<'a> FragmentSpreadSelectionValue<'a> { + pub(crate) fn new(fragment_spread_selection: &'a mut Arc) -> Self { + Self(fragment_spread_selection) + } + + pub(crate) fn get_selection_set_mut(&mut self) -> &mut SelectionSet { + &mut Arc::make_mut(self.0).selection_set + } + + pub(crate) fn get(&self) -> &Arc { + self.0 + } +} + +#[derive(Debug)] +pub(crate) struct InlineFragmentSelectionValue<'a>(&'a mut Arc); + +impl<'a> InlineFragmentSelectionValue<'a> { + pub(crate) fn new(inline_fragment_selection: &'a mut Arc) -> Self { + Self(inline_fragment_selection) + } + + pub(crate) fn get(&self) -> &Arc { + self.0 + } + + pub(crate) fn get_selection_set_mut(&mut self) -> &mut SelectionSet { + &mut Arc::make_mut(self.0).selection_set + } +} + +pub(crate) enum Entry<'a> { + Occupied(OccupiedEntry<'a>), + Vacant(VacantEntry<'a>), +} + +impl<'a> Entry<'a> { + pub fn or_insert( + self, + produce: impl FnOnce() -> Result, + ) -> Result, FederationError> { + match self { + Self::Occupied(entry) => Ok(entry.into_mut()), + Self::Vacant(entry) => entry.insert(produce()?), + } + } +} + +pub(crate) struct OccupiedEntry<'a>(indexmap::map::OccupiedEntry<'a, SelectionKey, Selection>); + +impl<'a> OccupiedEntry<'a> { + pub(crate) fn get(&self) -> &Selection { + self.0.get() + } + + pub(crate) fn get_mut(&mut self) -> SelectionValue { + SelectionValue::new(self.0.get_mut()) + } + + pub(crate) fn into_mut(self) -> SelectionValue<'a> { + SelectionValue::new(self.0.into_mut()) + } + + pub(crate) fn key(&self) -> &SelectionKey { + self.0.key() + } + + pub(crate) fn remove(self) -> Selection { + // We specifically use shift_remove() instead of swap_remove() to maintain order. + self.0.shift_remove() + } +} + +pub(crate) struct VacantEntry<'a>(indexmap::map::VacantEntry<'a, SelectionKey, Selection>); + +impl<'a> VacantEntry<'a> { + pub(crate) fn key(&self) -> &SelectionKey { + self.0.key() + } + + pub(crate) fn insert( + self, + value: Selection, + ) -> Result, FederationError> { + if *self.key() != value.key() { + return Err(Internal { + message: format!( + "Key mismatch when inserting selection {} into vacant entry ", + value + ), + } + .into()); + } + Ok(SelectionValue::new(self.0.insert(value))) + } +} + +impl IntoIterator for SelectionMap { + type Item = as IntoIterator>::Item; + type IntoIter = as IntoIterator>::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + as IntoIterator>::into_iter(self.0) + } +} + From 65fdbc1f8f4c0d1c3335a80c449fc44b213b9fc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 25 Sep 2024 13:01:42 +0200 Subject: [PATCH 02/17] Slim down the SelectionMap API --- apollo-federation/src/operation/mod.rs | 3 +- apollo-federation/src/operation/optimize.rs | 2 +- .../src/operation/selection_map.rs | 93 +++++++------------ 3 files changed, 35 insertions(+), 63 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index e12afe1cb2..1848047f6e 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -3056,8 +3056,7 @@ impl NamedFragments { } if selection_set.selections.len() == 1 { // true if NOT field selection OR non-leaf field - return if let Some((_, Selection::Field(field_selection))) = - selection_set.selections.first() + return if let Some(Selection::Field(field_selection)) = selection_set.selections.first() { field_selection.selection_set.is_some() } else { diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index bde0b6dfa9..76172961ee 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -3507,7 +3507,7 @@ mod tests { match path.split_first() { None => { // Base case - Arc::make_mut(&mut ss.selections).clear(); + ss.selections = Default::default(); Ok(()) } diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index 19948febf8..77ba7d2218 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -1,6 +1,6 @@ +#![warn(dead_code)] use std::borrow::Cow; use std::iter::Map; -use std::ops::Deref; use std::sync::Arc; use apollo_compiler::collections::IndexMap; @@ -95,32 +95,15 @@ pub(crate) trait HasSelectionKey { #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize)] pub(crate) struct SelectionMap(IndexMap); -impl Deref for SelectionMap { - type Target = IndexMap; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - impl SelectionMap { pub(crate) fn new() -> Self { SelectionMap(IndexMap::default()) } - pub(crate) fn clear(&mut self) { - self.0.clear(); - } - pub(crate) fn insert(&mut self, value: Selection) -> Option { self.0.insert(value.key(), value) } - /// Insert a selection at a specific index. - pub(crate) fn insert_at(&mut self, index: usize, value: Selection) -> Option { - self.0.shift_insert(index, value.key(), value) - } - /// Remove a selection from the map. Returns the selection and its numeric index. pub(crate) fn remove(&mut self, key: &SelectionKey) -> Option<(usize, Selection)> { // We specifically use shift_remove() instead of swap_remove() to maintain order. @@ -129,21 +112,29 @@ impl SelectionMap { .map(|(index, _key, selection)| (index, selection)) } - pub(crate) fn retain( - &mut self, - mut predicate: impl FnMut(&SelectionKey, &Selection) -> bool, - ) { + pub(crate) fn retain(&mut self, mut predicate: impl FnMut(&SelectionKey, &Selection) -> bool) { self.0.retain(|k, v| predicate(k, v)) } - pub(crate) fn get_mut(&mut self, key: &SelectionKey) -> Option { - self.0.get_mut(key).map(SelectionValue::new) + /// Iterate over all selections and selection keys. + pub(crate) fn iter(&self) -> indexmap::map::Iter<'_, SelectionKey, Selection> { + self.0.iter() } pub(crate) fn iter_mut(&mut self) -> IterMut { self.0.iter_mut().map(|(k, v)| (k, SelectionValue::new(v))) } + /// Iterate over all selections. + pub(crate) fn values(&self) -> indexmap::map::Values<'_, SelectionKey, Selection> { + self.0.values() + } + + /// Iterate over all selections. + pub(crate) fn into_values(self) -> indexmap::map::IntoValues { + self.0.into_values() + } + pub(super) fn entry(&mut self, key: SelectionKey) -> Entry { match self.0.entry(key) { indexmap::map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry(entry)), @@ -160,6 +151,10 @@ impl SelectionMap { .extend(other.iter().map(|(k, v)| (k.clone(), v.clone()))) } + pub(crate) fn as_slice(&self) -> &indexmap::map::Slice { + self.0.as_slice() + } + /// Returns the selection set resulting from "recursively" filtering any selection /// that does not match the provided predicate. /// This method calls `predicate` on every selection of the selection set, @@ -179,10 +174,9 @@ impl SelectionMap { if let Some(sub_selections) = &field.selection_set { match sub_selections.filter_recursive_depth_first(predicate)? { Cow::Borrowed(_) => Cow::Borrowed(selection), - Cow::Owned(new) => Cow::Owned(Selection::from_field( - field.field.clone(), - Some(new), - )), + Cow::Owned(new) => { + Cow::Owned(Selection::from_field(field.field.clone(), Some(new))) + } } } else { Cow::Borrowed(selection) @@ -193,12 +187,12 @@ impl SelectionMap { .filter_recursive_depth_first(predicate)? { Cow::Borrowed(_) => Cow::Borrowed(selection), - Cow::Owned(selection_set) => Cow::Owned(Selection::InlineFragment( - Arc::new(InlineFragmentSelection::new( + Cow::Owned(selection_set) => Cow::Owned(Selection::InlineFragment(Arc::new( + InlineFragmentSelection::new( fragment.inline_fragment.clone(), selection_set, - )), - )), + ), + ))), }, Selection::FragmentSpread(_) => { return Err(FederationError::internal("unexpected fragment spread")) @@ -274,16 +268,12 @@ impl<'a> SelectionValue<'a> { Selection::Field(field_selection) => { SelectionValue::Field(FieldSelectionValue::new(field_selection)) } - Selection::FragmentSpread(fragment_spread_selection) => { - SelectionValue::FragmentSpread(FragmentSpreadSelectionValue::new( - fragment_spread_selection, - )) - } - Selection::InlineFragment(inline_fragment_selection) => { - SelectionValue::InlineFragment(InlineFragmentSelectionValue::new( - inline_fragment_selection, - )) - } + Selection::FragmentSpread(fragment_spread_selection) => SelectionValue::FragmentSpread( + FragmentSpreadSelectionValue::new(fragment_spread_selection), + ), + Selection::InlineFragment(inline_fragment_selection) => SelectionValue::InlineFragment( + InlineFragmentSelectionValue::new(inline_fragment_selection), + ), } } @@ -383,22 +373,9 @@ impl<'a> OccupiedEntry<'a> { self.0.get() } - pub(crate) fn get_mut(&mut self) -> SelectionValue { - SelectionValue::new(self.0.get_mut()) - } - pub(crate) fn into_mut(self) -> SelectionValue<'a> { SelectionValue::new(self.0.into_mut()) } - - pub(crate) fn key(&self) -> &SelectionKey { - self.0.key() - } - - pub(crate) fn remove(self) -> Selection { - // We specifically use shift_remove() instead of swap_remove() to maintain order. - self.0.shift_remove() - } } pub(crate) struct VacantEntry<'a>(indexmap::map::VacantEntry<'a, SelectionKey, Selection>); @@ -408,10 +385,7 @@ impl<'a> VacantEntry<'a> { self.0.key() } - pub(crate) fn insert( - self, - value: Selection, - ) -> Result, FederationError> { + pub(crate) fn insert(self, value: Selection) -> Result, FederationError> { if *self.key() != value.key() { return Err(Internal { message: format!( @@ -433,4 +407,3 @@ impl IntoIterator for SelectionMap { as IntoIterator>::into_iter(self.0) } } - From 42489d1239d037f0757c857901f6c2fa1d16f750 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 26 Sep 2024 15:35:11 +0200 Subject: [PATCH 03/17] Move to a SelectionMap representation that does not require owned keys --- Cargo.lock | 18 + apollo-federation/Cargo.toml | 1 + apollo-federation/src/operation/contains.rs | 2 +- apollo-federation/src/operation/merging.rs | 6 +- apollo-federation/src/operation/mod.rs | 16 +- apollo-federation/src/operation/optimize.rs | 7 +- .../src/operation/selection_map.rs | 342 ++++++++++++++---- 7 files changed, 300 insertions(+), 92 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8b422db473..ab46b641c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -183,6 +183,7 @@ dependencies = [ "apollo-compiler", "derive_more", "either", + "hashbrown 0.15.0", "hex", "indexmap 2.2.6", "insta", @@ -2577,6 +2578,12 @@ version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" +[[package]] +name = "foldhash" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f81ec6369c545a7d40e4589b5597581fa1c441fe1cce96dd1de43159910a36a2" + [[package]] name = "forbid-anonymous-operations" version = "0.1.0" @@ -3085,6 +3092,17 @@ dependencies = [ "allocator-api2", ] +[[package]] +name = "hashbrown" +version = "0.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e087f84d4f86bf4b218b927129862374b72199ae7d8657835f1e89000eea4fb" +dependencies = [ + "allocator-api2", + "equivalent", + "foldhash", +] + [[package]] name = "hdrhistogram" version = "7.5.4" diff --git a/apollo-federation/Cargo.toml b/apollo-federation/Cargo.toml index 322fc9000d..b9b1e8c58c 100644 --- a/apollo-federation/Cargo.toml +++ b/apollo-federation/Cargo.toml @@ -22,6 +22,7 @@ time = { version = "0.3.34", default-features = false, features = [ "local-offset", ] } derive_more = "0.99.17" +hashbrown = "0.15.0" indexmap = { version = "2.2.6", features = ["serde"] } itertools = "0.13.0" lazy_static = "1.4.0" diff --git a/apollo-federation/src/operation/contains.rs b/apollo-federation/src/operation/contains.rs index b734dd304d..9e89698bb3 100644 --- a/apollo-federation/src/operation/contains.rs +++ b/apollo-federation/src/operation/contains.rs @@ -171,7 +171,7 @@ impl SelectionSet { continue; } - let Some(self_selection) = self.selections.get(key) else { + let Some(self_selection) = self.selections.get(&key) else { return Containment::NotContained; }; diff --git a/apollo-federation/src/operation/merging.rs b/apollo-federation/src/operation/merging.rs index c56ff5e66e..7f20171d3b 100644 --- a/apollo-federation/src/operation/merging.rs +++ b/apollo-federation/src/operation/merging.rs @@ -240,7 +240,7 @@ impl SelectionSet { 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) { + if let Some(other_field_selections) = fields.shift_remove(&key) { self_field_selection.merge_into( other_field_selections.iter().map(|selection| &***selection), )?; @@ -248,7 +248,7 @@ impl SelectionSet { } SelectionValue::FragmentSpread(mut self_fragment_spread_selection) => { if let Some(other_fragment_spread_selections) = - fragment_spreads.shift_remove(key) + fragment_spreads.shift_remove(&key) { self_fragment_spread_selection.merge_into( other_fragment_spread_selections @@ -259,7 +259,7 @@ impl SelectionSet { } SelectionValue::InlineFragment(mut self_inline_fragment_selection) => { if let Some(other_inline_fragment_selections) = - inline_fragments.shift_remove(key) + inline_fragments.shift_remove(&key) { self_inline_fragment_selection.merge_into( other_inline_fragment_selections diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 1848047f6e..b93a595d1b 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -866,8 +866,6 @@ pub(crate) use field_selection::FieldSelection; pub(crate) use field_selection::SiblingTypename; mod fragment_spread_selection { - use std::ops::Deref; - use apollo_compiler::Name; use serde::Serialize; @@ -966,7 +964,7 @@ mod fragment_spread_selection { fn key(&self) -> SelectionKey { if is_deferred_selection(&self.directives) { SelectionKey::Defer { - deferred_id: self.selection_id.clone(), + deferred_id: self.selection_id, } } else { SelectionKey::FragmentSpread { @@ -2413,8 +2411,8 @@ impl SelectionSet { } impl IntoIterator for SelectionSet { - type Item = as IntoIterator>::Item; - type IntoIter = as IntoIterator>::IntoIter; + type Item = ::Item; + type IntoIter = ::IntoIter; fn into_iter(self) -> Self::IntoIter { Arc::unwrap_or_clone(self.selections).into_iter() @@ -2422,8 +2420,8 @@ impl IntoIterator for SelectionSet { } impl<'a> IntoIterator for &'a SelectionSet { - type Item = <&'a IndexMap as IntoIterator>::Item; - type IntoIter = <&'a IndexMap as IntoIterator>::IntoIter; + type Item = <&'a SelectionMap as IntoIterator>::Item; + type IntoIter = <&'a SelectionMap as IntoIterator>::IntoIter; fn into_iter(self) -> Self::IntoIter { self.selections.as_ref().iter() @@ -2431,11 +2429,11 @@ impl<'a> IntoIterator for &'a SelectionSet { } pub(crate) struct FieldSelectionsIter<'sel> { - stack: Vec>, + stack: Vec>, } impl<'sel> FieldSelectionsIter<'sel> { - fn new(iter: indexmap::map::Values<'sel, SelectionKey, Selection>) -> Self { + fn new(iter: selection_map::Values<'sel>) -> Self { Self { stack: vec![iter] } } } diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 76172961ee..dcbba5fb66 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -51,6 +51,7 @@ use super::Field; use super::FieldSelection; use super::Fragment; use super::FragmentSpreadSelection; +use super::HasSelectionKey; use super::InlineFragmentSelection; use super::NamedFragments; use super::Operation; @@ -270,7 +271,7 @@ impl SelectionSet { .selections .iter() .map(|(k, v)| { - if let Some(other_v) = other.selections.get(k) { + if let Some(other_v) = other.selections.get(&k) { v.minus(other_v) } else { Ok(Some(v.clone())) @@ -299,7 +300,7 @@ impl SelectionSet { .selections .iter() .map(|(k, v)| { - if let Some(other_v) = other.selections.get(k) { + if let Some(other_v) = other.selections.get(&k) { v.intersection(other_v) } else { Ok(None) @@ -684,7 +685,7 @@ impl FragmentRestrictionAtType { fn is_useless(&self) -> bool { match self.selections.selections.as_slice().split_first() { None => true, - Some((first, rest)) => rest.is_empty() && first.0.is_typename_field(), + Some((first, rest)) => rest.is_empty() && first.key().is_typename_field(), } } } diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index 77ba7d2218..f06e187c9e 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -1,10 +1,11 @@ -#![warn(dead_code)] use std::borrow::Cow; -use std::iter::Map; +use std::hash::BuildHasher; use std::sync::Arc; -use apollo_compiler::collections::IndexMap; use apollo_compiler::Name; +use hashbrown::DefaultHashBuilder; +use hashbrown::HashTable; +use serde::ser::SerializeSeq; use serde::Serialize; use crate::error::FederationError; @@ -82,77 +83,253 @@ pub(crate) trait HasSelectionKey { fn key(&self) -> SelectionKey; } -/// A "normalized" selection map is an optimized representation of a selection set which does -/// not contain selections with the same selection "key". Selections that do have the same key -/// are merged during the normalization process. By storing a selection set as a map, we can -/// efficiently merge/join multiple selection sets. +#[derive(Clone)] +struct Bucket { + index: usize, + hash: u64, +} + +/// A selection map is the underlying representation of a selection set. It contains an ordered +/// list of selections with unique selection keys. Selections with the same key should be merged +/// together by the user of this structure: the selection map API itself will overwrite selections +/// with the same key. /// /// Because the key depends strictly on the value, we expose the underlying map's API in a /// read-only capacity, while mutations use an API closer to `IndexSet`. We don't just use an /// `IndexSet` since key computation is expensive (it involves sorting). This type is in its own /// module to prevent code from accidentally mutating the underlying map outside the mutation /// API. -#[derive(Debug, Clone, PartialEq, Eq, Default, Serialize)] -pub(crate) struct SelectionMap(IndexMap); +#[derive(Clone)] +pub(crate) struct SelectionMap { + hash_builder: DefaultHashBuilder, + table: HashTable, + selections: Vec, +} + +impl std::fmt::Debug for SelectionMap { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_map().entries(self.iter()).finish() + } +} + +impl PartialEq for SelectionMap { + /// Compare two selection maps. This is order independent. + fn eq(&self, other: &Self) -> bool { + self.len() == other.len() + && self + .values() + .all(|left| other.get(&left.key()).is_some_and(|right| left == right)) + } +} + +impl Eq for SelectionMap {} + +impl Serialize for SelectionMap { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + let mut map = serializer.serialize_map(Some(self.len()))?; + for (key, value) in self.iter() { + map.serialize_entry(&key, value)?; + } + map.end() + } +} + +impl Default for SelectionMap { + fn default() -> Self { + Self::new() + } +} + +pub(crate) type Values<'a> = std::slice::Iter<'a, Selection>; +pub(crate) type ValuesMut<'a> = std::slice::IterMut<'a, Selection>; +pub(crate) type IntoValues = std::vec::IntoIter; + +/// Return an equality function taking an index into `selections` and returning if the index +/// matches the given key. +/// +/// The returned function panics if the index is out of bounds. +fn key_eq<'a>(selections: &'a [Selection], key: &SelectionKey) -> impl Fn(&Bucket) -> bool + 'a { + move |bucket| selections[bucket.index].key() == *key +} impl SelectionMap { pub(crate) fn new() -> Self { - SelectionMap(IndexMap::default()) + SelectionMap { + hash_builder: Default::default(), + table: HashTable::new(), + selections: Vec::new(), + } + } + + /// Returns the number of selections in the map. + pub(crate) fn len(&self) -> usize { + self.selections.len() + } + + /// Returns true if there are no selections in the map. + pub(crate) fn is_empty(&self) -> bool { + self.selections.is_empty() + } + + /// Returns the first selection in the map, or None if the map is empty. + pub(crate) fn first(&self) -> Option<&Selection> { + self.selections.first() + } + + /// Computes the hash of a selection key. + fn hash(&self, key: SelectionKey) -> u64 { + self.hash_builder.hash_one(key) + } + + /// Returns true if the given key exists in the map. + pub(crate) fn contains_key(&self, key: &SelectionKey) -> bool { + let hash = self.hash(key); + self.table + .find(hash, key_eq(&self.selections, key)) + .is_some() + } + + /// Returns true if the given key exists in the map. + pub(crate) fn get(&self, key: &SelectionKey) -> Option<&Selection> { + let hash = self.hash(key); + let bucket = self.table.find(hash, key_eq(&self.selections, key))?; + Some(&self.selections[bucket.index]) + } + + pub(crate) fn get_mut(&mut self, key: &SelectionKey) -> Option> { + let hash = self.hash(key); + let bucket = self.table.find_mut(hash, key_eq(&self.selections, key))?; + Some(SelectionValue::new(&mut self.selections[bucket.index])) + } + + /// Insert a selection into the map. + fn raw_insert(&mut self, hash: u64, value: Selection) -> &mut Selection { + let index = self.selections.len(); + + // `insert` overwrites a selection without running `Drop`: because + // we only store an integer which is `Copy`, this works out fine + self.table.insert_unique(hash, Bucket { index, hash }, |existing| existing.hash); + + self.selections.push(value); + &mut self.selections[index] + } + + /// Resets and rebuilds the hash table. + /// + /// Preconditions: + /// - The table must have enough capacity for `self.selections.len()` elements. + fn rebuild_table_no_grow(&mut self) { + assert!(self.table.capacity() >= self.selections.len()); + self.table.clear(); + for (index, selection) in self.selections.iter().enumerate() { + let hash = self.hash(selection.key()); + self.table.insert_unique(hash, Bucket { index, hash }, |existing| existing.hash); + } } - pub(crate) fn insert(&mut self, value: Selection) -> Option { - self.0.insert(value.key(), value) + /// Decrements all the indices in the table starting at `pivot`. + fn decrement_table(&mut self, pivot: usize) { + for bucket in self.table.iter_mut() { + if bucket.index >= pivot { + bucket.index -= 1; + } + } + } + + pub(crate) fn insert(&mut self, value: Selection) { + let hash = self.hash(&value.key()); + self.raw_insert(hash, value); } /// Remove a selection from the map. Returns the selection and its numeric index. pub(crate) fn remove(&mut self, key: &SelectionKey) -> Option<(usize, Selection)> { - // We specifically use shift_remove() instead of swap_remove() to maintain order. - self.0 - .shift_remove_full(key) - .map(|(index, _key, selection)| (index, selection)) + let hash = self.hash(key); + let entry = self + .table + .find_entry(hash, key_eq(&self.selections, key)) + .ok()?; + let (bucket, _) = entry.remove(); + let selection = self.selections.remove(bucket.index); + self.decrement_table(bucket.index); + Some((bucket.index, selection)) } pub(crate) fn retain(&mut self, mut predicate: impl FnMut(&SelectionKey, &Selection) -> bool) { - self.0.retain(|k, v| predicate(k, v)) + self.selections.retain(|selection| { + let key = selection.key(); + predicate(&key, selection) + }); + if self.selections.len() < self.table.len() { + // In theory, we could track which keys were removed, and adjust the indices based on + // that, but it's very tricky and it might not even be faster than just resetting the + // whole map. + self.rebuild_table_no_grow(); + } + assert!(self.selections.len() == self.table.len()); } /// Iterate over all selections and selection keys. - pub(crate) fn iter(&self) -> indexmap::map::Iter<'_, SelectionKey, Selection> { - self.0.iter() + #[deprecated = "Prefer values()"] + pub(crate) fn iter(&self) -> impl Iterator { + self.selections.iter().map(|v| (v.key(), v)) } - pub(crate) fn iter_mut(&mut self) -> IterMut { - self.0.iter_mut().map(|(k, v)| (k, SelectionValue::new(v))) + #[deprecated = "Prefer values_mut()"] + pub(crate) fn iter_mut(&mut self) -> impl Iterator)> { + self.selections + .iter_mut() + .map(|v| (v.key(), SelectionValue::new(v))) } /// Iterate over all selections. - pub(crate) fn values(&self) -> indexmap::map::Values<'_, SelectionKey, Selection> { - self.0.values() + pub(crate) fn values(&self) -> std::slice::Iter<'_, Selection> { + self.selections.iter() } /// Iterate over all selections. - pub(crate) fn into_values(self) -> indexmap::map::IntoValues { - self.0.into_values() + pub(crate) fn values_mut(&mut self) -> impl Iterator> { + self.selections.iter_mut().map(SelectionValue::new) } - pub(super) fn entry(&mut self, key: SelectionKey) -> Entry { - match self.0.entry(key) { - indexmap::map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry(entry)), - indexmap::map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry(entry)), + /// Iterate over all selections. + pub(crate) fn into_values(self) -> std::vec::IntoIter { + self.selections.into_iter() + } + + pub(super) fn entry<'a>(&'a mut self, key: &SelectionKey) -> Entry<'a> { + let hash = self.hash(key); + let slot = self.table.find_entry(hash, key_eq(&self.selections, key)); + match slot { + Ok(occupied) => { + let index = occupied.get().index; + let selection = &mut self.selections[index]; + Entry::Occupied(OccupiedEntry(selection)) + } + Err(vacant) => Entry::Vacant(VacantEntry { + map: self, + hash, + key, + }), } } pub(crate) fn extend(&mut self, other: SelectionMap) { - self.0.extend(other.0) + for selection in other.into_values() { + self.insert(selection); + } } pub(crate) fn extend_ref(&mut self, other: &SelectionMap) { - self.0 - .extend(other.iter().map(|(k, v)| (k.clone(), v.clone()))) + for selection in other.values() { + self.insert(selection.clone()); + } } - pub(crate) fn as_slice(&self) -> &indexmap::map::Slice { - self.0.as_slice() + pub(crate) fn as_slice(&self) -> &[Selection] { + &self.selections } /// Returns the selection set resulting from "recursively" filtering any selection @@ -199,11 +376,11 @@ impl SelectionMap { } }) } - let mut iter = self.0.iter(); + let mut iter = self.values(); let mut enumerated = (&mut iter).enumerate(); - let mut new_map: IndexMap<_, _>; + let mut new_map: Self; loop { - let Some((index, (key, selection))) = enumerated.next() else { + let Some((index, selection)) = enumerated.next() else { return Ok(Cow::Borrowed(self)); }; let filtered = recur_sub_selections(selection, predicate)?; @@ -214,23 +391,20 @@ impl SelectionMap { } // Clone the map so far - new_map = self.0.as_slice()[..index] - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(); + new_map = self.as_slice()[..index].iter().cloned().collect(); if keep { - new_map.insert(key.clone(), filtered.into_owned()); + new_map.insert(filtered.into_owned()); } break; } - for (key, selection) in iter { + for selection in iter { let filtered = recur_sub_selections(selection, predicate)?; if predicate(&filtered)? { - new_map.insert(key.clone(), filtered.into_owned()); + new_map.insert(filtered.into_owned()); } } - Ok(Cow::Owned(Self(new_map))) + Ok(Cow::Owned(new_map)) } } @@ -247,11 +421,6 @@ where } } -type IterMut<'a> = Map< - indexmap::map::IterMut<'a, SelectionKey, Selection>, - fn((&'a SelectionKey, &'a mut Selection)) -> (&'a SelectionKey, SelectionValue<'a>), ->; - /// A mutable reference to a `Selection` value in a `SelectionMap`, which /// also disallows changing key-related data (to maintain the invariant that a value's key is /// the same as it's map entry's key). @@ -263,7 +432,7 @@ pub(crate) enum SelectionValue<'a> { } impl<'a> SelectionValue<'a> { - pub(crate) fn new(selection: &'a mut Selection) -> Self { + fn new(selection: &'a mut Selection) -> Self { match selection { Selection::Field(field_selection) => { SelectionValue::Field(FieldSelectionValue::new(field_selection)) @@ -277,19 +446,19 @@ impl<'a> SelectionValue<'a> { } } - pub(super) fn directives(&self) -> &'_ DirectiveList { + pub(super) fn key(&self) -> SelectionKey { match self { - Self::Field(field) => &field.get().field.directives, - Self::FragmentSpread(frag) => &frag.get().spread.directives, - Self::InlineFragment(frag) => &frag.get().inline_fragment.directives, + Self::Field(field) => field.get().key(), + Self::FragmentSpread(frag) => frag.get().key(), + Self::InlineFragment(frag) => frag.get().key(), } } - pub(super) fn get_selection_set_mut(&mut self) -> Option<&mut SelectionSet> { + pub(super) fn get_selection_set_mut(&mut self) -> Option<&'_ mut SelectionSet> { match self { - Self::Field(field) => field.get_selection_set_mut().as_mut(), - Self::FragmentSpread(spread) => Some(spread.get_selection_set_mut()), - Self::InlineFragment(inline) => Some(inline.get_selection_set_mut()), + SelectionValue::Field(field) => field.get_selection_set_mut(), + SelectionValue::FragmentSpread(frag) => Some(frag.get_selection_set_mut()), + SelectionValue::InlineFragment(frag) => Some(frag.get_selection_set_mut()), } } } @@ -310,8 +479,8 @@ impl<'a> FieldSelectionValue<'a> { Arc::make_mut(self.0).field.sibling_typename_mut() } - pub(crate) fn get_selection_set_mut(&mut self) -> &mut Option { - &mut Arc::make_mut(self.0).selection_set + pub(crate) fn get_selection_set_mut(&mut self) -> Option<&mut SelectionSet> { + Arc::make_mut(self.0).selection_set.as_mut() } } @@ -323,13 +492,13 @@ impl<'a> FragmentSpreadSelectionValue<'a> { Self(fragment_spread_selection) } - pub(crate) fn get_selection_set_mut(&mut self) -> &mut SelectionSet { - &mut Arc::make_mut(self.0).selection_set - } - pub(crate) fn get(&self) -> &Arc { self.0 } + + pub(crate) fn get_selection_set_mut(&mut self) -> &mut SelectionSet { + &mut Arc::make_mut(self.0).selection_set + } } #[derive(Debug)] @@ -355,7 +524,7 @@ pub(crate) enum Entry<'a> { } impl<'a> Entry<'a> { - pub fn or_insert( + pub(crate) fn or_insert( self, produce: impl FnOnce() -> Result, ) -> Result, FederationError> { @@ -366,23 +535,27 @@ impl<'a> Entry<'a> { } } -pub(crate) struct OccupiedEntry<'a>(indexmap::map::OccupiedEntry<'a, SelectionKey, Selection>); +pub(crate) struct OccupiedEntry<'a>(&'a mut Selection); impl<'a> OccupiedEntry<'a> { pub(crate) fn get(&self) -> &Selection { - self.0.get() + self.0 } pub(crate) fn into_mut(self) -> SelectionValue<'a> { - SelectionValue::new(self.0.into_mut()) + SelectionValue::new(self.0) } } -pub(crate) struct VacantEntry<'a>(indexmap::map::VacantEntry<'a, SelectionKey, Selection>); +pub(crate) struct VacantEntry<'a> { + map: &'a mut SelectionMap, + hash: u64, + key: SelectionKey, +} impl<'a> VacantEntry<'a> { pub(crate) fn key(&self) -> &SelectionKey { - self.0.key() + &self.key } pub(crate) fn insert(self, value: Selection) -> Result, FederationError> { @@ -394,16 +567,33 @@ impl<'a> VacantEntry<'a> { ), } .into()); - } - Ok(SelectionValue::new(self.0.insert(value))) + }; + Ok(SelectionValue::new(self.map.raw_insert(self.hash, value))) } } impl IntoIterator for SelectionMap { - type Item = as IntoIterator>::Item; - type IntoIter = as IntoIterator>::IntoIter; + type Item = (SelectionKey, Selection); + type IntoIter = + std::iter::Map, fn(Selection) -> (SelectionKey, Selection)>; + + fn into_iter(self) -> Self::IntoIter { + self.selections + .into_iter() + .map(|selection| (selection.key(), selection)) + } +} + +impl<'a> IntoIterator for &'a SelectionMap { + type Item = (SelectionKey, &'a Selection); + type IntoIter = std::iter::Map< + std::slice::Iter<'a, Selection>, + fn(&'a Selection) -> (SelectionKey, &'a Selection), + >; fn into_iter(self) -> Self::IntoIter { - as IntoIterator>::into_iter(self.0) + self.selections + .iter() + .map(|selection| (selection.key(), selection)) } } From de5ca955b0d08ce7b719f2a688f44b72e3e29b2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 26 Sep 2024 16:56:21 +0200 Subject: [PATCH 04/17] Remove usage of (key, selection) iterators --- apollo-federation/src/operation/contains.rs | 3 +- apollo-federation/src/operation/merging.rs | 3 +- apollo-federation/src/operation/mod.rs | 44 +++++++---------- apollo-federation/src/operation/optimize.rs | 20 ++++---- apollo-federation/src/operation/rebase.rs | 4 +- .../src/operation/selection_map.rs | 49 ++----------------- apollo-federation/src/operation/simplify.rs | 6 +-- 7 files changed, 42 insertions(+), 87 deletions(-) diff --git a/apollo-federation/src/operation/contains.rs b/apollo-federation/src/operation/contains.rs index 9e89698bb3..142d4a8d1a 100644 --- a/apollo-federation/src/operation/contains.rs +++ b/apollo-federation/src/operation/contains.rs @@ -163,7 +163,8 @@ impl SelectionSet { let mut is_equal = true; let mut did_ignore_typename = false; - for (key, other_selection) in other.selections.iter() { + for other_selection in other.selections.values() { + let key = other_selection.key(); if key.is_typename_field() && options.ignore_missing_typename { if !self.has_top_level_typename_field() { did_ignore_typename = true; diff --git a/apollo-federation/src/operation/merging.rs b/apollo-federation/src/operation/merging.rs index 7f20171d3b..22029fabe3 100644 --- a/apollo-federation/src/operation/merging.rs +++ b/apollo-federation/src/operation/merging.rs @@ -237,7 +237,8 @@ impl SelectionSet { } } - for (key, self_selection) in target.iter_mut() { + for self_selection in target.values_mut() { + let key = self_selection.key(); match self_selection { SelectionValue::Field(mut self_field_selection) => { if let Some(other_field_selections) = fields.shift_remove(&key) { diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index b93a595d1b..d85a9c1e26 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -1600,7 +1600,7 @@ impl SelectionSet { destination: &mut Vec, selection_set: &SelectionSet, ) -> Result<(), FederationError> { - for (_, value) in selection_set.selections.iter() { + for value in selection_set.selections.values() { match value { Selection::Field(field_selection) => { let selections = match &field_selection.selection_set { @@ -1688,7 +1688,8 @@ impl SelectionSet { let mut sibling_field_key: Option = None; let mutable_selection_map = Arc::make_mut(&mut self.selections); - for (key, entry) in mutable_selection_map.iter_mut() { + for entry in mutable_selection_map.values_mut() { + let key = entry.key(); match entry { SelectionValue::Field(mut field_selection) => { if field_selection.get().field.is_plain_typename_field() @@ -1852,8 +1853,8 @@ impl SelectionSet { sub_selection_updates.extend( sub_selection_set .selections - .iter() - .map(|(k, v)| (k.clone(), v.clone())), + .values() + .map(|v| (v.key(), v.clone())), ); } } @@ -1923,9 +1924,9 @@ impl SelectionSet { let mut updated_selections = MultiIndexMap::new(); updated_selections.extend( self.selections - .iter() + .values() .take(index) - .map(|(k, v)| (k.clone(), v.clone())), + .map(|v| (v.key(), v.clone())), ); let mut update_new_selection = |selection| match selection { @@ -2293,7 +2294,7 @@ impl SelectionSet { fn fields_in_set(&self) -> Vec { let mut fields = Vec::new(); - for (_key, selection) in self.selections.iter() { + for selection in self.selections.values() { match selection { Selection::Field(field) => fields.push(CollectedFieldInSet { path: Vec::new(), @@ -2411,20 +2412,20 @@ impl SelectionSet { } impl IntoIterator for SelectionSet { - type Item = ::Item; - type IntoIter = ::IntoIter; + type Item = Selection; + type IntoIter = selection_map::IntoValues; fn into_iter(self) -> Self::IntoIter { - Arc::unwrap_or_clone(self.selections).into_iter() + Arc::unwrap_or_clone(self.selections).into_values() } } impl<'a> IntoIterator for &'a SelectionSet { - type Item = <&'a SelectionMap as IntoIterator>::Item; - type IntoIter = <&'a SelectionMap as IntoIterator>::IntoIter; + type Item = &'a Selection; + type IntoIter = selection_map::Values<'a>; fn into_iter(self) -> Self::IntoIter { - self.selections.as_ref().iter() + self.selections.values() } } @@ -3097,10 +3098,7 @@ impl DeferNormalizer { assigned_labels: IndexSet::default(), conditions: IndexMap::default(), }; - let mut stack = selection_set - .into_iter() - .map(|(_, sel)| sel) - .collect::>(); + let mut stack = selection_set.into_iter().collect::>(); while let Some(selection) = stack.pop() { if let Selection::InlineFragment(inline) = selection { if let Some(args) = inline.inline_fragment.data().defer_directive_arguments()? { @@ -3110,13 +3108,7 @@ impl DeferNormalizer { } } } - stack.extend( - selection - .selection_set() - .into_iter() - .flatten() - .map(|(_, sel)| sel), - ); + stack.extend(selection.selection_set().into_iter().flatten()); } Ok(digest) } @@ -3423,8 +3415,8 @@ impl SelectionSet { selections, } = self; Arc::unwrap_or_clone(selections) - .into_iter() - .map(|(_, sel)| sel.normalize_defer(normalizer)) + .into_values() + .map(|sel| sel.normalize_defer(normalizer)) .try_collect() .map(|selections| Self { schema, diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index dcbba5fb66..16a418eecc 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -231,7 +231,7 @@ impl Selection { return self .with_updated_selections( self_sub_selection.type_position.clone(), - diff.into_iter().map(|(_, v)| v), + diff.into_iter(), ) .map(Some); } @@ -254,7 +254,7 @@ impl Selection { return self .with_updated_selections( self_sub_selection.type_position.clone(), - common.into_iter().map(|(_, v)| v), + common.into_iter(), ) .map(Some); } @@ -269,9 +269,9 @@ impl SelectionSet { pub(crate) fn minus(&self, other: &SelectionSet) -> Result { let iter = self .selections - .iter() - .map(|(k, v)| { - if let Some(other_v) = other.selections.get(&k) { + .values() + .map(|v| { + if let Some(other_v) = other.selections.get(&v.key()) { v.minus(other_v) } else { Ok(Some(v.clone())) @@ -298,9 +298,9 @@ impl SelectionSet { let iter = self .selections - .iter() - .map(|(k, v)| { - if let Some(other_v) = other.selections.get(&k) { + .values() + .map(|v| { + if let Some(other_v) = other.selections.get(&v.key()) { v.intersection(other_v) } else { Ok(None) @@ -752,7 +752,7 @@ impl Fragment { return false; } - self.selection_set.selections.iter().any(|(_, selection)| { + self.selection_set.selections.values().any(|selection| { matches!( selection, Selection::FragmentSpread(fragment) if fragment.spread.fragment_name == *other_fragment_name @@ -1663,7 +1663,7 @@ impl FragmentGenerator { selection_set.type_position.clone(), ); - for (_key, selection) in Arc::make_mut(&mut selection_set.selections).iter_mut() { + for selection in Arc::make_mut(&mut selection_set.selections).values_mut() { match selection { SelectionValue::Field(mut field) => { if let Some(selection_set) = field.get_selection_set_mut() { diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 5e2d29c75d..0a916e3017 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -710,8 +710,8 @@ impl SelectionSet { ) -> Result { let rebased_results = self .selections - .iter() - .map(|(_, selection)| { + .values() + .map(|selection| { selection.rebase_inner( parent_type, named_fragments, diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index f06e187c9e..210e28760e 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -108,7 +108,7 @@ pub(crate) struct SelectionMap { impl std::fmt::Debug for SelectionMap { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_map().entries(self.iter()).finish() + f.debug_set().entries(self.values()).finish() } } @@ -129,11 +129,11 @@ impl Serialize for SelectionMap { where S: serde::Serializer, { - let mut map = serializer.serialize_map(Some(self.len()))?; - for (key, value) in self.iter() { - map.serialize_entry(&key, value)?; + let mut seq = serializer.serialize_seq(Some(self.len()))?; + for value in self.values() { + seq.serialize_element(value)?; } - map.end() + seq.end() } } @@ -271,19 +271,6 @@ impl SelectionMap { assert!(self.selections.len() == self.table.len()); } - /// Iterate over all selections and selection keys. - #[deprecated = "Prefer values()"] - pub(crate) fn iter(&self) -> impl Iterator { - self.selections.iter().map(|v| (v.key(), v)) - } - - #[deprecated = "Prefer values_mut()"] - pub(crate) fn iter_mut(&mut self) -> impl Iterator)> { - self.selections - .iter_mut() - .map(|v| (v.key(), SelectionValue::new(v))) - } - /// Iterate over all selections. pub(crate) fn values(&self) -> std::slice::Iter<'_, Selection> { self.selections.iter() @@ -571,29 +558,3 @@ impl<'a> VacantEntry<'a> { Ok(SelectionValue::new(self.map.raw_insert(self.hash, value))) } } - -impl IntoIterator for SelectionMap { - type Item = (SelectionKey, Selection); - type IntoIter = - std::iter::Map, fn(Selection) -> (SelectionKey, Selection)>; - - fn into_iter(self) -> Self::IntoIter { - self.selections - .into_iter() - .map(|selection| (selection.key(), selection)) - } -} - -impl<'a> IntoIterator for &'a SelectionMap { - type Item = (SelectionKey, &'a Selection); - type IntoIter = std::iter::Map< - std::slice::Iter<'a, Selection>, - fn(&'a Selection) -> (SelectionKey, &'a Selection), - >; - - fn into_iter(self) -> Self::IntoIter { - self.selections - .iter() - .map(|selection| (selection.key(), selection)) - } -} diff --git a/apollo-federation/src/operation/simplify.rs b/apollo-federation/src/operation/simplify.rs index f7c339d210..9c7221c07e 100644 --- a/apollo-federation/src/operation/simplify.rs +++ b/apollo-federation/src/operation/simplify.rs @@ -260,7 +260,7 @@ impl InlineFragmentSelection { && this_condition.is_some_and(|c| c.is_abstract_type()) { let mut liftable_selections = SelectionMap::new(); - for (_, selection) in selection_set.selections.iter() { + for selection in selection_set.selections.values() { match selection { Selection::FragmentSpread(spread_selection) => { let type_condition = &spread_selection.spread.type_condition_position; @@ -323,8 +323,8 @@ impl InlineFragmentSelection { // Since liftable_selections are changing their parent, we need to rebase them. liftable_selections = liftable_selections - .into_iter() - .map(|(_key, sel)| sel.rebase_on(parent_type, named_fragments, schema)) + .into_values() + .map(|sel| sel.rebase_on(parent_type, named_fragments, schema)) .collect::>()?; let mut final_selection_map = SelectionMap::new(); From 6a8924d57f37fd86ee0f09c84837a313955af317 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 26 Sep 2024 17:20:50 +0200 Subject: [PATCH 05/17] Use non-owning selection keys --- apollo-federation/src/operation/contains.rs | 5 +- apollo-federation/src/operation/merging.rs | 8 +- apollo-federation/src/operation/mod.rs | 129 +++++++-------- apollo-federation/src/operation/optimize.rs | 16 +- .../src/operation/selection_map.rs | 147 ++++++++++++++---- apollo-federation/src/operation/tests/mod.rs | 25 ++- .../src/query_plan/fetch_dependency_graph.rs | 2 +- 7 files changed, 202 insertions(+), 130 deletions(-) diff --git a/apollo-federation/src/operation/contains.rs b/apollo-federation/src/operation/contains.rs index 142d4a8d1a..904fcdb9d0 100644 --- a/apollo-federation/src/operation/contains.rs +++ b/apollo-federation/src/operation/contains.rs @@ -164,15 +164,14 @@ impl SelectionSet { let mut did_ignore_typename = false; for other_selection in other.selections.values() { - let key = other_selection.key(); - if key.is_typename_field() && options.ignore_missing_typename { + if other_selection.is_typename_field() && options.ignore_missing_typename { if !self.has_top_level_typename_field() { did_ignore_typename = true; } continue; } - let Some(self_selection) = self.selections.get(&key) else { + let Some(self_selection) = self.selections.get(other_selection.key()) else { return Containment::NotContained; }; diff --git a/apollo-federation/src/operation/merging.rs b/apollo-federation/src/operation/merging.rs index 22029fabe3..d45fdfd4a4 100644 --- a/apollo-federation/src/operation/merging.rs +++ b/apollo-federation/src/operation/merging.rs @@ -193,7 +193,7 @@ impl SelectionSet { ); }; fields - .entry(other_key) + .entry(other_key.to_owned_key()) .or_insert_with(Vec::new) .push(other_field_selection); } @@ -207,7 +207,7 @@ impl SelectionSet { ); }; fragment_spreads - .entry(other_key) + .entry(other_key.to_owned_key()) .or_insert_with(Vec::new) .push(other_fragment_spread_selection); } @@ -226,7 +226,7 @@ impl SelectionSet { ); }; inline_fragments - .entry(other_key) + .entry(other_key.to_owned_key()) .or_insert_with(Vec::new) .push(other_inline_fragment_selection); } @@ -238,7 +238,7 @@ impl SelectionSet { } for self_selection in target.values_mut() { - let key = self_selection.key(); + let key = self_selection.key().to_owned_key(); match self_selection { SelectionValue::Field(mut self_field_selection) => { if let Some(other_field_selections) = fields.shift_remove(&key) { diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index d85a9c1e26..49ba9b2385 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -74,7 +74,7 @@ static NEXT_ID: atomic::AtomicUsize = atomic::AtomicUsize::new(1); /// /// Note that we shouldn't add `derive(Serialize, Deserialize)` to this without changing the types /// to be something like UUIDs. -#[derive(Clone, Debug, Eq, PartialEq, Hash)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] // NOTE(@TylerBloom): This feature gate can be removed once the condition in the comment above is // met. Note that there are `serde(skip)` statements that should be removed once this is removed. #[cfg_attr(feature = "snapshot_tracing", derive(Serialize))] @@ -404,9 +404,19 @@ impl Selection { } } + /// Returns true if the selection key is `__typename` *without directives*. + /// /// # Errors /// Returns an error if the selection contains a fragment spread, or if any of the /// @skip/@include directives are invalid (per GraphQL validation rules). + pub(crate) fn is_typename_field(&self) -> bool { + if let Selection::Field(field) = self { + *field.field.response_name() == TYPENAME_FIELD && field.field.directives.is_empty() + } else { + false + } + } + pub(crate) fn conditions(&self) -> Result { let self_conditions = Conditions::from_directives(self.directives())?; if let Conditions::Boolean(false) = self_conditions { @@ -519,7 +529,7 @@ impl From for Selection { } impl HasSelectionKey for Selection { - fn key(&self) -> SelectionKey { + fn key(&self) -> SelectionKey<'_> { match self { Selection::Field(field_selection) => field_selection.key(), Selection::FragmentSpread(fragment_spread_selection) => fragment_spread_selection.key(), @@ -606,7 +616,7 @@ mod field_selection { } impl HasSelectionKey for FieldSelection { - fn key(&self) -> SelectionKey { + fn key(&self) -> SelectionKey<'_> { self.field.key() } } @@ -634,7 +644,6 @@ mod field_selection { #[derive(Clone, Serialize)] pub(crate) struct Field { data: FieldData, - key: SelectionKey, } impl std::fmt::Debug for Field { @@ -646,7 +655,7 @@ mod field_selection { impl PartialEq for Field { fn eq(&self, other: &Self) -> bool { self.data.field_position.field_name() == other.data.field_position.field_name() - && self.key == other.key + && self.key() == other.key() && self.data.arguments == other.data.arguments } } @@ -656,7 +665,7 @@ mod field_selection { impl Hash for Field { fn hash(&self, state: &mut H) { self.data.field_position.field_name().hash(state); - self.key.hash(state); + self.key().hash(state); self.data.arguments.hash(state); } } @@ -671,10 +680,7 @@ mod field_selection { impl Field { pub(crate) fn new(data: FieldData) -> Self { - Self { - key: data.key(), - data, - } + Self { data } } // Note: The `schema` argument must be a subgraph schema, so the __typename field won't @@ -769,13 +775,13 @@ mod field_selection { } pub(crate) fn as_path_element(&self) -> FetchDataPathElement { - FetchDataPathElement::Key(self.response_name(), Default::default()) + FetchDataPathElement::Key(self.response_name().clone(), Default::default()) } } impl HasSelectionKey for Field { - fn key(&self) -> SelectionKey { - self.key.clone() + fn key(&self) -> SelectionKey<'_> { + self.data.key() } } @@ -831,8 +837,8 @@ mod field_selection { self.field_position.field_name() } - pub(crate) fn response_name(&self) -> Name { - self.alias.clone().unwrap_or_else(|| self.name().clone()) + pub(crate) fn response_name(&self) -> &Name { + self.alias.as_ref().unwrap_or_else(|| self.name()) } pub(crate) fn output_base_type(&self) -> Result { @@ -851,10 +857,10 @@ mod field_selection { } impl HasSelectionKey for FieldData { - fn key(&self) -> SelectionKey { + fn key(&self) -> SelectionKey<'_> { SelectionKey::Field { response_name: self.response_name(), - directives: self.directives.clone(), + directives: &self.directives, } } } @@ -885,7 +891,7 @@ mod fragment_spread_selection { } impl HasSelectionKey for FragmentSpreadSelection { - fn key(&self) -> SelectionKey { + fn key(&self) -> SelectionKey<'_> { self.spread.key() } } @@ -896,7 +902,6 @@ mod fragment_spread_selection { #[derive(Clone, Serialize)] pub(crate) struct FragmentSpread { data: FragmentSpreadData, - key: SelectionKey, } impl std::fmt::Debug for FragmentSpread { @@ -907,7 +912,7 @@ mod fragment_spread_selection { impl PartialEq for FragmentSpread { fn eq(&self, other: &Self) -> bool { - self.key == other.key + self.key() == other.key() } } @@ -923,10 +928,7 @@ mod fragment_spread_selection { impl FragmentSpread { pub(crate) fn new(data: FragmentSpreadData) -> Self { - Self { - key: data.key(), - data, - } + Self { data } } pub(crate) fn data(&self) -> &FragmentSpreadData { @@ -935,8 +937,8 @@ mod fragment_spread_selection { } impl HasSelectionKey for FragmentSpread { - fn key(&self) -> SelectionKey { - self.key.clone() + fn key(&self) -> SelectionKey<'_> { + self.data.key() } } @@ -961,15 +963,15 @@ mod fragment_spread_selection { } impl HasSelectionKey for FragmentSpreadData { - fn key(&self) -> SelectionKey { + fn key(&self) -> SelectionKey<'_> { if is_deferred_selection(&self.directives) { SelectionKey::Defer { deferred_id: self.selection_id, } } else { SelectionKey::FragmentSpread { - fragment_name: self.fragment_name.clone(), - directives: self.directives.clone(), + fragment_name: &self.fragment_name, + directives: &self.directives, } } } @@ -1122,7 +1124,7 @@ mod inline_fragment_selection { } impl HasSelectionKey for InlineFragmentSelection { - fn key(&self) -> SelectionKey { + fn key(&self) -> SelectionKey<'_> { self.inline_fragment.key() } } @@ -1132,7 +1134,6 @@ mod inline_fragment_selection { #[derive(Clone, Serialize)] pub(crate) struct InlineFragment { data: InlineFragmentData, - key: SelectionKey, } impl std::fmt::Debug for InlineFragment { @@ -1143,7 +1144,7 @@ mod inline_fragment_selection { impl PartialEq for InlineFragment { fn eq(&self, other: &Self) -> bool { - self.key == other.key + self.key() == other.key() } } @@ -1151,7 +1152,7 @@ mod inline_fragment_selection { impl Hash for InlineFragment { fn hash(&self, state: &mut H) { - self.key.hash(state); + self.key().hash(state); } } @@ -1165,10 +1166,7 @@ mod inline_fragment_selection { impl InlineFragment { pub(crate) fn new(data: InlineFragmentData) -> Self { - Self { - key: data.key(), - data, - } + Self { data } } pub(crate) fn schema(&self) -> &ValidFederationSchema { @@ -1206,8 +1204,8 @@ mod inline_fragment_selection { } impl HasSelectionKey for InlineFragment { - fn key(&self) -> SelectionKey { - self.key.clone() + fn key(&self) -> SelectionKey<'_> { + self.data.key() } } @@ -1242,18 +1240,18 @@ mod inline_fragment_selection { } impl HasSelectionKey for InlineFragmentData { - fn key(&self) -> SelectionKey { + fn key(&self) -> SelectionKey<'_> { if is_deferred_selection(&self.directives) { SelectionKey::Defer { - deferred_id: self.selection_id.clone(), + deferred_id: self.selection_id, } } else { SelectionKey::InlineFragment { type_condition: self .type_condition_position .as_ref() - .map(|pos| pos.type_name().clone()), - directives: self.directives.clone(), + .map(|pos| pos.type_name()), + directives: &self.directives, } } } @@ -1264,6 +1262,7 @@ pub(crate) use inline_fragment_selection::InlineFragment; pub(crate) use inline_fragment_selection::InlineFragmentData; pub(crate) use inline_fragment_selection::InlineFragmentSelection; +use self::selection_map::OwnedSelectionKey; use crate::schema::position::INTROSPECTION_TYPENAME_FIELD_NAME; /// A simple MultiMap implementation using IndexMap with Vec as its value type. @@ -1448,7 +1447,7 @@ impl SelectionSet { } pub(crate) fn contains_top_level_field(&self, field: &Field) -> Result { - if let Some(selection) = self.selections.get(&field.key()) { + if let Some(selection) = self.selections.get(field.key()) { let Selection::Field(field_selection) = selection else { return Err(SingleFederationError::Internal { message: format!( @@ -1684,21 +1683,21 @@ impl SelectionSet { interface_types_with_interface_objects.contains(&InterfaceTypeDefinitionPosition { type_name: self.type_position.type_name().clone(), }); - let mut typename_field_key: Option = None; - let mut sibling_field_key: Option = None; + let mut typename_field_key: Option = None; + let mut sibling_field_key: Option = None; let mutable_selection_map = Arc::make_mut(&mut self.selections); for entry in mutable_selection_map.values_mut() { - let key = entry.key(); + let key = entry.key().to_owned_key(); match entry { SelectionValue::Field(mut field_selection) => { if field_selection.get().field.is_plain_typename_field() && !is_interface_object && typename_field_key.is_none() { - typename_field_key = Some(key.clone()); + typename_field_key = Some(key); } else if sibling_field_key.is_none() { - sibling_field_key = Some(key.clone()); + sibling_field_key = Some(key); } if let Some(field_selection_set) = field_selection.get_selection_set_mut() { @@ -1730,8 +1729,8 @@ impl SelectionSet { Some((_, Selection::Field(typename_field))), Some(SelectionValue::Field(mut sibling_field)), ) = ( - mutable_selection_map.remove(&typename_key), - mutable_selection_map.get_mut(&sibling_field_key), + mutable_selection_map.remove(typename_key.as_borrowed_key()), + mutable_selection_map.get_mut(sibling_field_key.as_borrowed_key()), ) { // Note that as we tag the element, we also record the alias used if any since that // needs to be preserved. @@ -1926,17 +1925,19 @@ impl SelectionSet { self.selections .values() .take(index) - .map(|v| (v.key(), v.clone())), + .map(|v| (v.key().to_owned_key(), v.clone())), ); let mut update_new_selection = |selection| match selection { SelectionMapperReturn::None => {} // Removed; Skip it. SelectionMapperReturn::Selection(new_selection) => { - updated_selections.insert(new_selection.key(), new_selection) - } - SelectionMapperReturn::SelectionList(new_selections) => { - updated_selections.extend(new_selections.into_iter().map(|s| (s.key(), s))) + updated_selections.insert(new_selection.key().to_owned_key(), new_selection) } + SelectionMapperReturn::SelectionList(new_selections) => updated_selections.extend( + new_selections + .into_iter() + .map(|s| (s.key().to_owned_key(), s)), + ), }; // Now update the rest of the selections using the `mapper` function. @@ -2039,11 +2040,11 @@ impl SelectionSet { fn has_top_level_typename_field(&self) -> bool { const TYPENAME_KEY: SelectionKey = SelectionKey::Field { - response_name: TYPENAME_FIELD, - directives: DirectiveList::new(), + response_name: &TYPENAME_FIELD, + directives: &DirectiveList::new(), }; - self.selections.contains_key(&TYPENAME_KEY) + self.selections.contains_key(TYPENAME_KEY) } /// Adds a path, and optional some selections following that path, to this selection map. @@ -2517,7 +2518,7 @@ fn compute_aliases_for_non_merging_fields( let response_name = field.field.response_name(); let field_type = &field.field.field_position.get(field_schema)?.ty; - match seen_response_names.get(&response_name) { + match seen_response_names.get(response_name) { Some(previous) => { if &previous.field_name == field_name && types_can_be_merged(&previous.field_type, field_type, schema.schema())? @@ -2548,7 +2549,7 @@ fn compute_aliases_for_non_merging_fields( selections: field.selection_set.clone(), }); seen_response_names.insert( - response_name, + response_name.clone(), SeenResponseName { field_name: previous.field_name.clone(), field_type: previous.field_type.clone(), @@ -2560,7 +2561,7 @@ fn compute_aliases_for_non_merging_fields( } } else { // We need to alias the new occurence. - let alias = gen_alias_name(&response_name, &seen_response_names); + let alias = gen_alias_name(response_name, &seen_response_names); // Given how we generate aliases, it's is very unlikely that the generated alias will conflict with any of the other response name // at the level, but it's theoretically possible. By adding the alias to the seen names, we ensure that in the remote change that @@ -2590,7 +2591,7 @@ fn compute_aliases_for_non_merging_fields( alias_collector.push(FieldToAlias { path, - response_name, + response_name: response_name.clone(), alias, }) } @@ -2611,7 +2612,7 @@ fn compute_aliases_for_non_merging_fields( None => None, }; seen_response_names.insert( - response_name, + response_name.clone(), SeenResponseName { field_name: field_name.clone(), field_type: field_type.clone(), diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 16a418eecc..37ba8a0be6 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -271,7 +271,7 @@ impl SelectionSet { .selections .values() .map(|v| { - if let Some(other_v) = other.selections.get(&v.key()) { + if let Some(other_v) = other.selections.get(v.key()) { v.minus(other_v) } else { Ok(Some(v.clone())) @@ -300,7 +300,7 @@ impl SelectionSet { .selections .values() .map(|v| { - if let Some(other_v) = other.selections.get(&v.key()) { + if let Some(other_v) = other.selections.get(v.key()) { v.intersection(other_v) } else { Ok(None) @@ -414,7 +414,7 @@ impl FieldsConflictValidator { for selection_set in level { for field_selection in selection_set.field_selections() { let response_name = field_selection.field.response_name(); - let at_response_name = at_level.entry(response_name).or_default(); + let at_response_name = at_level.entry(response_name.clone()).or_default(); let entry = at_response_name .entry(field_selection.field.clone()) .or_default(); @@ -444,7 +444,7 @@ impl FieldsConflictValidator { fn for_field<'v>(&'v self, field: &Field) -> impl Iterator> + 'v { self.by_response_name - .get(&field.response_name()) + .get(field.response_name()) .into_iter() .flat_map(|by_response_name| by_response_name.values()) .flatten() @@ -685,7 +685,7 @@ impl FragmentRestrictionAtType { fn is_useless(&self) -> bool { match self.selections.selections.as_slice().split_first() { None => true, - Some((first, rest)) => rest.is_empty() && first.key().is_typename_field(), + Some((first, rest)) => rest.is_empty() && first.is_typename_field(), } } } @@ -3513,9 +3513,9 @@ mod tests { } Some((first, rest)) => { - let result = Arc::make_mut(&mut ss.selections).get_mut(&SelectionKey::Field { - response_name: (*first).clone(), - directives: Default::default(), + let result = Arc::make_mut(&mut ss.selections).get_mut(SelectionKey::Field { + response_name: first, + directives: &Default::default(), }); let Some(mut value) = result else { return Err(FederationError::internal("No matching field found")); diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index 210e28760e..38de54cbd3 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -29,28 +29,28 @@ use crate::operation::SiblingTypename; /// * directives have to be applied in the same order /// * directive arguments order does not matter (they get automatically sorted by their names). /// * selection cannot specify @defer directive -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] -pub(crate) enum SelectionKey { +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize)] +pub(crate) enum SelectionKey<'a> { Field { /// The field alias (if specified) or field name in the resulting selection set. - response_name: Name, + response_name: &'a Name, /// directives applied on the field #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] - directives: DirectiveList, + directives: &'a DirectiveList, }, FragmentSpread { /// The name of the fragment. - fragment_name: Name, + fragment_name: &'a Name, /// Directives applied on the fragment spread (does not contain @defer). #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] - directives: DirectiveList, + directives: &'a DirectiveList, }, InlineFragment { /// The optional type condition of the fragment. - type_condition: Option, + type_condition: Option<&'a Name>, /// Directives applied on the fragment spread (does not contain @defer). #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] - directives: DirectiveList, + directives: &'a DirectiveList, }, Defer { /// Unique selection ID used to distinguish deferred fragment spreads that cannot be merged. @@ -59,28 +59,107 @@ pub(crate) enum SelectionKey { }, } -impl SelectionKey { - /// Returns true if the selection key is `__typename` *without directives*. - #[deprecated = "Use the Selection type instead"] - pub(crate) fn is_typename_field(&self) -> bool { - matches!(self, SelectionKey::Field { response_name, directives } if *response_name == super::TYPENAME_FIELD && directives.is_empty()) +impl SelectionKey<'_> { + /// Get an owned structure representing the selection key, for use in map keys + /// that are not a plain selection map. + pub(crate) fn to_owned_key(self) -> OwnedSelectionKey { + match self { + Self::Field { + response_name, + directives, + } => OwnedSelectionKey::Field { + response_name: response_name.clone(), + directives: directives.clone(), + }, + Self::FragmentSpread { + fragment_name, + directives, + } => OwnedSelectionKey::FragmentSpread { + fragment_name: fragment_name.clone(), + directives: directives.clone(), + }, + Self::InlineFragment { + type_condition, + directives, + } => OwnedSelectionKey::InlineFragment { + type_condition: type_condition.cloned(), + directives: directives.clone(), + }, + Self::Defer { deferred_id } => OwnedSelectionKey::Defer { deferred_id }, + } } +} + +/// An owned structure representing the selection key, for use in map keys +/// that are not a plain selection map. +#[derive(Debug, Clone, Hash, PartialEq, Eq)] +pub(crate) enum OwnedSelectionKey { + Field { + response_name: Name, + directives: DirectiveList, + }, + FragmentSpread { + fragment_name: Name, + directives: DirectiveList, + }, + InlineFragment { + type_condition: Option, + directives: DirectiveList, + }, + Defer { + deferred_id: SelectionId, + }, +} + +impl OwnedSelectionKey { + /// Get a plain, borrowed selection key, that can be used for indexing into a selection map. + pub(crate) fn as_borrowed_key(&self) -> SelectionKey<'_> { + match self { + OwnedSelectionKey::Field { + response_name, + directives, + } => SelectionKey::Field { + response_name, + directives, + }, + OwnedSelectionKey::FragmentSpread { + fragment_name, + directives, + } => SelectionKey::FragmentSpread { + fragment_name, + directives, + }, + OwnedSelectionKey::InlineFragment { + type_condition, + directives, + } => SelectionKey::InlineFragment { + type_condition: type_condition.as_ref(), + directives, + }, + OwnedSelectionKey::Defer { deferred_id } => SelectionKey::Defer { + deferred_id: *deferred_id, + }, + } + } +} +impl<'a> SelectionKey<'a> { /// Create a selection key for a specific field name. /// /// This is available for tests only as selection keys should not normally be created outside of /// `HasSelectionKey::key`. #[cfg(test)] - pub(crate) fn field_name(name: &str) -> Self { + pub(crate) fn field_name(name: &'a Name) -> Self { + static EMPTY_LIST: DirectiveList = DirectiveList::new(); SelectionKey::Field { - response_name: Name::new(name).unwrap(), - directives: Default::default(), + response_name: &name, + directives: &EMPTY_LIST, } } } pub(crate) trait HasSelectionKey { - fn key(&self) -> SelectionKey; + fn key(&self) -> SelectionKey<'_>; } #[derive(Clone)] @@ -118,7 +197,7 @@ impl PartialEq for SelectionMap { self.len() == other.len() && self .values() - .all(|left| other.get(&left.key()).is_some_and(|right| left == right)) + .all(|left| other.get(left.key()).is_some_and(|right| left == right)) } } @@ -151,8 +230,8 @@ pub(crate) type IntoValues = std::vec::IntoIter; /// matches the given key. /// /// The returned function panics if the index is out of bounds. -fn key_eq<'a>(selections: &'a [Selection], key: &SelectionKey) -> impl Fn(&Bucket) -> bool + 'a { - move |bucket| selections[bucket.index].key() == *key +fn key_eq<'a>(selections: &'a [Selection], key: SelectionKey<'a>) -> impl Fn(&Bucket) -> bool + 'a { + move |bucket| selections[bucket.index].key() == key } impl SelectionMap { @@ -180,12 +259,12 @@ impl SelectionMap { } /// Computes the hash of a selection key. - fn hash(&self, key: SelectionKey) -> u64 { + fn hash(&self, key: SelectionKey<'_>) -> u64 { self.hash_builder.hash_one(key) } /// Returns true if the given key exists in the map. - pub(crate) fn contains_key(&self, key: &SelectionKey) -> bool { + pub(crate) fn contains_key(&self, key: SelectionKey<'_>) -> bool { let hash = self.hash(key); self.table .find(hash, key_eq(&self.selections, key)) @@ -193,13 +272,13 @@ impl SelectionMap { } /// Returns true if the given key exists in the map. - pub(crate) fn get(&self, key: &SelectionKey) -> Option<&Selection> { + pub(crate) fn get(&self, key: SelectionKey<'_>) -> Option<&Selection> { let hash = self.hash(key); let bucket = self.table.find(hash, key_eq(&self.selections, key))?; Some(&self.selections[bucket.index]) } - pub(crate) fn get_mut(&mut self, key: &SelectionKey) -> Option> { + pub(crate) fn get_mut(&mut self, key: SelectionKey<'_>) -> Option> { let hash = self.hash(key); let bucket = self.table.find_mut(hash, key_eq(&self.selections, key))?; Some(SelectionValue::new(&mut self.selections[bucket.index])) @@ -240,12 +319,12 @@ impl SelectionMap { } pub(crate) fn insert(&mut self, value: Selection) { - let hash = self.hash(&value.key()); + let hash = self.hash(value.key()); self.raw_insert(hash, value); } /// Remove a selection from the map. Returns the selection and its numeric index. - pub(crate) fn remove(&mut self, key: &SelectionKey) -> Option<(usize, Selection)> { + pub(crate) fn remove(&mut self, key: SelectionKey<'_>) -> Option<(usize, Selection)> { let hash = self.hash(key); let entry = self .table @@ -257,10 +336,10 @@ impl SelectionMap { Some((bucket.index, selection)) } - pub(crate) fn retain(&mut self, mut predicate: impl FnMut(&SelectionKey, &Selection) -> bool) { + pub(crate) fn retain(&mut self, mut predicate: impl FnMut(SelectionKey<'_>, &Selection) -> bool) { self.selections.retain(|selection| { let key = selection.key(); - predicate(&key, selection) + predicate(key, selection) }); if self.selections.len() < self.table.len() { // In theory, we could track which keys were removed, and adjust the indices based on @@ -286,7 +365,7 @@ impl SelectionMap { self.selections.into_iter() } - pub(super) fn entry<'a>(&'a mut self, key: &SelectionKey) -> Entry<'a> { + pub(super) fn entry<'a>(&'a mut self, key: SelectionKey<'_>) -> Entry<'a> { let hash = self.hash(key); let slot = self.table.find_entry(hash, key_eq(&self.selections, key)); match slot { @@ -433,7 +512,7 @@ impl<'a> SelectionValue<'a> { } } - pub(super) fn key(&self) -> SelectionKey { + pub(super) fn key(&self) -> SelectionKey<'_> { match self { Self::Field(field) => field.get().key(), Self::FragmentSpread(frag) => frag.get().key(), @@ -537,16 +616,16 @@ impl<'a> OccupiedEntry<'a> { pub(crate) struct VacantEntry<'a> { map: &'a mut SelectionMap, hash: u64, - key: SelectionKey, + key: SelectionKey<'a>, } impl<'a> VacantEntry<'a> { - pub(crate) fn key(&self) -> &SelectionKey { - &self.key + pub(crate) fn key(&self) -> SelectionKey<'a> { + self.key } pub(crate) fn insert(self, value: Selection) -> Result, FederationError> { - if *self.key() != value.key() { + if self.key() != value.key() { return Err(Internal { message: format!( "Key mismatch when inserting selection {} into vacant entry ", diff --git a/apollo-federation/src/operation/tests/mod.rs b/apollo-federation/src/operation/tests/mod.rs index 52eb810f7d..70fa203778 100644 --- a/apollo-federation/src/operation/tests/mod.rs +++ b/apollo-federation/src/operation/tests/mod.rs @@ -1120,13 +1120,13 @@ fn converting_operation_types() { } fn contains_field(ss: &SelectionSet, field_name: Name) -> bool { - ss.selections.contains_key(&SelectionKey::Field { - response_name: field_name, - directives: Default::default(), + ss.selections.contains_key(SelectionKey::Field { + response_name: &field_name, + directives: &Default::default(), }) } -fn is_named_field(sk: &SelectionKey, name: Name) -> bool { +fn is_named_field(sk: SelectionKey, name: Name) -> bool { matches!(sk, SelectionKey::Field { response_name, directives: _ } if *response_name == name) @@ -1137,14 +1137,7 @@ fn get_value_at_path<'a>(ss: &'a SelectionSet, path: &[Name]) -> Option<&'a Sele // Error: empty path return None; }; - let result = ss.selections.get(&SelectionKey::Field { - response_name: (*first).clone(), - directives: Default::default(), - }); - let Some(value) = result else { - // Error: No matching field found. - return None; - }; + let value = ss.selections.get(SelectionKey::field_name(first))?; if rest.is_empty() { // Base case => We are done. Some(value) @@ -1305,14 +1298,14 @@ mod lazy_map_tests { // Remove `foo` let remove_foo = - filter_rec(&selection_set, &|s| !is_named_field(&s.key(), name!("foo"))).unwrap(); + filter_rec(&selection_set, &|s| !is_named_field(s.key(), name!("foo"))).unwrap(); assert!(contains_field(&remove_foo, name!("some_int"))); assert!(contains_field(&remove_foo, name!("foo2"))); assert!(!contains_field(&remove_foo, name!("foo"))); // Remove `bar` let remove_bar = - filter_rec(&selection_set, &|s| !is_named_field(&s.key(), name!("bar"))).unwrap(); + filter_rec(&selection_set, &|s| !is_named_field(s.key(), name!("bar"))).unwrap(); // "foo2" should be removed, since it has no sub-selections left. assert!(!contains_field(&remove_bar, name!("foo2"))); } @@ -1355,7 +1348,7 @@ mod lazy_map_tests { // Add __typename next to any "id" field. let result = - add_typename_if(&selection_set, &|s| is_named_field(&s.key(), name!("id"))).unwrap(); + add_typename_if(&selection_set, &|s| is_named_field(s.key(), name!("id"))).unwrap(); // The top level won't have __typename, since it doesn't have "id". assert!(!contains_field(&result, name!("__typename"))); @@ -1622,7 +1615,7 @@ fn used_variables() { let Selection::Field(subquery) = operation .selection_set .selections - .get(&SelectionKey::field_name("subquery")) + .get(SelectionKey::field_name(&name!("subquery"))) .unwrap() else { unreachable!(); diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index f31bd0d2ff..085f6e0a08 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -660,7 +660,7 @@ impl FetchDependencyGraphNodePath { } new_path.push(FetchDataPathElement::Key( - field.response_name(), + field.response_name().clone(), Default::default(), )); From 07d2c779a1f506068ce39ff5259e5be028c85631 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 27 Sep 2024 09:28:14 +0200 Subject: [PATCH 06/17] Adjust doc comments and linting --- .../src/operation/selection_map.rs | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index 38de54cbd3..92d0b1e39f 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -9,7 +9,6 @@ use serde::ser::SerializeSeq; use serde::Serialize; use crate::error::FederationError; -use crate::error::SingleFederationError::Internal; use crate::operation::field_selection::FieldSelection; use crate::operation::fragment_spread_selection::FragmentSpreadSelection; use crate::operation::inline_fragment_selection::InlineFragmentSelection; @@ -152,7 +151,7 @@ impl<'a> SelectionKey<'a> { pub(crate) fn field_name(name: &'a Name) -> Self { static EMPTY_LIST: DirectiveList = DirectiveList::new(); SelectionKey::Field { - response_name: &name, + response_name: name, directives: &EMPTY_LIST, } } @@ -173,11 +172,9 @@ struct Bucket { /// together by the user of this structure: the selection map API itself will overwrite selections /// with the same key. /// -/// Because the key depends strictly on the value, we expose the underlying map's API in a -/// read-only capacity, while mutations use an API closer to `IndexSet`. We don't just use an -/// `IndexSet` since key computation is expensive (it involves sorting). This type is in its own -/// module to prevent code from accidentally mutating the underlying map outside the mutation -/// API. +/// Once a selection is in the selection map, it must not be modified in a way that changes the +/// selection key. Therefore, the selection map only hands out mutable access through the +/// SelectionValue types, which expose the parts of selections that are safe to modify. #[derive(Clone)] pub(crate) struct SelectionMap { hash_builder: DefaultHashBuilder, @@ -223,7 +220,10 @@ impl Default for SelectionMap { } pub(crate) type Values<'a> = std::slice::Iter<'a, Selection>; -pub(crate) type ValuesMut<'a> = std::slice::IterMut<'a, Selection>; +pub(crate) type ValuesMut<'a> = std::iter::Map< + std::slice::IterMut<'a, Selection>, + fn(&'a mut Selection) -> SelectionValue<'a>, +>; pub(crate) type IntoValues = std::vec::IntoIter; /// Return an equality function taking an index into `selections` and returning if the index @@ -288,8 +288,6 @@ impl SelectionMap { fn raw_insert(&mut self, hash: u64, value: Selection) -> &mut Selection { let index = self.selections.len(); - // `insert` overwrites a selection without running `Drop`: because - // we only store an integer which is `Copy`, this works out fine self.table.insert_unique(hash, Bucket { index, hash }, |existing| existing.hash); self.selections.push(value); @@ -351,17 +349,17 @@ impl SelectionMap { } /// Iterate over all selections. - pub(crate) fn values(&self) -> std::slice::Iter<'_, Selection> { + pub(crate) fn values(&self) -> Values<'_> { self.selections.iter() } /// Iterate over all selections. - pub(crate) fn values_mut(&mut self) -> impl Iterator> { + pub(crate) fn values_mut(&mut self) -> ValuesMut<'_> { self.selections.iter_mut().map(SelectionValue::new) } /// Iterate over all selections. - pub(crate) fn into_values(self) -> std::vec::IntoIter { + pub(crate) fn into_values(self) -> IntoValues { self.selections.into_iter() } @@ -626,13 +624,9 @@ impl<'a> VacantEntry<'a> { pub(crate) fn insert(self, value: Selection) -> Result, FederationError> { if self.key() != value.key() { - return Err(Internal { - message: format!( - "Key mismatch when inserting selection {} into vacant entry ", - value - ), - } - .into()); + return Err(FederationError::internal(format!( + "Key mismatch when inserting selection {value} into vacant entry " + ))); }; Ok(SelectionValue::new(self.map.raw_insert(self.hash, value))) } From 4e43243d34d4210aec347ceac0fb3f7adf4390d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 26 Sep 2024 17:46:20 +0200 Subject: [PATCH 07/17] Merge Field and FieldData --- apollo-federation/src/operation/mod.rs | 274 ++++++++---------- apollo-federation/src/operation/rebase.rs | 16 +- apollo-federation/src/operation/simplify.rs | 11 +- apollo-federation/src/operation/tests/mod.rs | 11 +- .../src/query_graph/graph_path.rs | 14 +- .../src/query_graph/path_tree.rs | 5 +- .../src/query_plan/fetch_dependency_graph.rs | 10 +- 7 files changed, 156 insertions(+), 185 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 49ba9b2385..7ab49430a7 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -582,7 +582,6 @@ impl Fragment { mod field_selection { use std::hash::Hash; use std::hash::Hasher; - use std::ops::Deref; use apollo_compiler::Name; use serde::Serialize; @@ -632,31 +631,52 @@ mod field_selection { } } - pub(crate) fn with_updated_alias(&self, alias: Name) -> Field { - let mut data = self.field.data().clone(); - data.alias = Some(alias); - Field::new(data) + pub(crate) fn with_updated_directives(&self, directives: impl Into) -> Self { + Self { + field: self.field.with_updated_directives(directives), + selection_set: self.selection_set.clone(), + } } } - /// The non-selection-set data of `FieldSelection`, used with operation paths and graph - /// paths. - #[derive(Clone, Serialize)] - pub(crate) struct Field { - data: FieldData, + // SiblingTypename indicates how the sibling __typename field should be restored. + // PORT_NOTE: The JS version used the empty string to indicate unaliased sibling typenames. + // Here we use an enum to make the distinction explicit. + #[derive(Debug, Clone, Serialize)] + pub(crate) enum SiblingTypename { + Unaliased, + Aliased(Name), // the sibling __typename has been aliased } - impl std::fmt::Debug for Field { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.data.fmt(f) + impl SiblingTypename { + pub(crate) fn alias(&self) -> Option<&Name> { + match self { + SiblingTypename::Unaliased => None, + SiblingTypename::Aliased(alias) => Some(alias), + } } } + /// The non-selection-set data of `FieldSelection`, used with operation paths and graph + /// paths. + #[derive(Debug, Clone, Serialize)] + pub(crate) struct Field { + #[serde(skip)] + pub(crate) schema: ValidFederationSchema, + pub(crate) field_position: FieldDefinitionPosition, + pub(crate) alias: Option, + #[serde(serialize_with = "crate::display_helpers::serialize_as_debug_string")] + pub(crate) arguments: ArgumentList, + #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] + pub(crate) directives: DirectiveList, + pub(crate) sibling_typename: Option, + } + impl PartialEq for Field { fn eq(&self, other: &Self) -> bool { - self.data.field_position.field_name() == other.data.field_position.field_name() + self.field_position.field_name() == other.field_position.field_name() && self.key() == other.key() - && self.data.arguments == other.data.arguments + && self.arguments == other.arguments } } @@ -664,23 +684,26 @@ mod field_selection { impl Hash for Field { fn hash(&self, state: &mut H) { - self.data.field_position.field_name().hash(state); + self.field_position.field_name().hash(state); self.key().hash(state); - self.data.arguments.hash(state); - } - } - - impl Deref for Field { - type Target = FieldData; - - fn deref(&self) -> &Self::Target { - &self.data + self.arguments.hash(state); } } impl Field { - pub(crate) fn new(data: FieldData) -> Self { - Self { data } + /// Create a trivial field selection without any arguments or directives. + pub(crate) fn from_position( + schema: &ValidFederationSchema, + field_position: FieldDefinitionPosition, + ) -> Self { + Self { + schema: schema.clone(), + field_position, + alias: None, + arguments: Default::default(), + directives: Default::default(), + sibling_typename: None, + } } // Note: The `schema` argument must be a subgraph schema, so the __typename field won't @@ -690,14 +713,77 @@ mod field_selection { parent_type: &CompositeTypeDefinitionPosition, alias: Option, ) -> Self { - Self::new(FieldData { + Self { schema: schema.clone(), field_position: parent_type.introspection_typename_field(), alias, arguments: Default::default(), directives: Default::default(), sibling_typename: None, - }) + } + } + + pub(crate) fn schema(&self) -> &ValidFederationSchema { + &self.schema + } + + pub(crate) fn name(&self) -> &Name { + self.field_position.field_name() + } + + pub(crate) fn response_name(&self) -> &Name { + self.alias.as_ref().unwrap_or_else(|| self.name()) + } + + // Is this a plain simple __typename without any directive or alias? + pub(crate) fn is_plain_typename_field(&self) -> bool { + *self.field_position.field_name() == TYPENAME_FIELD + && self.directives.is_empty() + && self.alias.is_none() + } + + pub(super) fn directives_mut(&mut self) -> &mut DirectiveList { + &mut self.directives + } + + pub(crate) fn sibling_typename(&self) -> Option<&SiblingTypename> { + self.sibling_typename.as_ref() + } + + pub(crate) fn sibling_typename_mut(&mut self) -> &mut Option { + &mut self.sibling_typename + } + + pub(crate) fn as_path_element(&self) -> FetchDataPathElement { + FetchDataPathElement::Key(self.response_name().clone(), Default::default()) + } + + pub(crate) fn output_base_type(&self) -> Result { + let definition = self.field_position.get(self.schema.schema())?; + self.schema + .get_type(definition.ty.inner_named_type().clone()) + } + + pub(crate) fn is_leaf(&self) -> Result { + let base_type_position = self.output_base_type()?; + Ok(matches!( + base_type_position, + TypeDefinitionPosition::Scalar(_) | TypeDefinitionPosition::Enum(_) + )) + } + + pub(crate) fn with_updated_directives(&self, directives: impl Into) -> Self { + Self { + directives: directives.into(), + ..self.clone() + } + } + + pub(crate) fn with_updated_alias(&self, alias: Name) -> Self { + Self { + alias: Some(alias), + ..self.clone() + } } /// Turn this `Field` into a `FieldSelection` with the given sub-selection. If this is @@ -708,7 +794,7 @@ mod field_selection { ) -> FieldSelection { if cfg!(debug_assertions) { if let Some(ref selection_set) = selection_set { - if let Ok(field_type) = self.data.output_base_type() { + if let Ok(field_type) = self.output_base_type() { if let Ok(field_type_position) = CompositeTypeDefinitionPosition::try_from(field_type) { @@ -741,122 +827,9 @@ mod field_selection { selection_set, } } - - pub(crate) fn schema(&self) -> &ValidFederationSchema { - &self.data.schema - } - - pub(crate) fn data(&self) -> &FieldData { - &self.data - } - - // Is this a plain simple __typename without any directive or alias? - pub(crate) fn is_plain_typename_field(&self) -> bool { - *self.data.field_position.field_name() == TYPENAME_FIELD - && self.data.directives.is_empty() - && self.data.alias.is_none() - } - - pub(crate) fn sibling_typename(&self) -> Option<&SiblingTypename> { - self.data.sibling_typename.as_ref() - } - - pub(crate) fn sibling_typename_mut(&mut self) -> &mut Option { - &mut self.data.sibling_typename - } - - pub(crate) fn with_updated_directives( - &self, - directives: impl Into, - ) -> Field { - let mut data = self.data.clone(); - data.directives = directives.into(); - Self::new(data) - } - - pub(crate) fn as_path_element(&self) -> FetchDataPathElement { - FetchDataPathElement::Key(self.response_name().clone(), Default::default()) - } } impl HasSelectionKey for Field { - fn key(&self) -> SelectionKey<'_> { - self.data.key() - } - } - - // SiblingTypename indicates how the sibling __typename field should be restored. - // PORT_NOTE: The JS version used the empty string to indicate unaliased sibling typenames. - // Here we use an enum to make the distinction explicit. - #[derive(Debug, Clone, Serialize)] - pub(crate) enum SiblingTypename { - Unaliased, - Aliased(Name), // the sibling __typename has been aliased - } - - impl SiblingTypename { - pub(crate) fn alias(&self) -> Option<&Name> { - match self { - SiblingTypename::Unaliased => None, - SiblingTypename::Aliased(alias) => Some(alias), - } - } - } - - #[derive(Debug, Clone, Serialize)] - pub(crate) struct FieldData { - #[serde(skip)] - pub(crate) schema: ValidFederationSchema, - pub(crate) field_position: FieldDefinitionPosition, - pub(crate) alias: Option, - #[serde(serialize_with = "crate::display_helpers::serialize_as_debug_string")] - pub(crate) arguments: ArgumentList, - #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] - pub(crate) directives: DirectiveList, - pub(crate) sibling_typename: Option, - } - - impl FieldData { - #[cfg(test)] - /// Create a trivial field selection without any arguments or directives. - pub(crate) fn from_position( - schema: &ValidFederationSchema, - field_position: FieldDefinitionPosition, - ) -> Self { - Self { - schema: schema.clone(), - field_position, - alias: None, - arguments: Default::default(), - directives: Default::default(), - sibling_typename: None, - } - } - - pub(crate) fn name(&self) -> &Name { - self.field_position.field_name() - } - - pub(crate) fn response_name(&self) -> &Name { - self.alias.as_ref().unwrap_or_else(|| self.name()) - } - - pub(crate) fn output_base_type(&self) -> Result { - let definition = self.field_position.get(self.schema.schema())?; - self.schema - .get_type(definition.ty.inner_named_type().clone()) - } - - pub(crate) fn is_leaf(&self) -> Result { - let base_type_position = self.output_base_type()?; - Ok(matches!( - base_type_position, - TypeDefinitionPosition::Scalar(_) | TypeDefinitionPosition::Enum(_) - )) - } - } - - impl HasSelectionKey for FieldData { fn key(&self) -> SelectionKey<'_> { SelectionKey::Field { response_name: self.response_name(), @@ -867,7 +840,6 @@ mod field_selection { } pub(crate) use field_selection::Field; -pub(crate) use field_selection::FieldData; pub(crate) use field_selection::FieldSelection; pub(crate) use field_selection::SiblingTypename; @@ -2253,7 +2225,7 @@ impl SelectionSet { selection_map.insert(selection.clone()); } else { let updated_field = match alias { - Some(alias) => field.with_updated_alias(alias.alias.clone()), + Some(alias) => field.field.with_updated_alias(alias.alias.clone()), None => field.field.clone(), }; selection_map @@ -2643,7 +2615,7 @@ fn gen_alias_name(base_name: &Name, unavailable_names: &IndexMap Self { + fn with_updated_element(&self, field: Field) -> Self { Self { - field: Field::new(element), - ..self.clone() + field, + selection_set: self.selection_set.clone(), } } diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 0a916e3017..1030f1a8d8 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -191,10 +191,10 @@ impl Field { } .into()) } else { - let mut updated_field_data = self.data().clone(); - updated_field_data.schema = schema.clone(); - updated_field_data.field_position = parent_type.introspection_typename_field(); - Ok(Field::new(updated_field_data)) + let mut updated_field = self.clone(); + updated_field.schema = schema.clone(); + updated_field.field_position = parent_type.introspection_typename_field(); + Ok(updated_field) }; } @@ -202,10 +202,10 @@ impl Field { return if field_from_parent.try_get(schema.schema()).is_some() && self.can_rebase_on(parent_type)? { - let mut updated_field_data = self.data().clone(); - updated_field_data.schema = schema.clone(); - updated_field_data.field_position = field_from_parent; - Ok(Field::new(updated_field_data)) + let mut updated_field = self.clone(); + updated_field.schema = schema.clone(); + updated_field.field_position = field_from_parent; + Ok(updated_field) } else { Err(RebaseError::CannotRebase { field_position: self.field_position.clone(), diff --git a/apollo-federation/src/operation/simplify.rs b/apollo-federation/src/operation/simplify.rs index 9c7221c07e..4e1caca2cb 100644 --- a/apollo-federation/src/operation/simplify.rs +++ b/apollo-federation/src/operation/simplify.rs @@ -6,7 +6,6 @@ use apollo_compiler::name; use super::runtime_types_intersect; use super::DirectiveList; use super::Field; -use super::FieldData; use super::FieldSelection; use super::FragmentSpreadSelection; use super::InlineFragmentSelection; @@ -62,7 +61,7 @@ impl FieldSelection { let field_element = if self.field.schema() == schema && self.field.field_position == field_position { - self.field.data().clone() + self.field.clone() } else { self.field .with_updated_position(schema.clone(), field_position) @@ -89,7 +88,7 @@ impl FieldSelection { arguments: vec![(name!("if"), false).into()], }); let non_included_typename = Selection::from_field( - Field::new(FieldData { + Field { schema: schema.clone(), field_position: field_composite_type_position .introspection_typename_field(), @@ -97,7 +96,7 @@ impl FieldSelection { arguments: Default::default(), directives, sibling_typename: None, - }), + }, None, ); let mut typename_selection = SelectionMap::new(); @@ -229,14 +228,14 @@ impl InlineFragmentSelection { parent_type.introspection_typename_field() }; let typename_field_selection = Selection::from_field( - Field::new(FieldData { + Field { schema: schema.clone(), field_position: parent_typename_field, alias: None, arguments: Default::default(), directives, sibling_typename: None, - }), + }, None, ); diff --git a/apollo-federation/src/operation/tests/mod.rs b/apollo-federation/src/operation/tests/mod.rs index 70fa203778..501069c5ea 100644 --- a/apollo-federation/src/operation/tests/mod.rs +++ b/apollo-federation/src/operation/tests/mod.rs @@ -3,6 +3,7 @@ use apollo_compiler::name; use apollo_compiler::schema::Schema; use apollo_compiler::ExecutableDocument; +use super::Field; use super::normalize_operation; use super::Name; use super::NamedFragments; @@ -1359,8 +1360,12 @@ mod lazy_map_tests { } } -fn field_element(schema: &ValidFederationSchema, object: Name, field: Name) -> OpPathElement { - OpPathElement::Field(super::Field::new(super::FieldData { +fn field_element( + schema: &ValidFederationSchema, + object: Name, + field: Name, +) -> OpPathElement { + OpPathElement::Field(Field { schema: schema.clone(), field_position: ObjectTypeDefinitionPosition::new(object) .field(field) @@ -1369,7 +1374,7 @@ fn field_element(schema: &ValidFederationSchema, object: Name, field: Name) -> O arguments: Default::default(), directives: Default::default(), sibling_typename: None, - })) + }) } const ADD_AT_PATH_TEST_SCHEMA: &str = r#" diff --git a/apollo-federation/src/query_graph/graph_path.rs b/apollo-federation/src/query_graph/graph_path.rs index 7c9fe75827..89a3fa5ed7 100644 --- a/apollo-federation/src/query_graph/graph_path.rs +++ b/apollo-federation/src/query_graph/graph_path.rs @@ -32,7 +32,6 @@ use crate::link::graphql_definition::OperationConditional; use crate::link::graphql_definition::OperationConditionalKind; use crate::operation::DirectiveList; use crate::operation::Field; -use crate::operation::FieldData; use crate::operation::HasSelectionKey; use crate::operation::InlineFragment; use crate::operation::InlineFragmentData; @@ -2503,14 +2502,14 @@ impl OpGraphPath { operation_field, edge_weight, self, ))); } - operation_field = Field::new(FieldData { + operation_field = Field { schema: self.graph.schema_by_source(&tail_weight.source)?.clone(), field_position: field_on_tail_type.into(), alias: operation_field.alias.clone(), arguments: operation_field.arguments.clone(), directives: operation_field.directives.clone(), sibling_typename: operation_field.sibling_typename.clone(), - }) + } } let field_path = self.add_field_edge( @@ -3892,7 +3891,6 @@ mod tests { use petgraph::stable_graph::NodeIndex; use crate::operation::Field; - use crate::operation::FieldData; use crate::query_graph::build_query_graph::build_query_graph; use crate::query_graph::condition_resolver::ConditionResolution; use crate::query_graph::graph_path::OpGraphPath; @@ -3928,8 +3926,8 @@ mod tests { type_name: Name::new("T").unwrap(), field_name: Name::new("t").unwrap(), }; - let data = FieldData::from_position(&schema, pos.into()); - let trigger = OpGraphPathTrigger::OpPathElement(OpPathElement::Field(Field::new(data))); + let field = Field::from_position(&schema, pos.into()); + let trigger = OpGraphPathTrigger::OpPathElement(OpPathElement::Field(field)); let path = path .add( trigger, @@ -3946,8 +3944,8 @@ mod tests { type_name: Name::new("ID").unwrap(), field_name: Name::new("id").unwrap(), }; - let data = FieldData::from_position(&schema, pos.into()); - let trigger = OpGraphPathTrigger::OpPathElement(OpPathElement::Field(Field::new(data))); + let field = Field::from_position(&schema, pos.into()); + let trigger = OpGraphPathTrigger::OpPathElement(OpPathElement::Field(field)); let path = path .add( trigger, diff --git a/apollo-federation/src/query_graph/path_tree.rs b/apollo-federation/src/query_graph/path_tree.rs index cb6b9fef18..cf758bed37 100644 --- a/apollo-federation/src/query_graph/path_tree.rs +++ b/apollo-federation/src/query_graph/path_tree.rs @@ -480,7 +480,6 @@ mod tests { use crate::error::FederationError; use crate::operation::normalize_operation; use crate::operation::Field; - use crate::operation::FieldData; use crate::query_graph::build_query_graph::build_query_graph; use crate::query_graph::condition_resolver::ConditionResolution; use crate::query_graph::graph_path::OpGraphPath; @@ -546,7 +545,7 @@ mod tests { .unwrap(); // build the trigger for the edge - let data = FieldData { + let field = Field { schema: query_graph.schema().unwrap().clone(), field_position: field_def.clone(), alias: None, @@ -554,7 +553,7 @@ mod tests { directives: Default::default(), sibling_typename: None, }; - let trigger = OpGraphPathTrigger::OpPathElement(OpPathElement::Field(Field::new(data))); + let trigger = OpGraphPathTrigger::OpPathElement(OpPathElement::Field(field)); // add the edge to the path graph_path = graph_path diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index 085f6e0a08..9e0c79289c 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -36,7 +36,6 @@ use crate::operation::ArgumentList; use crate::operation::ContainmentOptions; use crate::operation::DirectiveList; use crate::operation::Field; -use crate::operation::FieldData; use crate::operation::InlineFragment; use crate::operation::InlineFragmentData; use crate::operation::InlineFragmentSelection; @@ -587,7 +586,6 @@ impl FetchDependencyGraphNodePath { fn advance_field_type(&self, element: &Field) -> Result, FederationError> { if !element - .data() .output_base_type() .map(|base_type| base_type.is_composite_type()) .unwrap_or_default() @@ -2952,7 +2950,7 @@ fn operation_for_entities_fetch( let entities = FieldDefinitionPosition::Object(query_type.field(ENTITIES_QUERY.clone())); let entities_call = Selection::from_element( - OpPathElement::Field(Field::new(FieldData { + OpPathElement::Field(Field { schema: subgraph_schema.clone(), field_position: entities, alias: None, @@ -2962,7 +2960,7 @@ fn operation_for_entities_fetch( )), directives: Default::default(), sibling_typename: None, - })), + }), Some(selection_set), )?; @@ -4770,7 +4768,7 @@ mod tests { object: Name, field: Name, ) -> OpPathElement { - OpPathElement::Field(super::Field::new(FieldData { + OpPathElement::Field(Field { schema: schema.clone(), field_position: ObjectTypeDefinitionPosition::new(object) .field(field) @@ -4779,7 +4777,7 @@ mod tests { arguments: Default::default(), directives: Default::default(), sibling_typename: None, - })) + }) } fn inline_fragment_element( From 68f3ececa73af7dce69150d0094d94c7fd6d0c88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 27 Sep 2024 10:28:00 +0200 Subject: [PATCH 08/17] Inline InlineFragmentData::can_rebase_on This has the same name as InlineFragment::can_rebase_on, which will be a problem when I merge the two structures. It's only used in one place: computing the parent type of the inline fragment's subselection when it is being rebased. I inlined the function and I think it's more readable this way as well. --- apollo-federation/src/operation/rebase.rs | 25 ++++++----------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 1030f1a8d8..8180d59630 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -517,29 +517,16 @@ impl InlineFragmentData { if self.schema == *schema && self.parent_type_position == *parent_type { return Some(self.casted_type()); } - match self.can_rebase_on(parent_type, schema) { - (false, _) => None, - (true, None) => Some(parent_type.clone()), - (true, Some(ty)) => Some(ty), - } - } - - fn can_rebase_on( - &self, - parent_type: &CompositeTypeDefinitionPosition, - schema: &ValidFederationSchema, - ) -> (bool, Option) { let Some(ty) = self.type_condition_position.as_ref() else { - return (true, None); + return Some(parent_type.clone()); }; - match schema + + let rebased_type = schema .get_type(ty.type_name().clone()) .ok() - .and_then(|ty| CompositeTypeDefinitionPosition::try_from(ty).ok()) - { - Some(ty) if runtime_types_intersect(parent_type, &ty, schema) => (true, Some(ty)), - _ => (false, None), - } + .and_then(|ty| CompositeTypeDefinitionPosition::try_from(ty).ok())?; + + runtime_types_intersect(parent_type, &rebased_type, schema).then_some(rebased_type) } } From c2da90c650176fd46eca7a9a99d04219bd3575c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 27 Sep 2024 10:35:47 +0200 Subject: [PATCH 09/17] Merge InlineFragment and InlineFragmentData --- apollo-federation/src/operation/mod.rs | 105 ++++++------------ apollo-federation/src/operation/rebase.rs | 19 ++-- .../src/operation/selection_map.rs | 6 +- .../src/query_graph/graph_path.rs | 66 +++++------ .../src/query_plan/fetch_dependency_graph.rs | 13 +-- 5 files changed, 78 insertions(+), 131 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 7ab49430a7..f8c02c5283 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -1004,13 +1004,13 @@ impl FragmentSpreadSelection { parent_type_position: CompositeTypeDefinitionPosition, predicate: &mut impl FnMut(OpPathElement) -> Result, ) -> Result { - let inline_fragment = InlineFragment::new(InlineFragmentData { + let inline_fragment = InlineFragment { schema: self.spread.schema.clone(), parent_type_position, type_condition_position: Some(self.spread.type_condition_position.clone()), directives: self.spread.directives.clone(), selection_id: self.spread.selection_id.clone(), - }); + }; if predicate(inline_fragment.into())? { return Ok(true); } @@ -1037,7 +1037,6 @@ impl FragmentSpreadData { mod inline_fragment_selection { use std::hash::Hash; use std::hash::Hasher; - use std::ops::Deref; use serde::Serialize; @@ -1103,15 +1102,16 @@ mod inline_fragment_selection { /// The non-selection-set data of `InlineFragmentSelection`, used with operation paths and /// graph paths. - #[derive(Clone, Serialize)] + #[derive(Debug, Clone, Serialize)] pub(crate) struct InlineFragment { - data: InlineFragmentData, - } - - impl std::fmt::Debug for InlineFragment { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.data.fmt(f) - } + #[serde(skip)] + pub(crate) schema: ValidFederationSchema, + pub(crate) parent_type_position: CompositeTypeDefinitionPosition, + pub(crate) type_condition_position: Option, + #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] + pub(crate) directives: DirectiveList, + #[cfg_attr(not(feature = "snapshot_tracing"), serde(skip))] + pub(crate) selection_id: SelectionId, } impl PartialEq for InlineFragment { @@ -1128,42 +1128,26 @@ mod inline_fragment_selection { } } - impl Deref for InlineFragment { - type Target = InlineFragmentData; - - fn deref(&self) -> &Self::Target { - &self.data - } - } - impl InlineFragment { - pub(crate) fn new(data: InlineFragmentData) -> Self { - Self { data } - } - pub(crate) fn schema(&self) -> &ValidFederationSchema { - &self.data.schema - } - - pub(crate) fn data(&self) -> &InlineFragmentData { - &self.data + &self.schema } pub(crate) fn with_updated_type_condition( &self, - new: Option, + type_condition_position: Option, ) -> Self { - let mut data = self.data().clone(); - data.type_condition_position = new; - Self::new(data) + Self { + type_condition_position, + ..self.clone() + } } - pub(crate) fn with_updated_directives( - &self, - directives: impl Into, - ) -> InlineFragment { - let mut data = self.data().clone(); - data.directives = directives.into(); - Self::new(data) + + pub(crate) fn with_updated_directives(&self, directives: impl Into) -> Self { + Self { + directives: directives.into(), + ..self.clone() + } } pub(crate) fn as_path_element(&self) -> Option { @@ -1173,27 +1157,7 @@ mod inline_fragment_selection { condition.type_name().clone(), )) } - } - - impl HasSelectionKey for InlineFragment { - fn key(&self) -> SelectionKey<'_> { - self.data.key() - } - } - - #[derive(Debug, Clone, Serialize)] - pub(crate) struct InlineFragmentData { - #[serde(skip)] - pub(crate) schema: ValidFederationSchema, - pub(crate) parent_type_position: CompositeTypeDefinitionPosition, - pub(crate) type_condition_position: Option, - #[serde(serialize_with = "crate::display_helpers::serialize_as_string")] - pub(crate) directives: DirectiveList, - #[cfg_attr(not(feature = "snapshot_tracing"), serde(skip))] - pub(crate) selection_id: SelectionId, - } - impl InlineFragmentData { pub(crate) fn defer_directive_arguments( &self, ) -> Result, FederationError> { @@ -1211,7 +1175,7 @@ mod inline_fragment_selection { } } - impl HasSelectionKey for InlineFragmentData { + impl HasSelectionKey for InlineFragment { fn key(&self) -> SelectionKey<'_> { if is_deferred_selection(&self.directives) { SelectionKey::Defer { @@ -1231,7 +1195,6 @@ mod inline_fragment_selection { } pub(crate) use inline_fragment_selection::InlineFragment; -pub(crate) use inline_fragment_selection::InlineFragmentData; pub(crate) use inline_fragment_selection::InlineFragmentSelection; use self::selection_map::OwnedSelectionKey; @@ -2757,13 +2720,13 @@ impl InlineFragmentSelection { }; let new_selection_set = SelectionSet::from_selection_set(&inline_fragment.selection_set, fragments, schema)?; - let new_inline_fragment = InlineFragment::new(InlineFragmentData { + let new_inline_fragment = InlineFragment { schema: schema.clone(), parent_type_position: parent_type_position.clone(), type_condition_position, directives: inline_fragment.directives.clone().into(), selection_id: SelectionId::new(), - }); + }; Ok(InlineFragmentSelection::new( new_inline_fragment, new_selection_set, @@ -2796,7 +2759,7 @@ impl InlineFragmentSelection { // Note: We assume that fragment_spread_selection.spread.type_condition_position is the same as // fragment_spread_selection.selection_set.type_position. Ok(InlineFragmentSelection::new( - InlineFragment::new(InlineFragmentData { + InlineFragment { schema: fragment_spread_selection.spread.schema.clone(), parent_type_position, type_condition_position: Some( @@ -2807,7 +2770,7 @@ impl InlineFragmentSelection { ), directives: fragment_spread_selection.spread.directives.clone(), selection_id: SelectionId::new(), - }), + }, fragment_spread_selection .selection_set .expand_all_fragments()?, @@ -2821,14 +2784,14 @@ impl InlineFragmentSelection { selection_set: SelectionSet, directives: DirectiveList, ) -> Self { - let inline_fragment_data = InlineFragmentData { + let inline_fragment_data = InlineFragment { schema: selection_set.schema.clone(), parent_type_position, type_condition_position: selection_set.type_position.clone().into(), directives, selection_id: SelectionId::new(), }; - InlineFragmentSelection::new(InlineFragment::new(inline_fragment_data), selection_set) + InlineFragmentSelection::new(inline_fragment_data, selection_set) } pub(crate) fn casted_type(&self) -> &CompositeTypeDefinitionPosition { @@ -3074,7 +3037,7 @@ impl DeferNormalizer { let mut stack = selection_set.into_iter().collect::>(); while let Some(selection) = stack.pop() { if let Selection::InlineFragment(inline) = selection { - if let Some(args) = inline.inline_fragment.data().defer_directive_arguments()? { + if let Some(args) = inline.inline_fragment.defer_directive_arguments()? { let DeferDirectiveArguments { label, if_: _ } = args; if let Some(label) = label { digest.used_labels.insert(label); @@ -3207,9 +3170,9 @@ impl InlineFragment { } fn without_defer(&self, filter: DeferFilter<'_>) -> Result { - let mut data = self.data().clone(); - filter.remove_defer(&mut data.directives, data.schema.schema()); - Ok(Self::new(data)) + let mut without_defer = self.clone(); + filter.remove_defer(&mut without_defer.directives, without_defer.schema.schema()); + Ok(without_defer) } } diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 8180d59630..2f7598e726 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -14,7 +14,6 @@ use super::FragmentSpread; use super::FragmentSpreadData; use super::FragmentSpreadSelection; use super::InlineFragment; -use super::InlineFragmentData; use super::InlineFragmentSelection; use super::NamedFragments; use super::OperationElement; @@ -474,13 +473,13 @@ impl FragmentSpreadSelection { Err(RebaseError::EmptySelectionSet.into()) } else { Ok(InlineFragmentSelection::new( - InlineFragment::new(InlineFragmentData { + InlineFragment { schema: schema.clone(), parent_type_position: parent_type.clone(), type_condition_position: None, directives: Default::default(), selection_id: SelectionId::new(), - }), + }, expanded_selection_set, ) .into()) @@ -508,7 +507,7 @@ impl FragmentSpreadSelection { } } -impl InlineFragmentData { +impl InlineFragment { fn casted_type_if_add_to( &self, parent_type: &CompositeTypeDefinitionPosition, @@ -528,9 +527,7 @@ impl InlineFragmentData { runtime_types_intersect(parent_type, &rebased_type, schema).then_some(rebased_type) } -} -impl InlineFragment { pub(crate) fn rebase_on( &self, parent_type: &CompositeTypeDefinitionPosition, @@ -553,11 +550,11 @@ impl InlineFragment { } .into()) } else { - let mut rebased_fragment_data = self.data().clone(); - rebased_fragment_data.parent_type_position = parent_type.clone(); - rebased_fragment_data.type_condition_position = rebased_condition; - rebased_fragment_data.schema = schema.clone(); - Ok(InlineFragment::new(rebased_fragment_data)) + let mut rebased_fragment = self.clone(); + rebased_fragment.parent_type_position = parent_type.clone(); + rebased_fragment.type_condition_position = rebased_condition; + rebased_fragment.schema = schema.clone(); + Ok(rebased_fragment) } } diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index 92d0b1e39f..f2c4cdae99 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -220,10 +220,8 @@ impl Default for SelectionMap { } pub(crate) type Values<'a> = std::slice::Iter<'a, Selection>; -pub(crate) type ValuesMut<'a> = std::iter::Map< - std::slice::IterMut<'a, Selection>, - fn(&'a mut Selection) -> SelectionValue<'a>, ->; +pub(crate) type ValuesMut<'a> = + std::iter::Map, fn(&'a mut Selection) -> SelectionValue<'a>>; pub(crate) type IntoValues = std::vec::IntoIter; /// Return an equality function taking an index into `selections` and returning if the index diff --git a/apollo-federation/src/query_graph/graph_path.rs b/apollo-federation/src/query_graph/graph_path.rs index 89a3fa5ed7..a3d0476d56 100644 --- a/apollo-federation/src/query_graph/graph_path.rs +++ b/apollo-federation/src/query_graph/graph_path.rs @@ -34,7 +34,6 @@ use crate::operation::DirectiveList; use crate::operation::Field; use crate::operation::HasSelectionKey; use crate::operation::InlineFragment; -use crate::operation::InlineFragmentData; use crate::operation::SelectionId; use crate::operation::SelectionKey; use crate::operation::SelectionSet; @@ -2695,19 +2694,15 @@ impl OpGraphPath { debug!("Handling implementation {implementation_type_pos}"); let span = debug_span!(" |"); let guard = span.enter(); - let implementation_inline_fragment = - InlineFragment::new(InlineFragmentData { - schema: self - .graph - .schema_by_source(&tail_weight.source)? - .clone(), - parent_type_position: tail_type_pos.clone().into(), - type_condition_position: Some( - implementation_type_pos.clone().into(), - ), - directives: Default::default(), - selection_id: SelectionId::new(), - }); + let implementation_inline_fragment = InlineFragment { + schema: self.graph.schema_by_source(&tail_weight.source)?.clone(), + parent_type_position: tail_type_pos.clone().into(), + type_condition_position: Some( + implementation_type_pos.clone().into(), + ), + directives: Default::default(), + selection_id: SelectionId::new(), + }; let implementation_options = SimultaneousPathsWithLazyIndirectPaths::new( self.clone().into(), @@ -2902,19 +2897,15 @@ impl OpGraphPath { for implementation_type_pos in intersection { let span = debug_span!("Trying {implementation_type_pos}"); let guard = span.enter(); - let implementation_inline_fragment = - InlineFragment::new(InlineFragmentData { - schema: self - .graph - .schema_by_source(&tail_weight.source)? - .clone(), - parent_type_position: tail_type_pos.clone().into(), - type_condition_position: Some( - implementation_type_pos.clone().into(), - ), - directives: operation_inline_fragment.directives.clone(), - selection_id: SelectionId::new(), - }); + let implementation_inline_fragment = InlineFragment { + schema: self.graph.schema_by_source(&tail_weight.source)?.clone(), + parent_type_position: tail_type_pos.clone().into(), + type_condition_position: Some( + implementation_type_pos.clone().into(), + ), + directives: operation_inline_fragment.directives.clone(), + selection_id: SelectionId::new(), + }; let implementation_options = SimultaneousPathsWithLazyIndirectPaths::new( self.clone().into(), @@ -2988,17 +2979,16 @@ impl OpGraphPath { if operation_inline_fragment.directives.is_empty() { return Ok((Some(vec![self.clone().into()]), None)); } - let operation_inline_fragment = - InlineFragment::new(InlineFragmentData { - schema: self - .graph - .schema_by_source(&tail_weight.source)? - .clone(), - parent_type_position: tail_type_pos.clone().into(), - type_condition_position: None, - directives: operation_inline_fragment.directives.clone(), - selection_id: SelectionId::new(), - }); + let operation_inline_fragment = InlineFragment { + schema: self + .graph + .schema_by_source(&tail_weight.source)? + .clone(), + parent_type_position: tail_type_pos.clone().into(), + type_condition_position: None, + directives: operation_inline_fragment.directives.clone(), + selection_id: SelectionId::new(), + }; let defer_directive_arguments = operation_inline_fragment.defer_directive_arguments()?; let fragment_path = self.add( diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index 9e0c79289c..343945ce03 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -37,7 +37,6 @@ use crate::operation::ContainmentOptions; use crate::operation::DirectiveList; use crate::operation::Field; use crate::operation::InlineFragment; -use crate::operation::InlineFragmentData; use crate::operation::InlineFragmentSelection; use crate::operation::Operation; use crate::operation::Selection; @@ -3895,13 +3894,13 @@ fn wrap_selection_with_type_and_conditions( // PORT_NOTE: JS code looks for type condition in the wrapping type's schema based on // the name of wrapping type. Not sure why. return wrap_in_fragment( - InlineFragment::new(InlineFragmentData { + InlineFragment { schema: supergraph_schema.clone(), parent_type_position: wrapping_type.clone(), type_condition_position: Some(type_condition.clone()), directives: Default::default(), // None selection_id: SelectionId::new(), - }), + }, initial, ); } @@ -3922,13 +3921,13 @@ fn wrap_selection_with_type_and_conditions( .into()], }; wrap_in_fragment( - InlineFragment::new(InlineFragmentData { + InlineFragment { schema: supergraph_schema.clone(), parent_type_position: wrapping_type.clone(), type_condition_position: Some(type_condition.clone()), directives: [directive].into_iter().collect(), selection_id: SelectionId::new(), - }), + }, acc, ) }) @@ -4792,13 +4791,13 @@ mod tests { .unwrap(); let type_condition = type_condition_name.map(|n| schema.get_type(n).unwrap().try_into().unwrap()); - OpPathElement::InlineFragment(super::InlineFragment::new(InlineFragmentData { + OpPathElement::InlineFragment(InlineFragment { schema: schema.clone(), parent_type_position: parent_type, type_condition_position: type_condition, directives: Default::default(), selection_id: SelectionId::new(), - })) + }) } fn to_string(response_path: &[FetchDataPathElement]) -> String { From 1cf815711fd77c6550c76a2f763350a5aa269a3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 27 Sep 2024 10:38:55 +0200 Subject: [PATCH 10/17] Merge FragmentSpread and FragmentSpreadData --- apollo-federation/src/operation/mod.rs | 82 +++++++-------------- apollo-federation/src/operation/optimize.rs | 7 +- apollo-federation/src/operation/rebase.rs | 10 +-- 3 files changed, 32 insertions(+), 67 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index f8c02c5283..a185615e9b 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -871,51 +871,8 @@ mod fragment_spread_selection { /// An analogue of the apollo-compiler type `FragmentSpread` with these changes: /// - Stores the schema (may be useful for directives). /// - Encloses collection types in `Arc`s to facilitate cheaper cloning. - #[derive(Clone, Serialize)] - pub(crate) struct FragmentSpread { - data: FragmentSpreadData, - } - - impl std::fmt::Debug for FragmentSpread { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.data.fmt(f) - } - } - - impl PartialEq for FragmentSpread { - fn eq(&self, other: &Self) -> bool { - self.key() == other.key() - } - } - - impl Eq for FragmentSpread {} - - impl Deref for FragmentSpread { - type Target = FragmentSpreadData; - - fn deref(&self) -> &Self::Target { - &self.data - } - } - - impl FragmentSpread { - pub(crate) fn new(data: FragmentSpreadData) -> Self { - Self { data } - } - - pub(crate) fn data(&self) -> &FragmentSpreadData { - &self.data - } - } - - impl HasSelectionKey for FragmentSpread { - fn key(&self) -> SelectionKey<'_> { - self.data.key() - } - } - #[derive(Debug, Clone, Serialize)] - pub(crate) struct FragmentSpreadData { + pub(crate) struct FragmentSpread { #[serde(skip)] pub(crate) schema: ValidFederationSchema, pub(crate) fragment_name: Name, @@ -934,7 +891,21 @@ mod fragment_spread_selection { pub(crate) selection_id: SelectionId, } - impl HasSelectionKey for FragmentSpreadData { + impl PartialEq for FragmentSpread { + fn eq(&self, other: &Self) -> bool { + self.key() == other.key() + } + } + + impl Eq for FragmentSpread {} + + impl FragmentSpread { + pub(super) fn directives_mut(&mut self) -> &mut DirectiveList { + &mut self.directives + } + } + + impl HasSelectionKey for FragmentSpread { fn key(&self) -> SelectionKey<'_> { if is_deferred_selection(&self.directives) { SelectionKey::Defer { @@ -951,7 +922,6 @@ mod fragment_spread_selection { } pub(crate) use fragment_spread_selection::FragmentSpread; -pub(crate) use fragment_spread_selection::FragmentSpreadData; pub(crate) use fragment_spread_selection::FragmentSpreadSelection; impl FragmentSpreadSelection { @@ -964,9 +934,9 @@ impl FragmentSpreadSelection { fragment_spread: &executable::FragmentSpread, fragment: &Node, ) -> Result { - let spread_data = FragmentSpreadData::from_fragment(fragment, &fragment_spread.directives); + let spread = FragmentSpread::from_fragment(fragment, &fragment_spread.directives); Ok(FragmentSpreadSelection { - spread: FragmentSpread::new(spread_data), + spread, selection_set: fragment.selection_set.clone(), }) } @@ -975,9 +945,9 @@ impl FragmentSpreadSelection { fragment: &Node, directives: &executable::DirectiveList, ) -> Self { - let spread_data = FragmentSpreadData::from_fragment(fragment, directives); + let spread = FragmentSpread::from_fragment(fragment, directives); Self { - spread: FragmentSpread::new(spread_data), + spread, selection_set: fragment.selection_set.clone(), } } @@ -1018,12 +988,12 @@ impl FragmentSpreadSelection { } } -impl FragmentSpreadData { +impl FragmentSpread { pub(crate) fn from_fragment( fragment: &Node, spread_directives: &executable::DirectiveList, - ) -> FragmentSpreadData { - FragmentSpreadData { + ) -> FragmentSpread { + FragmentSpread { schema: fragment.schema.clone(), fragment_name: fragment.name.clone(), type_condition_position: fragment.type_condition_position.clone(), @@ -3151,9 +3121,9 @@ impl FragmentSpread { } fn without_defer(&self, filter: DeferFilter<'_>) -> Result { - let mut data = self.data().clone(); - filter.remove_defer(&mut data.directives, data.schema.schema()); - Ok(Self::new(data)) + let mut without_defer = self.clone(); + filter.remove_defer(&mut without_defer.directives, without_defer.schema.schema()); + Ok(without_defer) } } diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 37ba8a0be6..3854359ff6 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -61,7 +61,6 @@ use super::SelectionOrSet; use super::SelectionSet; use crate::error::FederationError; use crate::operation::FragmentSpread; -use crate::operation::FragmentSpreadData; use crate::operation::SelectionValue; use crate::schema::position::CompositeTypeDefinitionPosition; @@ -1385,7 +1384,7 @@ impl InlineFragmentSelection { // case they should be kept on the spread. // PORT_NOTE: We are assuming directives on fragment definitions are // carried over to their spread sites as JS version does, which - // is handled differently in Rust version (see `FragmentSpreadData`). + // is handled differently in Rust version (see `FragmentSpread`). let directives: executable::DirectiveList = self .inline_fragment .directives @@ -1752,14 +1751,14 @@ impl FragmentGenerator { }; new_selection_set.add_local_selection(&Selection::from( FragmentSpreadSelection { - spread: FragmentSpread::new(FragmentSpreadData { + spread: FragmentSpread { schema: selection_set.schema.clone(), fragment_name: existing.name.clone(), type_condition_position: existing.type_condition_position.clone(), directives: skip_include.into(), fragment_directives: existing.directives.clone(), selection_id: crate::operation::SelectionId::new(), - }), + }, selection_set: existing.selection_set.clone(), }, ))?; diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 2f7598e726..0fa734a8ed 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -11,7 +11,6 @@ use super::Field; use super::FieldSelection; use super::Fragment; use super::FragmentSpread; -use super::FragmentSpreadData; use super::FragmentSpreadSelection; use super::InlineFragment; use super::InlineFragmentSelection; @@ -389,10 +388,10 @@ impl FragmentSpread { &named_fragment.type_condition_position, &self.schema, ) { - Ok(FragmentSpread::new(FragmentSpreadData::from_fragment( + Ok(FragmentSpread::from_fragment( named_fragment, &self.directives, - ))) + )) } else { Err(RebaseError::NonIntersectingCondition { type_condition: named_fragment.type_condition_position.clone().into(), @@ -486,10 +485,7 @@ impl FragmentSpreadSelection { }; } - let spread = FragmentSpread::new(FragmentSpreadData::from_fragment( - named_fragment, - &self.spread.directives, - )); + let spread = FragmentSpread::from_fragment(named_fragment, &self.spread.directives); Ok(FragmentSpreadSelection { spread, selection_set: named_fragment.selection_set.clone(), From c199104f726e680352456bba3d186afc0becd50c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 27 Sep 2024 14:17:53 +0200 Subject: [PATCH 11/17] cleanup --- apollo-federation/src/operation/optimize.rs | 9 +++++---- .../src/operation/selection_map.rs | 19 ++++++++++++------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 3854359ff6..c397d713a8 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -682,10 +682,11 @@ impl FragmentRestrictionAtType { // Using `F` in those cases is, while not 100% incorrect, at least not productive, and so we // skip it that case. This is essentially an optimization. fn is_useless(&self) -> bool { - match self.selections.selections.as_slice().split_first() { - None => true, - Some((first, rest)) => rest.is_empty() && first.is_typename_field(), - } + let mut iter = self.selections.iter(); + let Some(first) = iter.next() else { + return true; + }; + iter.next().is_none() && first.is_typename_field() } } diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index f2c4cdae99..f0939d9e83 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -233,6 +233,7 @@ fn key_eq<'a>(selections: &'a [Selection], key: SelectionKey<'a>) -> impl Fn(&Bu } impl SelectionMap { + /// Create an empty selection map. pub(crate) fn new() -> Self { SelectionMap { hash_builder: Default::default(), @@ -361,7 +362,9 @@ impl SelectionMap { self.selections.into_iter() } - pub(super) fn entry<'a>(&'a mut self, key: SelectionKey<'_>) -> Entry<'a> { + /// Provides mutable access to a selection key. A new selection can be inserted or an existing + /// selection modified. + pub(super) fn entry<'a>(&'a mut self, key: SelectionKey<'a>) -> Entry<'a> { let hash = self.hash(key); let slot = self.table.find_entry(hash, key_eq(&self.selections, key)); match slot { @@ -378,22 +381,22 @@ impl SelectionMap { } } + /// Add selections from another selection map to this one. If there are key collisions, the + /// selections are *overwritten*. pub(crate) fn extend(&mut self, other: SelectionMap) { for selection in other.into_values() { self.insert(selection); } } + /// Add selections from another selection map to this one. If there are key collisions, the + /// selections are *overwritten*. pub(crate) fn extend_ref(&mut self, other: &SelectionMap) { for selection in other.values() { self.insert(selection.clone()); } } - pub(crate) fn as_slice(&self) -> &[Selection] { - &self.selections - } - /// Returns the selection set resulting from "recursively" filtering any selection /// that does not match the provided predicate. /// This method calls `predicate` on every selection of the selection set, @@ -453,7 +456,7 @@ impl SelectionMap { } // Clone the map so far - new_map = self.as_slice()[..index].iter().cloned().collect(); + new_map = self.selections[..index].iter().cloned().collect(); if keep { new_map.insert(filtered.into_owned()); @@ -474,6 +477,8 @@ impl FromIterator for SelectionMap where A: Into, { + /// Create a selection map from an iterator of selections. On key collisions, *only the later + /// selection is used*. fn from_iter>(iter: T) -> Self { let mut map = Self::new(); for selection in iter { @@ -516,7 +521,7 @@ impl<'a> SelectionValue<'a> { } } - pub(super) fn get_selection_set_mut(&mut self) -> Option<&'_ mut SelectionSet> { + pub(super) fn get_selection_set_mut(&mut self) -> Option<&mut SelectionSet> { match self { SelectionValue::Field(field) => field.get_selection_set_mut(), SelectionValue::FragmentSpread(frag) => Some(frag.get_selection_set_mut()), From d7b2d925fb58aab4728b24969fcd3577e1ecded2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 30 Sep 2024 17:38:18 +0200 Subject: [PATCH 12/17] clippy --- apollo-federation/src/operation/merging.rs | 2 +- apollo-federation/src/operation/mod.rs | 2 +- apollo-federation/src/operation/optimize.rs | 10 ++-------- apollo-federation/src/operation/rebase.rs | 4 ++-- apollo-federation/src/operation/simplify.rs | 2 +- 5 files changed, 7 insertions(+), 13 deletions(-) diff --git a/apollo-federation/src/operation/merging.rs b/apollo-federation/src/operation/merging.rs index d45fdfd4a4..cdc6898373 100644 --- a/apollo-federation/src/operation/merging.rs +++ b/apollo-federation/src/operation/merging.rs @@ -183,7 +183,7 @@ impl SelectionSet { 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()) { + match target.entry(other_key) { selection_map::Entry::Occupied(existing) => match existing.get() { Selection::Field(self_field_selection) => { let Selection::Field(other_field_selection) = other_selection else { diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index a185615e9b..2653c69894 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -979,7 +979,7 @@ impl FragmentSpreadSelection { parent_type_position, type_condition_position: Some(self.spread.type_condition_position.clone()), directives: self.spread.directives.clone(), - selection_id: self.spread.selection_id.clone(), + selection_id: self.spread.selection_id, }; if predicate(inline_fragment.into())? { return Ok(true); diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index c397d713a8..c7e54b23f7 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -228,10 +228,7 @@ impl Selection { let diff = self_sub_selection.minus(other_sub_selection)?; if !diff.is_empty() { return self - .with_updated_selections( - self_sub_selection.type_position.clone(), - diff.into_iter(), - ) + .with_updated_selections(self_sub_selection.type_position.clone(), diff) .map(Some); } } @@ -251,10 +248,7 @@ impl Selection { return Ok(None); } else { return self - .with_updated_selections( - self_sub_selection.type_position.clone(), - common.into_iter(), - ) + .with_updated_selections(self_sub_selection.type_position.clone(), common) .map(Some); } } diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 0fa734a8ed..ffc16d1b01 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -197,7 +197,7 @@ impl Field { } let field_from_parent = parent_type.field(self.name().clone())?; - return if field_from_parent.try_get(schema.schema()).is_some() + if field_from_parent.try_get(schema.schema()).is_some() && self.can_rebase_on(parent_type)? { let mut updated_field = self.clone(); @@ -210,7 +210,7 @@ impl Field { parent_type: parent_type.clone(), } .into()) - }; + } } /// Verifies whether given field can be rebase on following parent type. diff --git a/apollo-federation/src/operation/simplify.rs b/apollo-federation/src/operation/simplify.rs index 4e1caca2cb..d3996ad100 100644 --- a/apollo-federation/src/operation/simplify.rs +++ b/apollo-federation/src/operation/simplify.rs @@ -295,7 +295,7 @@ impl InlineFragmentSelection { // Otherwise, if there are "liftable" selections, we must return a set comprised of those lifted selection, // and the current fragment _without_ those lifted selections. - if liftable_selections.len() > 0 { + if !liftable_selections.is_empty() { // Converting `... [on T] { }` into // `{ ... [on T] { } }`. // PORT_NOTE: It appears that this lifting could be repeatable (meaning lifted From 6c6e554764920d409235bc495a572776eb3bdfc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 14 Oct 2024 10:54:25 +0200 Subject: [PATCH 13/17] Fixup split_top_level_fields post merge --- apollo-federation/src/operation/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 2653c69894..21d9b9c6ef 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -1235,7 +1235,7 @@ impl SelectionSet { // one :( struct TopLevelFieldSplitter { parent_type: CompositeTypeDefinitionPosition, - starting_set: ::IntoIter, + starting_set: selection_map::IntoValues, stack: Vec<(OpPathElement, Self)>, } @@ -1243,7 +1243,7 @@ impl SelectionSet { fn new(selection_set: SelectionSet) -> Self { Self { parent_type: selection_set.type_position, - starting_set: Arc::unwrap_or_clone(selection_set.selections).into_iter(), + starting_set: Arc::unwrap_or_clone(selection_set.selections).into_values(), stack: Vec::new(), } } @@ -1256,7 +1256,7 @@ impl SelectionSet { loop { match self.stack.last_mut() { None => { - let selection = self.starting_set.next()?.1; + let selection = self.starting_set.next()?; if selection.is_field() { return Some(SelectionSet::from_selection( self.parent_type.clone(), From 354beb28b0b8e6156ba64885e6cf1d5a89437e60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 15 Oct 2024 15:31:25 +0200 Subject: [PATCH 14/17] fmt --- apollo-federation/src/operation/selection_map.rs | 11 ++++++++--- apollo-federation/src/operation/tests/mod.rs | 8 ++------ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index f0939d9e83..3adb3c5299 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -287,7 +287,8 @@ impl SelectionMap { fn raw_insert(&mut self, hash: u64, value: Selection) -> &mut Selection { let index = self.selections.len(); - self.table.insert_unique(hash, Bucket { index, hash }, |existing| existing.hash); + self.table + .insert_unique(hash, Bucket { index, hash }, |existing| existing.hash); self.selections.push(value); &mut self.selections[index] @@ -302,7 +303,8 @@ impl SelectionMap { self.table.clear(); for (index, selection) in self.selections.iter().enumerate() { let hash = self.hash(selection.key()); - self.table.insert_unique(hash, Bucket { index, hash }, |existing| existing.hash); + self.table + .insert_unique(hash, Bucket { index, hash }, |existing| existing.hash); } } @@ -333,7 +335,10 @@ impl SelectionMap { Some((bucket.index, selection)) } - pub(crate) fn retain(&mut self, mut predicate: impl FnMut(SelectionKey<'_>, &Selection) -> bool) { + pub(crate) fn retain( + &mut self, + mut predicate: impl FnMut(SelectionKey<'_>, &Selection) -> bool, + ) { self.selections.retain(|selection| { let key = selection.key(); predicate(key, selection) diff --git a/apollo-federation/src/operation/tests/mod.rs b/apollo-federation/src/operation/tests/mod.rs index 501069c5ea..6988b8e659 100644 --- a/apollo-federation/src/operation/tests/mod.rs +++ b/apollo-federation/src/operation/tests/mod.rs @@ -3,8 +3,8 @@ use apollo_compiler::name; use apollo_compiler::schema::Schema; use apollo_compiler::ExecutableDocument; -use super::Field; use super::normalize_operation; +use super::Field; use super::Name; use super::NamedFragments; use super::Operation; @@ -1360,11 +1360,7 @@ mod lazy_map_tests { } } -fn field_element( - schema: &ValidFederationSchema, - object: Name, - field: Name, -) -> OpPathElement { +fn field_element(schema: &ValidFederationSchema, object: Name, field: Name) -> OpPathElement { OpPathElement::Field(Field { schema: schema.clone(), field_position: ObjectTypeDefinitionPosition::new(object) From cbf71d7c807f1cca2ab84382fc85ba5cda3392bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 15 Oct 2024 15:42:24 +0200 Subject: [PATCH 15/17] unused-ness --- apollo-federation/src/operation/mod.rs | 18 +----------------- .../src/operation/selection_map.rs | 8 +++++++- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 21d9b9c6ef..d96079ce6c 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -630,13 +630,6 @@ mod field_selection { selection_set, } } - - pub(crate) fn with_updated_directives(&self, directives: impl Into) -> Self { - Self { - field: self.field.with_updated_directives(directives), - selection_set: self.selection_set.clone(), - } - } } // SiblingTypename indicates how the sibling __typename field should be restored. @@ -692,6 +685,7 @@ mod field_selection { impl Field { /// Create a trivial field selection without any arguments or directives. + #[cfg(test)] pub(crate) fn from_position( schema: &ValidFederationSchema, field_position: FieldDefinitionPosition, @@ -742,10 +736,6 @@ mod field_selection { && self.alias.is_none() } - pub(super) fn directives_mut(&mut self) -> &mut DirectiveList { - &mut self.directives - } - pub(crate) fn sibling_typename(&self) -> Option<&SiblingTypename> { self.sibling_typename.as_ref() } @@ -899,12 +889,6 @@ mod fragment_spread_selection { impl Eq for FragmentSpread {} - impl FragmentSpread { - pub(super) fn directives_mut(&mut self) -> &mut DirectiveList { - &mut self.directives - } - } - impl HasSelectionKey for FragmentSpread { fn key(&self) -> SelectionKey<'_> { if is_deferred_selection(&self.directives) { diff --git a/apollo-federation/src/operation/selection_map.rs b/apollo-federation/src/operation/selection_map.rs index 3adb3c5299..19857ba274 100644 --- a/apollo-federation/src/operation/selection_map.rs +++ b/apollo-federation/src/operation/selection_map.rs @@ -378,7 +378,10 @@ impl SelectionMap { let selection = &mut self.selections[index]; Entry::Occupied(OccupiedEntry(selection)) } - Err(vacant) => Entry::Vacant(VacantEntry { + // We're not using `hashbrown`'s VacantEntry API here, because we have some custom + // insertion logic, it's easier to use `SelectionMap::raw_insert` to implement + // `VacantEntry::or_insert`. + Err(_) => Entry::Vacant(VacantEntry { map: self, hash, key, @@ -526,6 +529,8 @@ impl<'a> SelectionValue<'a> { } } + // This is used in operation::optimize tests + #[cfg(test)] pub(super) fn get_selection_set_mut(&mut self) -> Option<&mut SelectionSet> { match self { SelectionValue::Field(field) => field.get_selection_set_mut(), @@ -568,6 +573,7 @@ impl<'a> FragmentSpreadSelectionValue<'a> { self.0 } + #[cfg(test)] pub(crate) fn get_selection_set_mut(&mut self) -> &mut SelectionSet { &mut Arc::make_mut(self.0).selection_set } From 5a53b783614f283aca560ec9d16ee7135bd3e6dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 23 Oct 2024 14:25:20 +0200 Subject: [PATCH 16/17] Allow Zlib license --- about.toml | 3 ++- deny.toml | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/about.toml b/about.toml index dbd148a7a4..094647afae 100644 --- a/about.toml +++ b/about.toml @@ -9,7 +9,8 @@ accepted = [ "LicenseRef-ring", "MIT", "MPL-2.0", - "Unicode-DFS-2016" + "Unicode-DFS-2016", + "Zlib" ] # See https://github.com/EmbarkStudios/cargo-about/pull/216 diff --git a/deny.toml b/deny.toml index 915ca9e58c..55cd3fb0f0 100644 --- a/deny.toml +++ b/deny.toml @@ -53,7 +53,8 @@ allow = [ "MIT", "MPL-2.0", "Elastic-2.0", - "Unicode-DFS-2016" + "Unicode-DFS-2016", + "Zlib" ] copyleft = "warn" allow-osi-fsf-free = "neither" From 9390878e0bf3582b08e051bfc7f7b09a36d115c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 28 Oct 2024 14:56:26 +0100 Subject: [PATCH 17/17] Fix comment on `Selection{,Set}::conditions()` --- apollo-federation/src/operation/mod.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index d96079ce6c..5c0dc7c71f 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -405,10 +405,6 @@ impl Selection { } /// Returns true if the selection key is `__typename` *without directives*. - /// - /// # Errors - /// Returns an error if the selection contains a fragment spread, or if any of the - /// @skip/@include directives are invalid (per GraphQL validation rules). pub(crate) fn is_typename_field(&self) -> bool { if let Selection::Field(field) = self { *field.field.response_name() == TYPENAME_FIELD && field.field.directives.is_empty() @@ -417,6 +413,11 @@ impl Selection { } } + /// Returns the conditions for inclusion of this selection. + /// + /// # Errors + /// Returns an error if the selection contains a fragment spread, or if any of the + /// @skip/@include directives are invalid (per GraphQL validation rules). pub(crate) fn conditions(&self) -> Result { let self_conditions = Conditions::from_directives(self.directives())?; if let Conditions::Boolean(false) = self_conditions { @@ -1668,6 +1669,13 @@ impl SelectionSet { } } + /// Returns the conditions for inclusion of this selection set. + /// + /// This tries to be smart about including or excluding the whole selection set. + /// - If all selections have the same condition, returns that condition. + /// - If selections in the set have different conditions, the selection set must always be + /// included, so the individual selections can be evaluated. + /// /// # Errors /// Returns an error if the selection set contains a fragment spread, or if any of the /// @skip/@include directives are invalid (per GraphQL validation rules).