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

[Stefan Hanreich <s.hanreich@proxmox.com>]

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?

> +        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.

> +        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