Re: [pdm-devel] [RFC PATCH 1/3] pdm-client: add query builder

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

On Fri, Feb 14, 2025 at 03:11:25PM +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 why we use a `Cow<'_, str>` on the
> constructor.
> 
> 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 marked as RFC as I think this is better suited for proxmox-client and
> here it is easier to showcase in the second and third commit how the API falls
> into place.

Given that `add_query_arg` (and some variations) currently exist as
duplicates in pdm-client, pve-api-types and pdm/server, all of which
base their http part on proxmox-client - moving its functionality one
way or another into proxmox-client makes sense.

The builder seems fine to me, if you want to prepare patches for
proxmox-client with it.

Two minor nits below.

> 
>  lib/pdm-client/Cargo.toml              |   1 +
>  lib/pdm-client/src/api_path_builder.rs | 207 +++++++++++++++++++++++++
>  2 files changed, 208 insertions(+)
>  create mode 100644 lib/pdm-client/src/api_path_builder.rs
> 
> diff --git a/lib/pdm-client/Cargo.toml b/lib/pdm-client/Cargo.toml
> index fe5ddf7..af42c43 100644
> --- a/lib/pdm-client/Cargo.toml
> +++ b/lib/pdm-client/Cargo.toml
> @@ -26,3 +26,4 @@ pbs-api-types.workspace = true
>  [features]
>  default = []
>  hyper-client = [ "proxmox-client/hyper-client" ]
> +perl-api-path-builder = []
> diff --git a/lib/pdm-client/src/api_path_builder.rs b/lib/pdm-client/src/api_path_builder.rs
> new file mode 100644
> index 0000000..0058489
> --- /dev/null
> +++ b/lib/pdm-client/src/api_path_builder.rs
> @@ -0,0 +1,207 @@
> +use std::borrow::Cow;
> +
> +/// Builder for urls with a query.
> +///
> +/// The [Self::arg] method can be used to add multiple arguments to the query.
> +///
> +/// ```rust
> +/// use pdm_client::api_path_builder::ApiPathBuilder;
> +///
> +/// let query = ApiPathBuilder::new("/api2/extjs/cluster/resources")
> +///     .arg("type", pve_api_types::ClusterResourceKind::Vm)
> +///     .build();
> +///
> +/// assert_eq!(&query, "/api2/extjs/cluster/resources?type=vm");
> +/// ```
> +///
> +/// ## Compatibility with perl clients
> +///
> +/// The methods `bool_arg` and `list_arg` were added to translate booleans as
> +/// `"0"`/`"1"` and split lists so that they can be feed to perl's
> +/// `split_list()` respectively. These methods require the
> +/// `perl-api-path-builder` feature.
> +pub struct ApiPathBuilder {
> +    url: String,
> +    separator: char,
> +}
> +
> +impl ApiPathBuilder {
> +    /// Creates a new builder.
> +    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 value is percent-encoded.
> +    pub fn arg<T: std::fmt::Display>(mut self, name: &str, value: T) -> Self {
> +        self.push_name_and_separator(name);
> +        self.push_encoded(value);
> +        self
> +    }
> +
> +    /// Adds an optional argument to the query.
> +    ///
> +    /// Does nothing if the value is `None`. See [Self::arg].
> +    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_name_and_separator(&mut self, name: &str) {
> +        self.url.push(self.separator);
> +        self.separator = '&';
> +        self.url.push_str(name);

With the builder being a more public facing thing now, it would probably
make sense to also encode the name rather than pushing it as-is.

> +        self.url.push('=');
> +    }
> +
> +    fn push_encoded<T: std::fmt::Display>(&mut self, value: T) {
> +        let str_value = value.to_string();
> +        let enc_value = percent_encoding::percent_encode(
> +            str_value.as_bytes(),
> +            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.
> +    ///
> +    /// `true` will be converted into `"1"` and `false` to `"0"`.
> +    pub fn bool_arg(mut self, name: &str, value: bool) -> Self {
> +        self.push_name_and_separator(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].
> +    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.
> +    ///
> +    /// The values are percent-encoded.
> +    pub fn list_arg<T: std::fmt::Display, P: Iterator<Item = T>>(

I think it is more common to use `IntoIterator<Item = T>` as a bound for
such functions.

> +        mut self,
> +        name: &str,
> +        values: P,
> +    ) -> Self {
> +        self.push_name_and_separator(name);
> +        let mut list_separator = "";
> +        for entry in values {
> +            self.url.push_str(list_separator);
> +            list_separator = "%00";
> +            self.push_encoded(entry);
> +        }
> +        self
> +    }
> +
> +    /// Helper for building perl-friendly queries.
> +    ///
> +    /// See [Self::list_arg].
> +    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 pdm_api_types::ConfigurationState;
> +    use pve_api_types::ClusterResourceKind;
> +
> +    use super::*;
> +
> +    #[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::*;
> +
> +    use pve_api_types::client::{add_query_arg, add_query_bool};
> +
> +    #[test]
> +    fn test_perl_builder() {
> +        let history = true;
> +        let local_only = false;
> +        let start_time = 1000;
> +
> +        let (mut query, mut sep) = (String::new(), '?');
> +        add_query_bool(&mut query, &mut sep, "history", Some(history));
> +        add_query_bool(&mut query, &mut sep, "local-only", Some(local_only));
> +        add_query_arg(&mut query, &mut sep, "start-time", &Some(start_time));
> +        let url = format!("/api2/extjs/cluster/metrics/export{query}");
> +
> +        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!(url, query);
> +
> +        let maybe_arg = 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!(url, maybe_arg);
> +    }
> +}
> -- 
> 2.39.5


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