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 [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 2B30B1FF164
for <inbox@lore.proxmox.com>; Fri, 28 Mar 2025 13:37:55 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 07D0D1B86;
Fri, 28 Mar 2025 13:37:49 +0100 (CET)
Mime-Version: 1.0
Date: Fri, 28 Mar 2025 13:37:14 +0100
Message-Id: <D8RWKPISRLBU.1L59OI8VN1QC4@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: <20250326144410.430616-1-m.sandoval@proxmox.com>
In-Reply-To: <20250326144410.430616-1-m.sandoval@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -1.428 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
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLACK 3 Contains an URL listed in the URIBL blacklist
[rust-lang.github.io]
Subject: Re: [pbs-devel] [PATCH proxmox] 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 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@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@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};
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel