[pbs-devel] [PATCH proxmox] proxmox-client: add query builder
Wolfgang Bumiller
w.bumiller at proxmox.com
Tue Apr 15 14:20:35 CEST 2025
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 at 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
More information about the pbs-devel
mailing list