[pdm-devel] [PATCH proxmox 2/3] section-config: remove DerefMut and make underlying HashMap private

[Gabriel Goller]

Currently to insert a SectionConfig-section correctly we need to insert
it into the HashMap and insert the key in the order vector as well. This
is not ensure the SectionConfig gets written in the same order as it was
read. By implementing DerefMut and making the underlying HashMap `pub`
we allow to insert sections in the HashMap without inserting the key
into the the order. This caused a whole range of bugs where sections
where inserted, but never written (because we iterate over the order).

To improve this, we add our own insert and remove functions that
automatically add and remove the keys from the order. Also delete the
DerefMut impl and make the underlying Vec and HashMap private.

Note that this is a breaking change.

Signed-off-by: Gabriel Goller <g.goller at proxmox.com>
---
 proxmox-section-config/src/typed.rs | 61 +++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 8 deletions(-)

diff --git a/proxmox-section-config/src/typed.rs b/proxmox-section-config/src/typed.rs
index a25798e8a29b..14a8d87e3425 100644
--- a/proxmox-section-config/src/typed.rs
+++ b/proxmox-section-config/src/typed.rs
@@ -143,8 +143,8 @@ fn to_pair(
 /// This dereferences to the section hash for convenience.
 #[derive(Debug, Clone)]
 pub struct SectionConfigData<T> {
-    pub sections: HashMap<String, T>,
-    pub order: Vec<String>,
+    sections: HashMap<String, T>,
+    order: Vec<String>,
 }
 
 impl<T> Default for SectionConfigData<T> {
@@ -156,6 +156,57 @@ impl<T> Default for SectionConfigData<T> {
     }
 }
 
+impl<T> SectionConfigData<T> {
+    /// Return a mutable reference to the value corresponding to the key.
+    pub fn get_mut(&mut self, key: &str) -> Option<&mut T> {
+        self.sections.get_mut(key)
+    }
+
+    /// Returns an iterator over the section key-value pairs in their original order.
+    pub fn iter(&self) -> Iter<'_, T> {
+        Iter {
+            sections: &self.sections,
+            order: self.order.iter(),
+        }
+    }
+
+    /// Insert a key-value pair in the section config.
+    ///
+    /// If the key does not exist in the map, it will be added to the end of
+    /// the order vector.
+    ///
+    /// # Returns
+    ///
+    /// * Some(value) - If the key was present, returns the previous value
+    /// * None - If the key was not present
+    pub fn insert(&mut self, key: String, value: T) -> Option<T> {
+        let previous_value = self.sections.insert(key.clone(), value);
+        // If there was no previous value, this is a new key, so add it to the order
+        if previous_value.is_none() {
+            self.order.push(key);
+        }
+        previous_value
+    }
+
+    /// Remove a key-value pair from the section config.
+    ///
+    /// If the key existed and was removed, also remove the key from the order vector
+    /// to maintain ordering consistency.
+    ///
+    /// # Returns
+    ///
+    /// * `Some(value)` - If the key was present, returns the value that was removed
+    /// * `None` - If the key was not present
+    pub fn remove(&mut self, key: &str) -> Option<T> {
+        let removed_value = self.sections.remove(key);
+        // only update the order vector if we actually removed something
+        if removed_value.is_some() {
+            self.order.retain(|k| k != key);
+        }
+        removed_value
+    }
+}
+
 impl<T: ApiSectionDataEntry + DeserializeOwned> TryFrom<RawSectionConfigData>
     for SectionConfigData<T>
 {
@@ -184,12 +235,6 @@ impl<T> std::ops::Deref for SectionConfigData<T> {
     }
 }
 
-impl<T> std::ops::DerefMut for SectionConfigData<T> {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        &mut self.sections
-    }
-}
-
 impl<T: Serialize + ApiSectionDataEntry> TryFrom<SectionConfigData<T>> for RawSectionConfigData {
     type Error = serde_json::Error;
 
-- 
2.39.5