[pbs-devel] [PATCH proxmox] proxmox-client: add query builder
Max Carrara
m.carrara at proxmox.com
Fri Mar 28 13:37:14 CET 2025
On Wed Mar 26, 2025 at 3:44 PM CET, 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 looks pretty good to me! I've also looked at the usages of this
over at pdm-devel [2]. I haven't built and tested this patch and the
others [2] in tandem, but I did run `cargo test` for good measure
(always happy to see tests btw, nicely done!).
Since the tests and the callsites [2] are almost identical, I'm
confident that this works as expected. And I'm confident you built and
tested all of this yourself too, of course. ;)
There's one minor comment inline, but that can be addressed in a
follow-up, if you'd like.
Consider:
Reviewed-by: Max Carrara <m.carrara at proxmox.com>
[2]: https://lore.proxmox.com/pdm-devel/20250326150636.460010-1-m.sandoval@proxmox.com/T/#t
>
> 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.
In this case, this is a really small thing, but in general, whenever
you're exposing a struct as part of a public API, you want the struct to
behave in an *expected* or *interoperable* manner -- in other words, the
struct should implement a bunch of common traits wherever applicable
[api-traits]. I'm sure you're familiar with this, so pardon me if I tell
you something you already know ;P
In this specific case, I'd suggest adding
#[derive(Clone, Debug, PartialEq, Eq)]
to `ApiPathBuilder` below. In this case this isn't really that
important, so it IMHO doesn't really warrant a v2 (unless you want to
ofc), but it's just something to keep in mind in general when designing
an API, even if it's just a little thing.
With that being said, you almost always want to derive (or implement)
`Debug`, and here `Clone`, `PartialEq` and `Eq` are just for convenience
in the future. (Not that we're gonna go around cloning and comparing
builders, but still.)
The other common traits don't really make that much sense, obviously.
Anyhow, it's your call whether you wanna add this or not. I'd say it
doesn't hurt, but then again, it's nothing critical.
Good work overall btw! There's nothing else I can complain about. ;P
[api-traits]: https://rust-lang.github.io/api-guidelines/interoperability.html#types-eagerly-implement-common-traits-c-common-traits
> +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};
>
More information about the pbs-devel
mailing list