From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 792F41FF187 for ; Mon, 25 Aug 2025 15:14:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 689D112F88; Mon, 25 Aug 2025 15:14:15 +0200 (CEST) Message-ID: <18d57db1-a6fc-43cd-a8ca-fb04fabd8cb4@proxmox.com> Date: Mon, 25 Aug 2025 15:14:10 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Datacenter Manager development discussion , Dominik Csapak References: <20250825090014.1138485-1-d.csapak@proxmox.com> <20250825090014.1138485-3-d.csapak@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20250825090014.1138485-3-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.709 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: [pdm-devel] [PATCH datacenter-manager v2 2/9] lib: add pdm-search crate X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" some comments inline, I think the FromIterator would be a nice improvement, the rest is just potential for future / follow-ups. On 8/25/25 11:01 AM, Dominik Csapak wrote: > Introduce a new create for search & filter related code. It currently > includes basic parsing & testing of search terms. Intended to be used on > some API calls that allow for more complex filters, such as the > resources API. > > Contains a `SearchTerm` and a `Search` struct. The former represents > a single term to search for, with an optional category and if it's > optional or not. The latter represents a full search with multiple > terms. > > Signed-off-by: Dominik Csapak > --- > Cargo.toml | 2 + > lib/pdm-search/Cargo.toml | 12 ++ > lib/pdm-search/src/lib.rs | 259 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 273 insertions(+) > create mode 100644 lib/pdm-search/Cargo.toml > create mode 100644 lib/pdm-search/src/lib.rs > > diff --git a/Cargo.toml b/Cargo.toml > index 08b9373..236f00b 100644 > --- a/Cargo.toml > +++ b/Cargo.toml > @@ -19,6 +19,7 @@ members = [ > "lib/pdm-api-types", > "lib/pdm-client", > "lib/pdm-config", > + "lib/pdm-search", > "lib/pdm-ui-shared", > > "cli/client", > @@ -86,6 +87,7 @@ pdm-api-types = { path = "lib/pdm-api-types" } > pdm-buildcfg = { path = "lib/pdm-buildcfg" } > pdm-config = { path = "lib/pdm-config" } > pdm-client = { version = "0.2", path = "lib/pdm-client" } > +pdm-search = { version = "0.2", path = "lib/pdm-search" } > pdm-ui-shared = { version = "0.2", path = "lib/pdm-ui-shared" } > proxmox-fido2 = { path = "cli/proxmox-fido2" } > > diff --git a/lib/pdm-search/Cargo.toml b/lib/pdm-search/Cargo.toml > new file mode 100644 > index 0000000..5f51e75 > --- /dev/null > +++ b/lib/pdm-search/Cargo.toml > @@ -0,0 +1,12 @@ > +[package] > +name = "pdm-search" > +description = "Proxmox Datacenter Manager shared ui modules" > +homepage = "https://www.proxmox.com" > + > +version.workspace = true > +edition.workspace = true > +license.workspace = true > +repository.workspace = true > + > +[dependencies] > +anyhow.workspace = true > diff --git a/lib/pdm-search/src/lib.rs b/lib/pdm-search/src/lib.rs > new file mode 100644 > index 0000000..8d6cca3 > --- /dev/null > +++ b/lib/pdm-search/src/lib.rs > @@ -0,0 +1,259 @@ > +//! Abstraction over a [`Search`] that contains multiple [`SearchTerm`]s. > +//! > +//! Provides methods to filter an item over a combination of such terms and > +//! construct them from text, and serialize them back to text. > +use std::{fmt::Display, str::FromStr}; > + > +use anyhow::bail; > + > +#[derive(Clone)] implement Default as well? > +pub struct Search { > + required_terms: Vec, > + optional_terms: Vec, > +} > + > +impl> From for Search { > + fn from(value: S) -> Self { > + let mut optional_terms = Vec::new(); > + let mut required_terms = Vec::new(); > + for term in value.as_ref().split_whitespace() { > + match term.parse::() { > + Ok(term) => { > + if term.optional { > + optional_terms.push(term) > + } else { > + required_terms.push(term) > + } > + } > + Err(_) => {} // ignore invalid search terms > + }> + } > + > + Self { > + required_terms, > + optional_terms, > + } > + } > +} this implementation could use a potential FromIterator implementation for Search, avoiding the duplicated code shared w/ with_terms? i.e. value.split_whitespace().filter_map(|term| term.parse().ok()).collect() > + > +impl Search { > + pub fn new() -> Self { > + Self::with_terms(Vec::new()) > + } use Default::default() here then? (see next mail for further reasoning). > + > + pub fn is_empty(&self) -> bool { > + self.required_terms.is_empty() && self.optional_terms.is_empty() > + } > + > + pub fn with_terms(terms: Vec) -> Self { > + let mut optional_terms = Vec::new(); > + let mut required_terms = Vec::new(); > + > + for term in terms { > + if term.optional { > + optional_terms.push(term); > + } else { > + required_terms.push(term); > + } > + } nit: partition could be used, although in the future there might be more than required / optional. > + > + Self { > + optional_terms, > + required_terms, > + } > + } potentially moving this into a FromIterator implementation would be more appropriate? Potentially a FromIterator or FromIterator)> might also be interesting (with respective >From implementations for SearchTerm)? From might also be interesting? or even a From> > + > + /// Test if the given `Fn(&SearchTerm) -> bool` for all [`SearchTerm`] configured matches > + /// > + /// Returns true if it matches considering the constraints: > + /// if there are no filters, returns true > + pub fn matches bool>(&self, matches: F) -> bool { > + if self.is_empty() { > + return true; > + } > + > + let optional_matches: Vec = self.optional_terms.iter().map(&matches).collect(); > + let required_matches: Vec = self.required_terms.iter().map(&matches).collect(); > + > + if !required_matches.is_empty() && required_matches.iter().any(|f| !f) { > + return false; > + } > + > + if !optional_matches.is_empty() && optional_matches.iter().all(|f| !f) { > + return false; > + } > + > + true > + } nit: we could avoid collecting altogether and shortcircuit, removing the need for calling is_empty twice as well? if !self.optional_matches.is_empty() && optional_matches.iter().map(&matches).all(..) for required_matches we don't even need the empty check since any is always false for empty collections > + > + /// Returns true if the combination of [`SearchTerm`]s require that this category value must be > + /// true. Useful to find out if some condition is required (e.g. type == 'remote') > + pub fn category_value_required(&self, category: &str, value: &str) -> bool { > + for term in &self.required_terms { > + if term.category.as_deref() == Some(category) && value.contains(&term.value) { > + return true; > + } > + } > + > + let mut optional_count = 0; > + > + for term in &self.optional_terms { > + if term.category.as_deref() == Some(category) && term.value == value { > + optional_count += 1; > + } > + } > + > + self.required_terms.is_empty() > + && self.optional_terms.len() == optional_count > + && optional_count > 0 > + } > +} > + > +impl Default for Search { > + fn default() -> Self { > + Self::new() > + } > +} > + > +impl std::fmt::Display for Search { > + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { > + for (count, term) in self.required_terms.iter().enumerate() { > + if count != 0 { > + write!(f, " ")?; > + } > + > + write!(f, "{term}")?; > + } > + > + if !self.required_terms.is_empty() && !self.optional_terms.is_empty() { > + write!(f, " ")?; > + } > + > + for (count, term) in self.optional_terms.iter().enumerate() { > + if count != 0 { > + write!(f, " ")?; > + } > + > + write!(f, "{term}")?; > + } > + > + Ok(()) > + } > +} > + > +#[derive(Debug, Clone, PartialEq)] > +pub struct SearchTerm { > + optional: bool, > + pub value: String, > + pub category: Option, > +} > + > +impl SearchTerm { > + /// Creates a new [`SearchTerm`]. > + pub fn new>(term: S) -> Self { are spaces potentially a problem here? > + Self { > + value: term.into(), > + optional: false, > + category: None, > + } > + } > + > + pub fn category>(mut self, category: Option) -> Self { same remark w.r.t. spaces Is ToString potentially better as trait bound here, since I often see .to_string() at the call sites? I usually prefer impl Into> for functions taking optional values, since it avoids having to type out Some(_) everywhere, but in this case with Into this breaks type inference when passing None. > + self.category = category.map(|s| s.into()); > + self > + } > + > + pub fn optional(mut self, optional: bool) -> Self { > + self.optional = optional; > + self > + } > +} > + > +impl FromStr for SearchTerm { > + type Err = anyhow::Error; > + > + fn from_str(s: &str) -> Result { > + let mut optional = true; > + let mut term: String = s.into(); > + if term.starts_with("+") { > + optional = false; > + term.remove(0); > + } > + > + let (term, category) = if let Some(idx) = term.find(":") { > + let mut real_term = term.split_off(idx); > + real_term.remove(0); // remove ':' > + (real_term, Some(term)) > + } else { > + (term, None) > + }; > + > + if term.is_empty() { > + bail!("term cannot be empty"); > + } > + > + if category == Some("".into()) { > + bail!("category cannot be empty"); > + } > + > + Ok(SearchTerm::new(term).optional(optional).category(category)) > + } > +} > + > +impl std::fmt::Display for SearchTerm { > + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { > + if !self.optional { > + f.write_str("+")?; > + } > + > + if let Some(cat) = &self.category { > + f.write_str(cat)?; > + f.write_str(":")?; > + } > + > + f.write_str(&self.value) > + } > +} > + > +#[cfg(test)] > +mod tests { > + use crate::SearchTerm; > + > + #[test] > + fn parse_test_simple_filter() { > + assert_eq!( > + "foo".parse::().unwrap(), > + SearchTerm::new("foo").optional(true), > + ); > + } > + > + #[test] > + fn parse_test_requires_filter() { > + assert_eq!( > + "+foo".parse::().unwrap(), > + SearchTerm::new("foo"), > + ); > + } > + > + #[test] > + fn parse_test_category_filter() { > + assert_eq!( > + "foo:bar".parse::().unwrap(), > + SearchTerm::new("bar") > + .optional(true) > + .category(Some("foo".into())) > + ); > + assert_eq!( > + "+foo:bar".parse::().unwrap(), > + SearchTerm::new("bar").category(Some("foo".into())) > + ); > + } > + > + #[test] > + fn parse_test_invalid_filter() { > + assert!(":bar".parse::().is_err()); > + assert!("+cat:".parse::().is_err()); > + assert!("+".parse::().is_err()); > + assert!(":".parse::().is_err()); > + } > +} _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel