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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

1 minor thing...

On Mon, Apr 14, 2025 at 02:00:44PM +0200, Gabriel Goller wrote:
> 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@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);

`retain()` always goes through the entire vector. We don't typically
have large numbers of sections, so, not that bad, butin general I do
lean more towards

    if let Some(pos) = vec.iter().position(|k| k == key) {
        vec.remove(pos);
    }

when we know there should only be at most 1 such element

> +        }
> +        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


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel