[pdm-devel] [PATCH proxmox{, -datacenter-manager} 0/4] Remove direct access to SectionConfigData<T> sections

[pdm-devel] [PATCH proxmox{, -datacenter-manager} 0/4] Remove direct access to SectionConfigData<T> sections

[Gabriel Goller]

This patch series removes direct access to SectionConfigData<T>. Previously,
one could directly access and add sections by using DerefMut and the public
`sections` HashMap property. This created a significant risk of not updating
the `order` Vector, which maintains the section sequence to ensure consistent
read and write ordering.

To address this issue, this patch removes the DerefMut implementation and makes
both `sections` and `order` private. Read access remains available through the
Deref implementation for the `sections` HashMap, while write access is now
provided through newly introduced helper methods: `insert`, `remove`, and
`get_mut`.

This is a breaking change that affects multiple `SectionConfigData<T>` usages.
Since SectionConfigData<T> is primarily used in the proxmox-datacenter-manager,
all usages there have been updated accordingly.

A follow-up patch series is in preparation that will convert untyped
SectionConfigData to typed SectionConfigData<T>, which provides better
usability and eliminates the need for frequent serialization and
deserialization operations.

proxmox:

Gabriel Goller (3):
  section-config: make write_section_config parameter more generic
  section-config: remove DerefMut and make underlying HashMap private
  section-config: add lookup and convert_to_typed_array helpers

 proxmox-section-config/Cargo.toml   |   3 +
 proxmox-section-config/src/typed.rs | 189 ++++++++++++++++++++++++++--
 2 files changed, 183 insertions(+), 9 deletions(-)


proxmox-datacenter-manager:

Gabriel Goller (1):
  remotes: remove direct access on underlying sections HashMap

 server/src/api/resources.rs            | 2 +-
 server/src/metric_collection/mod.rs    | 8 ++++----
 server/src/remote_tasks/mod.rs         | 6 +++---
 server/src/test_support/fake_remote.rs | 9 +++------
 4 files changed, 11 insertions(+), 14 deletions(-)


Summary over all repositories:
  6 files changed, 194 insertions(+), 23 deletions(-)

-- 
Generated by git-murpp 0.8.0


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

[pdm-devel] [PATCH proxmox 1/3] section-config: make write_section_config parameter more generic

[Gabriel Goller]

The underlying function which gets called by `write_section_config` also
takes a `impl AsRef<Path>` and in some cases we do pass a path, so
taking the same parameter is better.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-section-config/src/typed.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/proxmox-section-config/src/typed.rs b/proxmox-section-config/src/typed.rs
index 31e8e4b2e546..a25798e8a29b 100644
--- a/proxmox-section-config/src/typed.rs
+++ b/proxmox-section-config/src/typed.rs
@@ -1,6 +1,7 @@
 //! Support for `enum` typed section configs.
 
 use std::collections::HashMap;
+use std::path::Path;
 
 use anyhow::Error;
 use serde::{de::DeserializeOwned, Serialize};
@@ -90,7 +91,7 @@ pub trait ApiSectionDataEntry: Sized {
     }
 
     /// Provided. Shortcut for `Self::section_config().write(filename, &data.try_into()?)`.
-    fn write_section_config(filename: &str, data: &SectionConfigData<Self>) -> Result<String, Error>
+    fn write_section_config(filename: impl AsRef<Path>, data: &SectionConfigData<Self>) -> Result<String, Error>
     where
         Self: Serialize,
     {
-- 
2.39.5



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

[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@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



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

[pdm-devel] [PATCH proxmox 3/3] section-config: add lookup and convert_to_typed_array helpers

[Gabriel Goller]

These helpers exist on the untyped SectionConfigData struct, but not on
the typed one. To facilitate the migration to the typed
SectionConfigData<T> add these as macros.

These can be used to simply lookup an item with a specific type by
passing the type, e.g.:

    lookup!(config, "root", UserSectionConfig::User)

or retrieve a vector with the sections that correspond to the passed
type, e.g.:

    convert_to_typed_array!(config, UserSectionConfig::ApiToken)

Also add some simple unit-tests to test them.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-section-config/Cargo.toml   |   3 +
 proxmox-section-config/src/typed.rs | 125 ++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

diff --git a/proxmox-section-config/Cargo.toml b/proxmox-section-config/Cargo.toml
index 4796a6bff5e6..eb456a3bb3e6 100644
--- a/proxmox-section-config/Cargo.toml
+++ b/proxmox-section-config/Cargo.toml
@@ -19,3 +19,6 @@ serde_json.workspace = true
 proxmox-schema.workspace = true
 # FIXME: remove!
 proxmox-lang.workspace = true
+
+[dev-dependencies]
+serde = { workspace = true, features = ["derive"] }
diff --git a/proxmox-section-config/src/typed.rs b/proxmox-section-config/src/typed.rs
index 14a8d87e3425..c13e2338461d 100644
--- a/proxmox-section-config/src/typed.rs
+++ b/proxmox-section-config/src/typed.rs
@@ -207,6 +207,37 @@ impl<T> SectionConfigData<T> {
     }
 }
 
+#[macro_export]
+macro_rules! lookup {
+    ($map:expr, $key:expr, $variant:path) => {
+        $map.get($key).and_then(|value| {
+            if let $variant(inner) = value {
+                Some(inner)
+            } else {
+                None
+            }
+        })
+    };
+}
+
+#[macro_export]
+macro_rules! convert_to_typed_array {
+    ($map:expr, $variant:path) => {
+        $map.values()
+            .filter_map(|value| {
+                if let $variant(inner) = value {
+                    Some(inner)
+                } else {
+                    None
+                }
+            })
+            .collect::<Vec<_>>()
+    };
+}
+
+pub use lookup;
+pub use convert_to_typed_array;
+
 impl<T: ApiSectionDataEntry + DeserializeOwned> TryFrom<RawSectionConfigData>
     for SectionConfigData<T>
 {
@@ -367,6 +398,7 @@ mod test {
     use std::sync::OnceLock;
 
     use proxmox_schema::{ApiStringFormat, EnumEntry, ObjectSchema, Schema, StringSchema};
+    use serde::{Deserialize, Serialize};
 
     use crate::{SectionConfig, SectionConfigPlugin};
 
@@ -508,4 +540,97 @@ mod test {
             .expect("failed to write out section config");
         assert_eq!(written, raw);
     }
+
+    #[derive(Deserialize, Serialize, Debug, PartialEq, Eq, PartialOrd, Ord)]
+    struct User {
+        user_id: String,
+    }
+
+    #[derive(Deserialize, Serialize, Debug, PartialEq, Eq, PartialOrd, Ord)]
+    struct Token {
+        token: String,
+    }
+
+    #[derive(Deserialize, Serialize)]
+    enum UserSectionConfig {
+        #[serde(rename="user")]
+        User(User),
+        #[serde(rename="token")]
+        Token(Token)
+    }
+
+    const USER_PROPERTIES: ObjectSchema = ObjectSchema::new(
+        "User",
+        &[
+            ("user_id", false, &StringSchema::new("Some id.").schema()),
+        ],
+    );
+
+    const TOKEN_PROPERTIES: ObjectSchema = ObjectSchema::new(
+        "Token",
+        &[
+            ("token", false, &StringSchema::new("Some token.").schema()),
+        ],
+    );
+
+    impl ApiSectionDataEntry for UserSectionConfig {
+        fn section_config() -> &'static SectionConfig {
+            static SC: OnceLock<SectionConfig> = OnceLock::new();
+
+            SC.get_or_init(|| {
+                let mut config = SectionConfig::new(&ID_SCHEMA);
+                config.register_plugin(SectionConfigPlugin::new(
+                    "user".to_string(),
+                    None,
+                    &USER_PROPERTIES,
+                ));
+                config.register_plugin(SectionConfigPlugin::new(
+                    "token".to_string(),
+                    None,
+                    &TOKEN_PROPERTIES,
+                ));
+                config
+            })
+        }
+
+        fn section_type(&self) -> &'static str {
+            match self {
+                UserSectionConfig::User(_) => "user",
+                UserSectionConfig::Token(_) => "token",
+            }
+        }
+    }
+
+    #[test]
+    fn test_macros() {
+        let filename = "sync.cfg";
+        let raw = "\
+            token: first\n\
+                \ttoken 1\n\
+            \n\
+            user: second\n\
+                \tuser_id 2\n\
+            \n\
+            user: third\n\
+                \tuser_id 4\n\
+        ";
+
+        let parsed = UserSectionConfig::section_config()
+            .parse(filename, raw)
+            .expect("failed to parse");
+        let config: SectionConfigData<UserSectionConfig> = parsed.try_into().expect("failed to convert");
+
+        let token = lookup!(config, "first", UserSectionConfig::Token);
+        assert_eq!(token.unwrap().token, "1");
+        let user = lookup!(config, "second", UserSectionConfig::User);
+        assert_eq!(user.unwrap().user_id, "2");
+
+        let mut tokens = convert_to_typed_array!(config, UserSectionConfig::Token);
+        tokens.sort();
+        assert_eq!(tokens, vec![&Token{token: "1".to_owned()}]);
+
+        let mut users = convert_to_typed_array!(config, UserSectionConfig::User);
+        users.sort();
+        assert_eq!(users, vec![&User{user_id: "2".to_owned()}, &User{user_id: "4".to_owned()}]);
+    }
 }
-- 
2.39.5



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

[pdm-devel] [PATCH proxmox-datacenter-manager 1/1] remotes: remove direct access on underlying sections HashMap

[Gabriel Goller]

Since accessing the underlying SectionConfigData<T> HashMap poses a lot
of risks (missing order insertion, removal) it has been made private and
the DerefMut implementation was removed. Use either Deref for read
access or use the implemented helper functions (insert, get_mut, etc.).
Instead of iterating over the HashMap, use the SectionConfigData
iterator, which will iterate in the correct order as configured in the
`order` vector.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 server/src/api/resources.rs            | 2 +-
 server/src/metric_collection/mod.rs    | 8 ++++----
 server/src/remote_tasks/mod.rs         | 6 +++---
 server/src/test_support/fake_remote.rs | 9 +++------
 4 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
index 453d9e8ea6e5..6a8c8ef25b71 100644
--- a/server/src/api/resources.rs
+++ b/server/src/api/resources.rs
@@ -320,7 +320,7 @@ async fn get_top_entities(timeframe: Option<RrdTimeframe>) -> Result<TopEntities
     let (remotes_config, _) = pdm_config::remotes::config()?;
 
     let timeframe = timeframe.unwrap_or(RrdTimeframe::Day);
-    let res = crate::metric_collection::calculate_top(&remotes_config.sections, timeframe, 10);
+    let res = crate::metric_collection::calculate_top(&remotes_config, timeframe, 10);
     Ok(res)
 }
 
diff --git a/server/src/metric_collection/mod.rs b/server/src/metric_collection/mod.rs
index b37d678250a0..726884e5f732 100644
--- a/server/src/metric_collection/mod.rs
+++ b/server/src/metric_collection/mod.rs
@@ -49,14 +49,14 @@ async fn metric_collection_task() -> Result<(), Error> {
             }
         };
 
-        for (remote_name, remote) in &remotes.sections {
-            let start_time = *most_recent_timestamps.get(remote_name).unwrap_or(&0);
+        for (remote_name, remote) in remotes.into_iter() {
+            let start_time = *most_recent_timestamps.get(&remote_name).unwrap_or(&0);
             let remote_name_clone = remote_name.clone();
 
             let res = async {
                 let most_recent_timestamp = match remote.ty {
                     RemoteType::Pve => {
-                        let client = connection::make_pve_client(remote)?;
+                        let client = connection::make_pve_client(&remote)?;
                         let metrics = client
                             .cluster_metrics_export(Some(true), Some(false), Some(start_time))
                             .await?;
@@ -76,7 +76,7 @@ async fn metric_collection_task() -> Result<(), Error> {
                         .await
                     }
                     RemoteType::Pbs => {
-                        let client = connection::make_pbs_client(remote)?;
+                        let client = connection::make_pbs_client(&remote)?;
                         let metrics = client.metrics(Some(true), Some(start_time)).await?;
 
                         // Involves some blocking file IO
diff --git a/server/src/remote_tasks/mod.rs b/server/src/remote_tasks/mod.rs
index 7ded540845f2..234ffa76921b 100644
--- a/server/src/remote_tasks/mod.rs
+++ b/server/src/remote_tasks/mod.rs
@@ -36,10 +36,10 @@ pub async fn get_tasks(max_age: i64, filters: TaskFilters) -> Result<Vec<TaskLis
     // Room for improvements in the future.
     invalidate_cache_for_finished_tasks(&mut cache);
 
-    for (remote_name, remote) in &remotes.sections {
+    for (remote_name, remote) in remotes.iter() {
         let now = proxmox_time::epoch_i64();
 
-        if let Some(tasks) = cache.get_tasks(remote_name.as_str(), now, max_age) {
+        if let Some(tasks) = cache.get_tasks(remote_name, now, max_age) {
             // Data in cache is recent enough and has not been invalidated.
             all_tasks.extend(tasks);
         } else {
@@ -50,7 +50,7 @@ pub async fn get_tasks(max_age: i64, filters: TaskFilters) -> Result<Vec<TaskLis
                     continue;
                 }
             };
-            cache.set_tasks(remote_name.as_str(), tasks.clone(), now);
+            cache.set_tasks(remote_name, tasks.clone(), now);
 
             all_tasks.extend(tasks);
         }
diff --git a/server/src/test_support/fake_remote.rs b/server/src/test_support/fake_remote.rs
index 40f688074da0..0161d8e6e5f7 100644
--- a/server/src/test_support/fake_remote.rs
+++ b/server/src/test_support/fake_remote.rs
@@ -27,13 +27,12 @@ pub struct FakeRemoteConfig {
 
 impl RemoteConfig for FakeRemoteConfig {
     fn config(&self) -> Result<(SectionConfigData<Remote>, ConfigDigest), Error> {
-        let mut order = Vec::new();
-        let mut sections = HashMap::new();
+        let mut section_config = SectionConfigData::default();
 
         for i in 0..self.nr_of_pve_remotes {
             let name = format!("pve-{i}");
 
-            sections.insert(
+            section_config.insert(
                 name.clone(),
                 Remote {
                     ty: pdm_api_types::remotes::RemoteType::Pve,
@@ -44,13 +43,11 @@ impl RemoteConfig for FakeRemoteConfig {
                     web_url: None,
                 },
             );
-
-            order.push(name);
         }
 
         let digest = [0u8; 32].into();
 
-        Ok((SectionConfigData { sections, order }, digest))
+        Ok((section_config, digest))
     }
 
     fn lock_config(&self) -> Result<ApiLockGuard, Error> {
-- 
2.39.5



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

Re: [pdm-devel] [PATCH proxmox 1/3] section-config: make write_section_config parameter more generic

[Wolfgang Bumiller]

style nit

On Mon, Apr 14, 2025 at 02:00:43PM +0200, Gabriel Goller wrote:
> The underlying function which gets called by `write_section_config` also
> takes a `impl AsRef<Path>` and in some cases we do pass a path, so
> taking the same parameter is better.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  proxmox-section-config/src/typed.rs | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox-section-config/src/typed.rs b/proxmox-section-config/src/typed.rs
> index 31e8e4b2e546..a25798e8a29b 100644
> --- a/proxmox-section-config/src/typed.rs
> +++ b/proxmox-section-config/src/typed.rs
> @@ -1,6 +1,7 @@
>  //! Support for `enum` typed section configs.
>  
>  use std::collections::HashMap;
> +use std::path::Path;
>  
>  use anyhow::Error;
>  use serde::{de::DeserializeOwned, Serialize};
> @@ -90,7 +91,7 @@ pub trait ApiSectionDataEntry: Sized {
>      }
>  
>      /// Provided. Shortcut for `Self::section_config().write(filename, &data.try_into()?)`.
> -    fn write_section_config(filename: &str, data: &SectionConfigData<Self>) -> Result<String, Error>
> +    fn write_section_config(filename: impl AsRef<Path>, data: &SectionConfigData<Self>) -> Result<String, Error>

- This is now too long (missing `fmt` call?)
- I'd like to avoid too many `impl` parameters as they very quickly lead
  to very long lines, particularly...

>      where

...since we already have a `where` section here. With `filename: P` the
signature still fits in into a single line with
    P: AsRef<Path>,
here

>          Self: Serialize,
>      {
> -- 
> 2.39.5


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

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

[Wolfgang Bumiller]

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

Re: [pdm-devel] [PATCH proxmox 3/3] section-config: add lookup and convert_to_typed_array helpers

[Wolfgang Bumiller]

On Mon, Apr 14, 2025 at 02:00:45PM +0200, Gabriel Goller wrote:
> These helpers exist on the untyped SectionConfigData struct, but not on
> the typed one. To facilitate the migration to the typed
> SectionConfigData<T> add these as macros.
> 
> These can be used to simply lookup an item with a specific type by
> passing the type, e.g.:
> 
>     lookup!(config, "root", UserSectionConfig::User)
> 
> or retrieve a vector with the sections that correspond to the passed
> type, e.g.:
> 
>     convert_to_typed_array!(config, UserSectionConfig::ApiToken)
> 
> Also add some simple unit-tests to test them.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  proxmox-section-config/Cargo.toml   |   3 +
>  proxmox-section-config/src/typed.rs | 125 ++++++++++++++++++++++++++++
>  2 files changed, 128 insertions(+)
> 
> diff --git a/proxmox-section-config/Cargo.toml b/proxmox-section-config/Cargo.toml
> index 4796a6bff5e6..eb456a3bb3e6 100644
> --- a/proxmox-section-config/Cargo.toml
> +++ b/proxmox-section-config/Cargo.toml
> @@ -19,3 +19,6 @@ serde_json.workspace = true
>  proxmox-schema.workspace = true
>  # FIXME: remove!
>  proxmox-lang.workspace = true
> +
> +[dev-dependencies]
> +serde = { workspace = true, features = ["derive"] }
> diff --git a/proxmox-section-config/src/typed.rs b/proxmox-section-config/src/typed.rs
> index 14a8d87e3425..c13e2338461d 100644
> --- a/proxmox-section-config/src/typed.rs
> +++ b/proxmox-section-config/src/typed.rs
> @@ -207,6 +207,37 @@ impl<T> SectionConfigData<T> {
>      }
>  }
>  
> +#[macro_export]
> +macro_rules! lookup {
> +    ($map:expr, $key:expr, $variant:path) => {
> +        $map.get($key).and_then(|value| {
> +            if let $variant(inner) = value {
> +                Some(inner)
> +            } else {
> +                None
> +            }
> +        })

^ You could shorten this by matching on the nested structure here:

    match $map.get($key) {
        Some($variant(inner)) => Some(inner),
        _ => None,
    }

> +    };
> +}
> +
> +#[macro_export]
> +macro_rules! convert_to_typed_array {
> +    ($map:expr, $variant:path) => {
> +        $map.values()
> +            .filter_map(|value| {
> +                if let $variant(inner) = value {
> +                    Some(inner)
> +                } else {
> +                    None
> +                }

^ (rustfmt puts a `match` here on the `filter_map` line, so that would
be much shorter ;-) )

> +            })
> +            .collect::<Vec<_>>()
> +    };
> +}
> +
> +pub use lookup;
> +pub use convert_to_typed_array;

If, for now, you intend for these to be used via the `typed` module
instead of at the top level you should mark these re-exports with
`#[doc(inline)]` and the macros above with `#[doc(hidden)]`.

Otherwise the documentation shows them at the top level, with the ones
in `typed` as re-exports.

> +
>  impl<T: ApiSectionDataEntry + DeserializeOwned> TryFrom<RawSectionConfigData>
>      for SectionConfigData<T>
>  {
> @@ -367,6 +398,7 @@ mod test {
>      use std::sync::OnceLock;
>  
>      use proxmox_schema::{ApiStringFormat, EnumEntry, ObjectSchema, Schema, StringSchema};
> +    use serde::{Deserialize, Serialize};
>  
>      use crate::{SectionConfig, SectionConfigPlugin};
>  
> @@ -508,4 +540,97 @@ mod test {
>              .expect("failed to write out section config");
>          assert_eq!(written, raw);
>      }
> +
> +    #[derive(Deserialize, Serialize, Debug, PartialEq, Eq, PartialOrd, Ord)]
> +    struct User {
> +        user_id: String,
> +    }
> +
> +    #[derive(Deserialize, Serialize, Debug, PartialEq, Eq, PartialOrd, Ord)]
> +    struct Token {
> +        token: String,
> +    }
> +
> +    #[derive(Deserialize, Serialize)]
> +    enum UserSectionConfig {
> +        #[serde(rename="user")]
> +        User(User),
> +        #[serde(rename="token")]
> +        Token(Token)
> +    }
> +
> +    const USER_PROPERTIES: ObjectSchema = ObjectSchema::new(
> +        "User",
> +        &[
> +            ("user_id", false, &StringSchema::new("Some id.").schema()),
> +        ],
> +    );
> +
> +    const TOKEN_PROPERTIES: ObjectSchema = ObjectSchema::new(
> +        "Token",
> +        &[
> +            ("token", false, &StringSchema::new("Some token.").schema()),
> +        ],
> +    );
> +
> +    impl ApiSectionDataEntry for UserSectionConfig {
> +        fn section_config() -> &'static SectionConfig {
> +            static SC: OnceLock<SectionConfig> = OnceLock::new();
> +
> +            SC.get_or_init(|| {
> +                let mut config = SectionConfig::new(&ID_SCHEMA);
> +                config.register_plugin(SectionConfigPlugin::new(
> +                    "user".to_string(),
> +                    None,
> +                    &USER_PROPERTIES,
> +                ));
> +                config.register_plugin(SectionConfigPlugin::new(
> +                    "token".to_string(),
> +                    None,
> +                    &TOKEN_PROPERTIES,
> +                ));
> +                config
> +            })
> +        }
> +
> +        fn section_type(&self) -> &'static str {
> +            match self {
> +                UserSectionConfig::User(_) => "user",
> +                UserSectionConfig::Token(_) => "token",
> +            }
> +        }
> +    }
> +
> +    #[test]
> +    fn test_macros() {
> +        let filename = "sync.cfg";
> +        let raw = "\
> +            token: first\n\
> +                \ttoken 1\n\
> +            \n\
> +            user: second\n\
> +                \tuser_id 2\n\
> +            \n\
> +            user: third\n\
> +                \tuser_id 4\n\
> +        ";
> +
> +        let parsed = UserSectionConfig::section_config()
> +            .parse(filename, raw)
> +            .expect("failed to parse");
> +        let config: SectionConfigData<UserSectionConfig> = parsed.try_into().expect("failed to convert");
> +
> +        let token = lookup!(config, "first", UserSectionConfig::Token);
> +        assert_eq!(token.unwrap().token, "1");
> +        let user = lookup!(config, "second", UserSectionConfig::User);
> +        assert_eq!(user.unwrap().user_id, "2");
> +
> +        let mut tokens = convert_to_typed_array!(config, UserSectionConfig::Token);
> +        tokens.sort();
> +        assert_eq!(tokens, vec![&Token{token: "1".to_owned()}]);
> +
> +        let mut users = convert_to_typed_array!(config, UserSectionConfig::User);
> +        users.sort();
> +        assert_eq!(users, vec![&User{user_id: "2".to_owned()}, &User{user_id: "4".to_owned()}]);
> +    }
>  }
> -- 
> 2.39.5


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

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

[Wolfgang Bumiller]

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> {

Oh btw. this could be an `impl Into<String>` (or generic), that way we
can at least avoid *some* double-clones...

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


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