From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 72E051FF172
for <inbox@lore.proxmox.com>; Wed, 16 Apr 2025 15:19:18 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 46073379FC;
Wed, 16 Apr 2025 15:19:16 +0200 (CEST)
Mime-Version: 1.0
Date: Wed, 16 Apr 2025 15:18:42 +0200
Message-Id: <D983CSUPOR2T.1BXOML81AVCFN@proxmox.com>
To: "Proxmox Backup Server development discussion"
<pbs-devel@lists.proxmox.com>
From: "Max Carrara" <m.carrara@proxmox.com>
X-Mailer: aerc 0.18.2-0-ge037c095a049
References: <20250416113601.256829-1-m.sandoval@proxmox.com>
In-Reply-To: <20250416113601.256829-1-m.sandoval@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.079 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pbs-devel] [PATCH proxmox v2] proxmox-client: add query builder
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>
On Wed Apr 16, 2025 at 1:36 PM CEST, 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>
> ---
>
> Differences from v1:
> - Fix tests in docs
> - Backtick function names in docstrings
> - Use where clause in list_arg
> - Make api_path_build mod private
> - Add Clone and Debug
This time also ran the doc tests (I always forget those, *sigh*)---one
small comment inline.
Otherwise LGTM. I like the changes Wolfgang suggested a lot.
Consider:
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
>
> proxmox-client/Cargo.toml | 5 +
> proxmox-client/src/api_path_builder.rs | 242 +++++++++++++++++++++++++
> proxmox-client/src/lib.rs | 3 +
> 3 files changed, 250 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..91a0a235
> --- /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 [`arg`](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.
> +#[derive(Clone, Debug)]
> +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 [`arg`](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 [`bool_arg`](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;
s/api_path_builder:://
Seems you missed that one, but that one can be fixed up when applying.
> + ///
> + /// let content = vec!["backup", "images"];
> + /// let node = "my_node";
> + /// let query = proxmox_client::ApiPathBuilder::new(format!("/api2/extjs/nodes/{node}/storage"))
> + /// .list_arg("content", &content)
> + /// .arg("type", "vm")
> + /// .build();
> + ///
> + /// assert_eq!(&query, "/api2/extjs/nodes/my_node/storage?content=backup%00images&type=vm");
> + /// ```
> + ///
> + /// The name and values will be percent-encoded.
> + pub fn list_arg<I>(mut self, name: &str, values: I) -> Self
> + where
> + I: IntoIterator<Item: std::fmt::Display>,
> + {
> + 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 [`list_arg`](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..e802f4ce 100644
> --- a/proxmox-client/src/lib.rs
> +++ b/proxmox-client/src/lib.rs
> @@ -14,6 +14,9 @@ pub use error::Error;
> pub use proxmox_login::tfa::TfaChallenge;
> pub use proxmox_login::{Authentication, Ticket};
>
> +mod api_path_builder;
> +pub use api_path_builder::ApiPathBuilder;
> +
> pub(crate) mod auth;
> pub use auth::{AuthenticationKind, Token};
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel