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

["Lukas Wagner" <l.wagner@proxmox.com>]

Looks good to me, two small suggestions inline.

Some of the `pub` fn's and struct members don't have doc comments, would
be nice if you could add them as well (can also be in a followup if you
prefer).

On Tue Aug 26, 2025 at 2:31 PM CEST, 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.
>
> Short syntax summary:
> 'sometext'  : normal search term
> '+sometext' : required search term
> 'cat:text'  : looks for 'text' in the category 'cat'
>
> required terms have to exist in the resulting match, while optional
> ones are OR'd (so at least one optional match must exist)
>
> This is loosely inspired by various search syntaxes, e.g. the one from
> gitlab[0] or query-dsl from elastic[1]
>
> [1] https://docs.gitlab.com/user/search/advanced_search/
> [2] https://www.elastic.co/docs/reference/query-languages/query-dsl/query-dsl-simple-query-string-query
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  Cargo.toml                |   2 +
>  lib/pdm-search/Cargo.toml |  12 ++
>  lib/pdm-search/src/lib.rs | 282 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 296 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..85dddda
> --- /dev/null
> +++ b/lib/pdm-search/src/lib.rs
> @@ -0,0 +1,282 @@
> +//! 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::str::FromStr;
> +
> +use anyhow::bail;
> +
> +#[derive(Clone)]
> +pub struct Search {
> +    required_terms: Vec<SearchTerm>,
> +    optional_terms: Vec<SearchTerm>,
> +}
> +
> +impl Default for Search {
> +    fn default() -> Self {
> +        Self::new()
> +    }
> +}
> +
> +impl FromIterator<SearchTerm> for Search {
> +    fn from_iter<T: IntoIterator<Item = SearchTerm>>(iter: T) -> Self {
> +        let (optional_terms, required_terms) = iter.into_iter().partition(|term| term.optional);
> +
> +        Self {
> +            required_terms,
> +            optional_terms,
> +        }
> +    }
> +}
> +
> +impl<S: AsRef<str>> From<S> for Search {
> +    fn from(value: S) -> Self {
> +        value
> +            .as_ref()
> +            .split_whitespace()
> +            .filter_map(|term| term.parse().ok())
> +            .collect()
> +    }
> +}
> +
> +impl Search {
> +    pub fn new() -> Self {
> +        Self::with_terms(Vec::new())
> +    }
> +
> +    pub fn is_empty(&self) -> bool {
> +        self.required_terms.is_empty() && self.optional_terms.is_empty()
> +    }
> +
> +    pub fn with_terms(terms: Vec<SearchTerm>) -> Self {
> +        terms.into_iter().collect()
> +    }
> +
> +    /// 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;
> +        }
> +
> +        if self.required_terms.iter().map(&matches).any(|f| !f) {
> +            return false;
> +        }
> +
> +        if !self.optional_terms.is_empty() && self.optional_terms.iter().map(&matches).all(|f| !f) {
> +            return false;
> +        }
> +
> +        true
> +    }
> +
> +    /// 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 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(())
> +    }
> +}

Would be cool to also add two quick tests for category_value_required
and the Display trait implementation; I always find tests valuable as
additional documentation about how something is supposed to work.

> +
> +#[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: ToString>(term: S) -> Self {
> +        Self {
> +            value: term.to_string(),
> +            optional: false,
> +            category: None,
> +        }
> +    }
> +
> +    pub fn category<S: ToString>(mut self, category: Option<S>) -> Self {
> +        self.category = category.map(|s| s.to_string());
> +        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::{Search, 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"))
> +        );
> +        assert_eq!(
> +            "+foo:bar".parse::<SearchTerm>().unwrap(),
> +            SearchTerm::new("bar").category(Some("foo"))
> +        );
> +    }
> +
> +    #[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());
> +    }
> +
> +    #[test]
> +    fn match_tests() {
> +        let search = Search::from_iter(vec![
> +            SearchTerm::new("required1").optional(false),
> +            SearchTerm::new("required2").optional(false),
> +            SearchTerm::new("optional1").optional(true),
> +            SearchTerm::new("optional2").optional(true),
> +        ]);
> +
> +        // each case contains results for
> +        // required1, required2, optional1, optional2
> +        // and if it should match or not
> +        let cases = [
> +            (true, true, true, false, true),
> +            (true, true, false, true, true),
> +            (true, true, true, true, true),
> +            (true, true, false, false, false),
> +            (true, false, false, false, false),
> +            (false, false, false, false, false),
> +            (false, true, false, false, false),
> +            (false, false, true, true, false),
> +            (false, true, true, true, false),
> +            (true, false, true, true, false),
> +            (true, false, true, false, false),
> +            (false, true, true, false, false),
> +            (false, false, true, false, false),
> +            (true, false, false, true, false),
> +            (false, true, false, true, false),
> +            (false, false, false, true, false),
> +        ];

Might be a bit more readable if you split the inputs and outputs as a
2-tuple, e.g.:

let cases = [
   ((true, true, true, false), true)),
    ...
]

> +        for case in cases {

and then do a `for (input, expected_output)` here.

> +            assert!(
> +                search.matches(|term| {
> +                    match term.value.as_str() {
> +                        "required1" => case.0,
> +                        "required2" => case.1,
> +                        "optional1" => case.2,
> +                        "optional2" => case.3,
> +                        _ => unreachable!(),
> +                    }
> +                }) == case.4
> +            )
> +        }
> +    }
> +}



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