[pbs-devel] [PATCH proxmox] proxmox-client: add query builder
Maximiliano Sandoval
m.sandoval at proxmox.com
Wed Apr 16 13:36:52 CEST 2025
Wolfgang Bumiller <w.bumiller at 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 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
Superseded by https://lore.proxmox.com/pbs-devel/20250416113601.256829-1-m.sandoval@proxmox.com/T/#u.
More information about the pbs-devel
mailing list