Re: [pdm-devel] [PATCH datacenter-manager v2 2/9] lib: add pdm-search crate

[Dominik Csapak <d.csapak@proxmox.com>]

On 8/25/25 3:14 PM, Stefan Hanreich wrote:
> some comments inline, I think the FromIterator<Item = SearchTerm> 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 <d.csapak@proxmox.com>
>> ---
>>   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<SearchTerm>,
>> +    optional_terms: Vec<SearchTerm>,
>> +}
>> +
>> +impl<S: AsRef<str>> From<S> 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::<SearchTerm>() {
>> +                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<SearchTerm>) -> 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<Item = SearchTerm>
> implementation would be more appropriate?
> 
> Potentially a FromIterator<Item = (String, String)> or FromIterator<Item
> = (String, Option<String>)> might also be interesting (with respective
>  From implementations for SearchTerm)?
> 
> From<SearchTerm> might also be interesting? or even a From<T: impl
> Into<SearchTerm>>
> 
>> +
>> +    /// 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<F: Fn(&SearchTerm) -> bool>(&self, matches: F) -> bool {
>> +        if self.is_empty() {
>> +            return true;
>> +        }
>> +
>> +        let optional_matches: Vec<bool> = self.optional_terms.iter().map(&matches).collect();
>> +        let required_matches: Vec<bool> = 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<String>,
>> +}
>> +
>> +impl SearchTerm {
>> +    /// Creates a new [`SearchTerm`].
>> +    pub fn new<S: Into<String>>(term: S) -> Self {
> 
> are spaces potentially a problem here?
> 

since we use 'split_whitespace' when we convert from the search
string to the terms, it shouldn't be?

isn't be able to generate a SearchTerm directly, so I don't
see how it's currently a problem

>> +        Self {
>> +            value: term.into(),
>> +            optional: false,
>> +            category: None,
>> +        }
>> +    }
>> +
>> +    pub fn category<S: Into<String>>(mut self, category: Option<S>) -> 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<Option<>> for functions taking optional
> values, since it avoids having to type out Some(_) everywhere, but in
> this case with Into<String> this breaks type inference when passing None.

yeah Into<Option> does not work for a trait type (without manually 
specifying the type when using none

but yeah, i can try ToString, i wanted to avoid the extra copying in 
case we can convert the object into a string

> 
>> +        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<Self, Self::Err> {
>> +        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::<SearchTerm>().unwrap(),
>> +            SearchTerm::new("foo").optional(true),
>> +        );
>> +    }
>> +
>> +    #[test]
>> +    fn parse_test_requires_filter() {
>> +        assert_eq!(
>> +            "+foo".parse::<SearchTerm>().unwrap(),
>> +            SearchTerm::new("foo"),
>> +        );
>> +    }
>> +
>> +    #[test]
>> +    fn parse_test_category_filter() {
>> +        assert_eq!(
>> +            "foo:bar".parse::<SearchTerm>().unwrap(),
>> +            SearchTerm::new("bar")
>> +                .optional(true)
>> +                .category(Some("foo".into()))
>> +        );
>> +        assert_eq!(
>> +            "+foo:bar".parse::<SearchTerm>().unwrap(),
>> +            SearchTerm::new("bar").category(Some("foo".into()))
>> +        );
>> +    }
>> +
>> +    #[test]
>> +    fn parse_test_invalid_filter() {
>> +        assert!(":bar".parse::<SearchTerm>().is_err());
>> +        assert!("+cat:".parse::<SearchTerm>().is_err());
>> +        assert!("+".parse::<SearchTerm>().is_err());
>> +        assert!(":".parse::<SearchTerm>().is_err());
>> +    }
>> +}
> 
> 



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel