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

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

Wolfgang Bumiller <w.bumiller@proxmox.com> writes:

> On Wed, Mar 26, 2025 at 03:44:10PM +0100, Maximiliano Sandoval wrote:
>> 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.
>
> Missing `backticks` around `Self::arg`.
>
> Also, maybe use "[`arg`](Self::arg())" instead?
>
> This happens throughout the code below.
>
>> +///
>> +/// ```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")
>
> ^ Contains type=vm
>
>> +    ///     .build();
>> +    ///
>> +    /// assert_eq!(&query, "/api2/extjs/nodes/my_node/storage?content=backup%00imagess");
>
> ^ does not contain type=vm...
>
> iow. the test suite fails with `--all-features`
>
>> +    /// ```
>> +    ///
>> +    /// The name and values will be percent-encoded.
>> +    pub fn list_arg<T: std::fmt::Display, I: IntoIterator<Item = T>>(
>
> ^ This is quite long, please move it into a where caluse.
> Also, you can skip `T` via:
>
>     where
>         I: IntoIterator,
>         I::Item: Display,
>
> And since 1.79 associated type bounds are stable, so you can actually
> do:
>     where
>         I: IntoIterator<Item: Display>,
>
>
>
>> +        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 {
>
> Oh I so wish
>     where for<'a> &'a I: IntoIterator<T: Display>
> would actually *work* (to replace `Vec<T>` with an `I`, but it doesn't
> right now...
>
>> +        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;
>
> ↑ Drop the `pub` here
> And the newline ↓
>
>> +
>> +pub use api_path_builder::ApiPathBuilder;
>> +
>>  pub(crate) mod auth;
>>  pub use auth::{AuthenticationKind, Token};
>>  
>> -- 
>> 2.39.5


Superseded by https://lore.proxmox.com/pbs-devel/20250416113601.256829-1-m.sandoval@proxmox.com/T/#u.


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