[pbs-devel] [PATCH proxmox] proxmox-client: add query builder

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

A big proportion of the consumers of this API use the `format!` macro to
define the base of the url, hence we use a `Cow<'_, str>` on the
constructor to prevent unnecessary allocations.

A `perl-api-path-builder` feature was added to satisfy the needs of
pve-api-types.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---

This is a follow-up of [1]. I will send the follow-ups separately.

Differences from the RFC:
 - More docs
 - More tests
 - Moved into proxmox-client

[1] https://lore.proxmox.com/pdm-devel/7qh5kchmrg4rlavkowqx44jiamcznarxn4konxww24zj7msuuv@gshqpr4ldxui/T/#u

 proxmox-client/Cargo.toml              |   5 +
 proxmox-client/src/api_path_builder.rs | 242 +++++++++++++++++++++++++
 proxmox-client/src/lib.rs              |   4 +
 3 files changed, 251 insertions(+)
 create mode 100644 proxmox-client/src/api_path_builder.rs

diff --git a/proxmox-client/Cargo.toml b/proxmox-client/Cargo.toml
index f890501e..027fbe3e 100644
--- a/proxmox-client/Cargo.toml
+++ b/proxmox-client/Cargo.toml
@@ -14,6 +14,7 @@ repository.workspace = true
 anyhow.workspace = true
 hex.workspace = true
 http.workspace = true
+percent-encoding.workspace = true
 serde.workspace = true
 serde_json.workspace = true
 
@@ -26,7 +27,11 @@ proxmox-login = { workspace = true, features = [ "http" ] }
 proxmox-http = { workspace = true, optional = true, features = [ "client" ] }
 hyper = { workspace = true, optional = true }
 
+[dev-dependencies]
+serde_plain.workspace = true
+
 [features]
 default = []
 hyper-client = [ "dep:openssl", "dep:hyper", "dep:proxmox-http", "dep:log" ]
+perl-api-path-builder = []
 webauthn = [ "proxmox-login/webauthn" ]
diff --git a/proxmox-client/src/api_path_builder.rs b/proxmox-client/src/api_path_builder.rs
new file mode 100644
index 00000000..bc2dfde4
--- /dev/null
+++ b/proxmox-client/src/api_path_builder.rs
@@ -0,0 +1,242 @@
+use std::borrow::Cow;
+
+/// Builder for API paths with a query.
+///
+/// The [Self::arg] method can be used to add multiple arguments to the query.
+///
+/// ```rust
+/// use proxmox_client::ApiPathBuilder;
+///
+/// let storage = "my-storage";
+/// let target = "my-target";
+/// let node = "pve01";
+/// let query = ApiPathBuilder::new(format!("/api2/extjs/nodes/{node}/storage"))
+///     .arg("storage", storage)
+///     .arg("target", target)
+///     .build();
+///
+/// assert_eq!(&query, "/api2/extjs/nodes/pve01/storage?storage=my%2Dstorage&target=my%2Dtarget");
+/// ```
+///
+/// ## Compatibility with perl HTTP servers
+///
+/// The following methods are added for compatibility with perl HTTP servers.
+///
+/// - `bool_arg`: translates booleans to `"0"`/`"1"`
+/// - `list_arg`: split lists so that they can be feed to perl's `split_list()`
+///
+/// These methods require the `perl-api-path-builder` feature.
+pub struct ApiPathBuilder {
+    url: String,
+    separator: char,
+}
+
+impl ApiPathBuilder {
+    /// Creates a new builder from a base path.
+    pub fn new<'a>(base: impl Into<Cow<'a, str>>) -> Self {
+        Self {
+            url: base.into().into_owned(),
+            separator: '?',
+        }
+    }
+
+    /// Adds an argument to the query.
+    ///
+    /// The name and value will be percent-encoded.
+    pub fn arg<T: std::fmt::Display>(mut self, name: &str, value: T) -> Self {
+        self.push_separator_and_name(name);
+        self.push_encoded(value.to_string().as_bytes());
+        self
+    }
+
+    /// Adds an optional argument to the query.
+    ///
+    /// Does nothing if the value is `None`. See [Self::arg] for more details.
+    pub fn maybe_arg<T: std::fmt::Display>(mut self, name: &str, value: &Option<T>) -> Self {
+        if let Some(value) = value {
+            self = self.arg(name, value);
+        }
+        self
+    }
+
+    /// Builds the url.
+    pub fn build(self) -> String {
+        self.url
+    }
+
+    fn push_separator_and_name(&mut self, name: &str) {
+        self.url.push(self.separator);
+        self.separator = '&';
+        self.push_encoded(name.as_bytes());
+        self.url.push('=');
+    }
+
+    fn push_encoded(&mut self, value: &[u8]) {
+        let enc_value = percent_encoding::percent_encode(value, percent_encoding::NON_ALPHANUMERIC);
+        self.url.extend(enc_value);
+    }
+}
+
+#[cfg(feature = "perl-api-path-builder")]
+impl ApiPathBuilder {
+    /// Adds a boolean arg in a perl-friendly fashion.
+    ///
+    /// ```rust
+    /// use proxmox_client::ApiPathBuilder;
+    ///
+    /// let node = "pve01";
+    /// let query = ApiPathBuilder::new(format!("/api2/extjs/nodes/{node}/storage"))
+    ///     .bool_arg("enabled", true)
+    ///     .build();
+    ///
+    /// assert_eq!(&query, "/api2/extjs/nodes/pve01/storage?enabled=1");
+    /// ```
+    ///
+    /// `true` will be converted into `"1"` and `false` to `"0"`.
+    pub fn bool_arg(mut self, name: &str, value: bool) -> Self {
+        self.push_separator_and_name(name);
+        if value {
+            self.url.push('1');
+        } else {
+            self.url.push('0');
+        };
+        self
+    }
+
+    /// Adds an optional boolean arg in a perl-friendly fashion.
+    ///
+    /// Does nothing if `value` is `None`. See [Self::bool_arg] for more
+    /// details.
+    pub fn maybe_bool_arg(mut self, name: &str, value: Option<bool>) -> Self {
+        if let Some(value) = value {
+            self = self.bool_arg(name, value);
+        }
+        self
+    }
+
+    /// Helper for building perl-friendly queries.
+    ///
+    /// For `<type>-list` entries we turn an array into a string ready for
+    /// perl's `split_list()` call.
+    ///
+    /// ```rust
+    /// use proxmox_client::api_path_builder::ApiPathBuilder;
+    ///
+    /// let content = vec!["backup", "images"];
+    /// let node = "my_node";
+    /// let query = &proxmox_client::ApiPathBuilder::new(format!("/api2/extjs/nodes/{node}/storage"))
+    ///     .maybe_list_arg("content", &content)
+    ///     .arg("type", "vm")
+    ///     .build();
+    ///
+    /// assert_eq!(&query, "/api2/extjs/nodes/my_node/storage?content=backup%00imagess");
+    /// ```
+    ///
+    /// The name and values will be percent-encoded.
+    pub fn list_arg<T: std::fmt::Display, I: IntoIterator<Item = T>>(
+        mut self,
+        name: &str,
+        values: I,
+    ) -> Self {
+        self.push_separator_and_name(name);
+        let mut list_separator = "";
+        for entry in values.into_iter() {
+            self.url.push_str(list_separator);
+            list_separator = "%00";
+            self.push_encoded(entry.to_string().as_bytes());
+        }
+        self
+    }
+
+    /// Helper for building perl-friendly queries.
+    ///
+    /// See [Self::list_arg] for more details.
+    pub fn maybe_list_arg<T: std::fmt::Display>(
+        mut self,
+        name: &str,
+        values: &Option<Vec<T>>,
+    ) -> Self {
+        if let Some(values) = values {
+            self = self.list_arg(name, values.iter());
+        };
+        self
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // See pdm_api_types::ConfigurationState.
+    #[derive(serde::Serialize)]
+    #[serde(rename_all = "kebab-case")]
+    enum ConfigurationState {
+        Active,
+    }
+    serde_plain::derive_display_from_serialize!(ConfigurationState);
+
+    // See pve_api_types::ClusterResourceKind.
+    #[derive(serde::Serialize)]
+    enum ClusterResourceKind {
+        #[serde(rename = "vm")]
+        Vm,
+    }
+    serde_plain::derive_display_from_serialize!(ClusterResourceKind);
+
+    #[test]
+    fn test_builder() {
+        let expected = "/api2/extjs/cluster/resources?type=vm";
+        let ty = ClusterResourceKind::Vm;
+
+        let query = ApiPathBuilder::new("/api2/extjs/cluster/resources")
+            .arg("type", ty)
+            .build();
+
+        assert_eq!(&query, expected);
+
+        let second_expected =
+            "/api2/extjs/pve/remotes/some-remote/qemu/100/config?state=active&node=myNode";
+        let state = ConfigurationState::Active;
+        let node = "myNode";
+        let snapshot = None::<&str>;
+
+        let second_query =
+            ApiPathBuilder::new("/api2/extjs/pve/remotes/some-remote/qemu/100/config")
+                .arg("state", state)
+                .arg("node", node)
+                .maybe_arg("snapshot", &snapshot)
+                .build();
+
+        assert_eq!(&second_query, &second_expected);
+    }
+}
+
+#[cfg(all(test, feature = "perl-api-path-builder"))]
+mod perl_tests {
+    use super::*;
+
+    #[test]
+    fn test_perl_builder() {
+        let history = true;
+        let local_only = false;
+        let start_time = 1000;
+
+        let expected_url =
+            "/api2/extjs/cluster/metrics/export?history=1&local%2Donly=0&start%2Dtime=1000";
+
+        let query = ApiPathBuilder::new("/api2/extjs/cluster/metrics/export")
+            .bool_arg("history", history)
+            .bool_arg("local-only", local_only)
+            .arg("start-time", start_time)
+            .build();
+        assert_eq!(expected_url, query);
+
+        let query_with_maybe = ApiPathBuilder::new("/api2/extjs/cluster/metrics/export")
+            .maybe_bool_arg("history", Some(history))
+            .maybe_bool_arg("local-only", Some(local_only))
+            .maybe_arg("start-time", &Some(start_time))
+            .build();
+
+        assert_eq!(expected_url, query_with_maybe);
+    }
+}
diff --git a/proxmox-client/src/lib.rs b/proxmox-client/src/lib.rs
index 00c48665..5978d501 100644
--- a/proxmox-client/src/lib.rs
+++ b/proxmox-client/src/lib.rs
@@ -14,6 +14,10 @@ pub use error::Error;
 pub use proxmox_login::tfa::TfaChallenge;
 pub use proxmox_login::{Authentication, Ticket};
 
+pub mod api_path_builder;
+
+pub use api_path_builder::ApiPathBuilder;
+
 pub(crate) mod auth;
 pub use auth::{AuthenticationKind, Token};
 
-- 
2.39.5



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