From 82a0ef6d14c6f4e144ee1b4f5eff6abebf0108f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Natalie=20Klestrup=20R=C3=B6ijezon?= Date: Thu, 17 Oct 2024 07:57:38 +0200 Subject: [PATCH] Replace KeyValuePairs' internal BTreeSet with BTreeMap (#888) * Replace KeyValuePairs' internal BTreeSet with BTreeMap Fixes #887 * Rename PairAlreadyExists => KeyAlreadyExists * Changelog --- crates/stackable-operator/CHANGELOG.md | 11 +++- .../src/kvp/annotation/mod.rs | 5 +- .../stackable-operator/src/kvp/label/mod.rs | 4 +- crates/stackable-operator/src/kvp/mod.rs | 62 ++++++++++++------- 4 files changed, 53 insertions(+), 29 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index d1f938f45..447649fed 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -13,6 +13,14 @@ All notable changes to this project will be documented in this file. - BREAKING: The `CustomResourceExt` trait is now re-exported from the `stackable-shared` crate. The trait functions use the same parameters but return a different error type ([#883]). +- BREAKING: `KeyValuePairs` (as well as `Labels`/`Annotations` via it) is now backed by a `BTreeMap` rather than a `BTreeSet` ([#888]). + - The `Deref` impl now returns a `BTreeMap` instead. + - `iter()` now clones the values. + +### Fixed + +- BREAKING: `KeyValuePairs::insert` (as well as `Labels::`/`Annotations::` via it) now overwrites the old value if the key already exists ([#888]). + - Previously, `iter()` would return *both* values in lexicographical order (causing further conversions like `Into` to prefer the maximum value). ### Removed @@ -22,6 +30,7 @@ All notable changes to this project will be documented in this file. [#883]: https://github.com/stackabletech/operator-rs/pull/883 [#885]: https://github.com/stackabletech/operator-rs/pull/885 +[#888]: https://github.com/stackabletech/operator-rs/pull/888 ## [0.78.0] - 2024-09-30 @@ -273,7 +282,7 @@ All notable changes to this project will be documented in this file. ### Changed -- Implement `PartialEq` for most _Snafu_ Error enums ([#757]). +- Implement `PartialEq` for most *Snafu* Error enums ([#757]). - Update Rust to 1.77 ([#759]) ### Fixed diff --git a/crates/stackable-operator/src/kvp/annotation/mod.rs b/crates/stackable-operator/src/kvp/annotation/mod.rs index 295a4ec88..ed9ee4215 100644 --- a/crates/stackable-operator/src/kvp/annotation/mod.rs +++ b/crates/stackable-operator/src/kvp/annotation/mod.rs @@ -293,15 +293,14 @@ impl Annotations { pub fn contains_key(&self, key: impl TryInto) -> bool; /// Returns an [`Iterator`] over [`Annotations`] yielding a reference to every [`Annotation`] contained within. - pub fn iter(&self) -> impl Iterator>; - + pub fn iter(&self) -> impl Iterator> + '_; } } } impl IntoIterator for Annotations { type Item = KeyValuePair; - type IntoIter = std::collections::btree_set::IntoIter; + type IntoIter = as IntoIterator>::IntoIter; /// Returns a consuming [`Iterator`] over [`Annotations`] moving every [`Annotation`] out. /// The [`Annotations`] cannot be used again after calling this. diff --git a/crates/stackable-operator/src/kvp/label/mod.rs b/crates/stackable-operator/src/kvp/label/mod.rs index 7895e9c5f..ae38c12d5 100644 --- a/crates/stackable-operator/src/kvp/label/mod.rs +++ b/crates/stackable-operator/src/kvp/label/mod.rs @@ -385,7 +385,7 @@ impl Labels { pub fn contains_key(&self, key: impl TryInto) -> bool; /// Returns an [`Iterator`] over [`Labels`] yielding a reference to every [`Label`] contained within. - pub fn iter(&self) -> impl Iterator>; + pub fn iter(&self) -> impl Iterator> + '_; } } @@ -393,7 +393,7 @@ impl Labels { impl IntoIterator for Labels { type Item = KeyValuePair; - type IntoIter = std::collections::btree_set::IntoIter; + type IntoIter = as IntoIterator>::IntoIter; /// Returns a consuming [`Iterator`] over [`Labels`] moving every [`Label`] out. /// The [`Labels`] cannot be used again after calling this. diff --git a/crates/stackable-operator/src/kvp/mod.rs b/crates/stackable-operator/src/kvp/mod.rs index c37ce71c0..5a233c603 100644 --- a/crates/stackable-operator/src/kvp/mod.rs +++ b/crates/stackable-operator/src/kvp/mod.rs @@ -152,8 +152,8 @@ where #[derive(Debug, PartialEq, Snafu)] pub enum KeyValuePairsError { - #[snafu(display("key/value pair already exists"))] - PairAlreadyExists, + #[snafu(display("key already exists"))] + KeyAlreadyExists, } /// A validated set/list of Kubernetes key/value pairs. @@ -178,7 +178,7 @@ pub enum KeyValuePairsError { /// which ultimately prevent unncessary reconciliations due to changes /// in item order. #[derive(Clone, Debug, Default)] -pub struct KeyValuePairs(BTreeSet>); +pub struct KeyValuePairs(BTreeMap); impl TryFrom> for KeyValuePairs where @@ -224,7 +224,7 @@ where T: Value, { fn from_iter>>(iter: I) -> Self { - Self(iter.into_iter().collect()) + Self(iter.into_iter().map(|kvp| (kvp.key, kvp.value)).collect()) } } @@ -242,7 +242,7 @@ where .map(KeyValuePair::try_from) .collect::, KeyValuePairError>>()?; - Ok(Self(pairs)) + Ok(Self::from_iter(pairs)) } } @@ -253,7 +253,7 @@ where fn from(value: KeyValuePairs) -> Self { value .iter() - .map(|pair| (pair.key().to_string(), pair.value().to_string())) + .map(|(k, v)| (k.to_string(), v.to_string())) .collect() } } @@ -262,7 +262,7 @@ impl Deref for KeyValuePairs where T: Value, { - type Target = BTreeSet>; + type Target = BTreeMap; fn deref(&self) -> &Self::Target { &self.0 @@ -280,7 +280,7 @@ where /// Creates a new list of [`KeyValuePair`]s from `pairs`. pub fn new_with(pairs: BTreeSet>) -> Self { - Self(pairs) + Self::from_iter(pairs) } /// Extends `self` with `other`. @@ -295,16 +295,16 @@ where /// [`KeyValuePairs::contains_key`] before inserting or try to insert /// fallible via [`KeyValuePairs::try_insert`]. pub fn insert(&mut self, kvp: KeyValuePair) -> &mut Self { - self.0.insert(kvp); + self.0.insert(kvp.key, kvp.value); self } /// Tries to insert a new [`KeyValuePair`] into the list of pairs. /// - /// If the list already had this pair present, nothing is updated, and an + /// If the list already had this key present, nothing is updated, and an /// error is returned. pub fn try_insert(&mut self, kvp: KeyValuePair) -> Result<(), KeyValuePairsError> { - ensure!(!self.0.contains(&kvp), PairAlreadyExistsSnafu); + ensure!(!self.0.contains_key(&kvp.key), KeyAlreadyExistsSnafu); self.insert(kvp); Ok(()) } @@ -314,7 +314,10 @@ where let Ok(kvp) = kvp.try_into() else { return false; }; - self.0.contains(&kvp) + let Some(value) = self.get(&kvp.key) else { + return false; + }; + value == &kvp.value } /// Returns if the list contains a key/value pair with a specific [`Key`]. @@ -323,18 +326,15 @@ where return false; }; - for kvp in &self.0 { - if kvp.key == key { - return true; - } - } - - false + self.0.contains_key(&key) } /// Returns an [`Iterator`] over [`KeyValuePairs`] yielding a reference to every [`KeyValuePair`] contained within. - pub fn iter(&self) -> impl Iterator> { - self.0.iter() + pub fn iter(&self) -> impl Iterator> + '_ { + self.0.iter().map(|(k, v)| KeyValuePair { + key: k.clone(), + value: v.clone(), + }) } } @@ -343,12 +343,15 @@ where T: Value, { type Item = KeyValuePair; - type IntoIter = std::collections::btree_set::IntoIter; + type IntoIter = + std::iter::Map, fn((Key, T)) -> Self::Item>; /// Returns a consuming [`Iterator`] over [`KeyValuePairs`] moving every [`KeyValuePair`] out. /// The [`KeyValuePairs`] cannot be used again after calling this. fn into_iter(self) -> Self::IntoIter { - self.0.into_iter() + self.0 + .into_iter() + .map(|(key, value)| KeyValuePair { key, value }) } } @@ -488,4 +491,17 @@ mod test { let report = Report::from_error(err); println!("{report}") } + + #[test] + fn merge() { + let mut merged_labels = + Labels::try_from_iter([("a", "b"), ("b", "a"), ("c", "c")]).unwrap(); + merged_labels.extend(Labels::try_from_iter([("a", "a"), ("b", "b"), ("d", "d")]).unwrap()); + assert_eq!( + BTreeMap::from(merged_labels), + BTreeMap::from( + Labels::try_from_iter([("a", "a"), ("b", "b"), ("c", "c"), ("d", "d")]).unwrap() + ) + ) + } }