[pdm-devel] [PATCH datacenter-manager v4 00/10] implement more complex search syntax

[pdm-devel] [PATCH datacenter-manager v4 00/10] implement more complex search syntax

[Dominik Csapak]

this introduces a more complex search syntax for the resources api call
and uses that with the dashboard to show relevant resources, e.g.
when clicking on the remotes panel when there are failed remotes, it
adds a search to the box that filters for offline remotes. Same
for clicking on the running vm count, etc.

The syntax is a first draft of mine, we can still tweak and change it
as we see fit, but it's a start.

a 'normal' search term gets filtered to id/name so that does not change
as before, but you can now specify 'categories' with `category:value`
e.g. it's now possible to search for `type:remote` or `status:offline`

it also adds the possibility to mark terms as required like this:

+someterm

required terms have to exist in the resulting resource, while optional
ones are OR'd (so at least one optional match must exist)

Not implemented yet are (but can be done afterwards):
* GUI for filtering
* Include subscription status
* negation of terms (this get's complicated fast ;) )

changes from v3:
* use 'starts_with' for type/status filtering
* add missing doc comments
* add more tests
* refactor guest panel in dashboard to use `List`
* add icon in guest panel for discoverability

changes from v2:
* incorporated stefans feedback for FromIterator, Default, etc.
* added tests for the 'matches' function
* add required search term for the guest 'All' text

changes from v1:
* added commit to improve text width
* added commit to add clear trigger to search box
* rebased on master


Dominik Csapak (10):
  pdm-api-types: resources: add helper methods for fields
  lib: add pdm-search crate
  server: api: resources: add more complex filter syntax
  ui: add possibility to insert into search box
  ui: dashboard: remotes panel: open search on click
  ui: dashboard: guest panel: search for guest states when clicking on
    them
  ui: dashboard: search for nodes when clicking on the nodes panel
  ui: search box: add clear trigger
  ui: dashboard: guest panel: use `List` instead of `DataTable`
  ui: dashboard: guest panel: add search icon for better discoverability

 Cargo.toml                        |   2 +
 lib/pdm-api-types/src/resource.rs |  27 +++
 lib/pdm-search/Cargo.toml         |  12 ++
 lib/pdm-search/src/lib.rs         | 339 ++++++++++++++++++++++++++++++
 server/Cargo.toml                 |   1 +
 server/src/api/resources.rs       |  80 +++++--
 ui/Cargo.toml                     |   1 +
 ui/src/dashboard/guest_panel.rs   | 209 ++++++++++--------
 ui/src/dashboard/mod.rs           |  46 +++-
 ui/src/dashboard/remote_panel.rs  |  30 ++-
 ui/src/lib.rs                     |   3 +
 ui/src/main.rs                    |  17 +-
 ui/src/pve/mod.rs                 |  10 +
 ui/src/search_provider.rs         |  35 +++
 ui/src/widget/search_box.rs       |  40 +++-
 15 files changed, 737 insertions(+), 115 deletions(-)
 create mode 100644 lib/pdm-search/Cargo.toml
 create mode 100644 lib/pdm-search/src/lib.rs
 create mode 100644 ui/src/search_provider.rs

-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 01/10] pdm-api-types: resources: add helper methods for fields

[Dominik Csapak]

namely 'resource_type' and 'status'. All resources have those fields
in one way or another, so adding a helper that one does not have to
use `match` on every call site makes code there shorter.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v4
 lib/pdm-api-types/src/resource.rs | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/pdm-api-types/src/resource.rs b/lib/pdm-api-types/src/resource.rs
index 6227855..0dfadd5 100644
--- a/lib/pdm-api-types/src/resource.rs
+++ b/lib/pdm-api-types/src/resource.rs
@@ -62,6 +62,33 @@ impl Resource {
             Resource::PbsDatastore(r) => r.name.as_str(),
         }
     }
+
+    pub fn resource_type(&self) -> &str {
+        match self {
+            Resource::PveStorage(_) => "storage",
+            Resource::PveQemu(_) => "qemu",
+            Resource::PveLxc(_) => "lxc",
+            Resource::PveNode(_) | Resource::PbsNode(_) => "node",
+            Resource::PbsDatastore(_) => "datastore",
+        }
+    }
+
+    pub fn status(&self) -> &str {
+        match self {
+            Resource::PveStorage(r) => r.status.as_str(),
+            Resource::PveQemu(r) => r.status.as_str(),
+            Resource::PveLxc(r) => r.status.as_str(),
+            Resource::PveNode(r) => r.status.as_str(),
+            Resource::PbsNode(r) => {
+                if r.uptime > 0 {
+                    "online"
+                } else {
+                    "offline"
+                }
+            }
+            Resource::PbsDatastore(_) => "online",
+        }
+    }
 }
 
 #[api(
-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 02/10] lib: add pdm-search crate

[Dominik Csapak]

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>
---
changes from v3:
* add missing doc comments
* more tests

 Cargo.toml                |   2 +
 lib/pdm-search/Cargo.toml |  12 ++
 lib/pdm-search/src/lib.rs | 339 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 353 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 f07733d..ae2816a 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..4854166
--- /dev/null
+++ b/lib/pdm-search/src/lib.rs
@@ -0,0 +1,339 @@
+//! 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 {
+    /// Create a new empty [`Search`]
+    pub fn new() -> Self {
+        Self::with_terms(Vec::new())
+    }
+
+    /// Returns true if no [`SearchTerm`] exist
+    pub fn is_empty(&self) -> bool {
+        self.required_terms.is_empty() && self.optional_terms.is_empty()
+    }
+
+    /// Create a new [`Search`] with the given [`SearchTerm`]s
+    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(())
+    }
+}
+
+#[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,
+        }
+    }
+
+    /// Builder style method to set the category
+    pub fn category<S: ToString>(mut self, category: Option<S>) -> Self {
+        self.category = category.map(|s| s.to_string());
+        self
+    }
+
+    /// Builder style method to mark this [`SearchTerm`] as optional
+    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),
+        ];
+        for (input, expected) in cases {
+            assert!(
+                search.matches(|term| {
+                    match term.value.as_str() {
+                        "required1" => input.0,
+                        "required2" => input.1,
+                        "optional1" => input.2,
+                        "optional2" => input.3,
+                        _ => unreachable!(),
+                    }
+                }) == expected
+            )
+        }
+    }
+
+    #[test]
+    fn category_value_required() {
+        let search: Search = Search::from_iter(vec![SearchTerm::new("foo")]);
+        assert!(!search.category_value_required("bar", "baz"));
+
+        let search: Search = Search::from_iter(vec![SearchTerm::new("foo").optional(true)]);
+        assert!(!search.category_value_required("bar", "baz"));
+
+        let search: Search = Search::from_iter(vec![SearchTerm::new("foo").category(Some("bar"))]);
+        assert!(!search.category_value_required("bar", "baz"));
+
+        let search: Search = Search::from_iter(vec![SearchTerm::new("baz").category(Some("bar"))]);
+        assert!(search.category_value_required("bar", "baz"));
+
+        let search: Search = Search::from_iter(vec![SearchTerm::new("foo")
+            .optional(true)
+            .category(Some("bar"))]);
+        assert!(!search.category_value_required("bar", "baz"));
+
+        let search: Search = Search::from_iter(vec![SearchTerm::new("baz")
+            .optional(true)
+            .category(Some("bar"))]);
+        assert!(search.category_value_required("bar", "baz"));
+
+        let search: Search = Search::from_iter(vec![
+            SearchTerm::new("baz").optional(true).category(Some("bar")),
+            SearchTerm::new("foo").optional(true),
+        ]);
+        assert!(!search.category_value_required("bar", "baz"));
+    }
+
+    #[test]
+    fn test_display() {
+        let term = SearchTerm::new("foo");
+        assert_eq!("+foo", &term.to_string());
+
+        let term = SearchTerm::new("foo").optional(true);
+        assert_eq!("foo", &term.to_string());
+
+        let term = SearchTerm::new("foo").optional(false);
+        assert_eq!("+foo", &term.to_string());
+
+        let term = SearchTerm::new("foo").category(Some("bar"));
+        assert_eq!("+bar:foo", &term.to_string());
+
+        let term = SearchTerm::new("foo").optional(true).category(Some("bar"));
+        assert_eq!("bar:foo", &term.to_string());
+
+        let term = SearchTerm::new("foo").optional(false).category(Some("bar"));
+        assert_eq!("+bar:foo", &term.to_string());
+    }
+}
-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 03/10] server: api: resources: add more complex filter syntax

[Dominik Csapak]

by using the new pdm-search crate for the resources api call.

We have to do 3 filter passes:
* one fast pass for remotes if the filter are constructed in a way that
  must filter to 'remote' (in this case we don't have to look at/return
  the resources at all, and can skip remotes that don't match)
* a pass for the resources
* a second pass for the remotes that check if they match for
  remote/non-remote mixed results

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v3:
* use starts_with for filtering type/status
 server/Cargo.toml           |  1 +
 server/src/api/resources.rs | 80 +++++++++++++++++++++++++++++++------
 2 files changed, 69 insertions(+), 12 deletions(-)

diff --git a/server/Cargo.toml b/server/Cargo.toml
index 24a2e40..ada7c80 100644
--- a/server/Cargo.toml
+++ b/server/Cargo.toml
@@ -74,6 +74,7 @@ proxmox-acme-api = { workspace = true, features = [ "impl" ] }
 pdm-api-types.workspace = true
 pdm-buildcfg.workspace = true
 pdm-config.workspace = true
+pdm-search.workspace = true
 
 pve-api-types = { workspace = true, features = [ "client" ] }
 pbs-api-types.workspace = true
diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
index fb6218a..8114944 100644
--- a/server/src/api/resources.rs
+++ b/server/src/api/resources.rs
@@ -15,12 +15,13 @@ use pdm_api_types::subscription::{
     NodeSubscriptionInfo, RemoteSubscriptionState, RemoteSubscriptions, SubscriptionLevel,
 };
 use pdm_api_types::{Authid, PRIV_RESOURCE_AUDIT};
+use pdm_search::{Search, SearchTerm};
 use proxmox_access_control::CachedUserInfo;
 use proxmox_router::{
     http_bail, list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap,
 };
 use proxmox_rrd_api_types::RrdTimeframe;
-use proxmox_schema::api;
+use proxmox_schema::{api, parse_boolean};
 use proxmox_sortable_macro::sortable;
 use proxmox_subscription::SubscriptionStatus;
 use pve_api_types::{ClusterResource, ClusterResourceType};
@@ -45,6 +46,44 @@ const SUBDIRS: SubdirMap = &sorted!([
     ),
 ]);
 
+fn resource_matches_search_term(resource: &Resource, term: &SearchTerm) -> bool {
+    match &term.category {
+        Some(category) => match category.as_str() {
+            "type" => resource.resource_type().starts_with(&term.value),
+            "name" => resource.name().contains(&term.value),
+            "id" => resource.id().contains(&term.value),
+            "status" => resource.status().starts_with(&term.value),
+            "template" => match resource {
+                Resource::PveQemu(PveQemuResource { template, .. }, ..)
+                | Resource::PveLxc(PveLxcResource { template, .. }) => {
+                    match parse_boolean(&term.value) {
+                        Ok(boolean) => boolean == *template,
+                        Err(_) => false,
+                    }
+                }
+                _ => false,
+            },
+            "remote" => true, // this has to be checked beforehand
+            _ => false,
+        },
+        None => resource.name().contains(&term.value) || resource.id().contains(&term.value),
+    }
+}
+
+fn remote_matches_search_term(remote_name: &str, online: Option<bool>, term: &SearchTerm) -> bool {
+    match term.category.as_deref() {
+        Some("remote" | "name" | "id") => remote_name.contains(&term.value),
+        Some("type") => "remote".starts_with(&term.value),
+        Some("status") => match online {
+            None => true,
+            Some(true) => "online".starts_with(&term.value),
+            Some(false) => "offline".starts_with(&term.value),
+        },
+        None => remote_name.contains(&term.value) || "remote".contains(&term.value),
+        Some(_) => false,
+    }
+}
+
 #[api(
     // FIXME:: see list-like API calls in resource routers, we probably want more fine-grained
     // checks..
@@ -104,6 +143,10 @@ pub(crate) async fn get_resources_impl(
     let (remotes_config, _) = pdm_config::remotes::config()?;
     let mut join_handles = Vec::new();
 
+    let filters = search.map(Search::from).unwrap_or_default();
+
+    let remotes_only = filters.category_value_required("type", "remote");
+
     for (remote_name, remote) in remotes_config {
         if let Some(ref auth_id) = opt_auth_id {
             let remote_privs = user_info.lookup_privs(auth_id, &["resource", &remote_name]);
@@ -111,12 +154,27 @@ pub(crate) async fn get_resources_impl(
                 continue;
             }
         }
+
+        if remotes_only
+            && !filters.matches(|term| remote_matches_search_term(&remote_name, None, term))
+        {
+            continue;
+        }
+        let filter = filters.clone();
         let handle = tokio::spawn(async move {
-            let (resources, error) = match get_resources_for_remote(remote, max_age).await {
+            let (mut resources, error) = match get_resources_for_remote(remote, max_age).await {
                 Ok(resources) => (resources, None),
                 Err(error) => (Vec::new(), Some(error.to_string())),
             };
 
+            if remotes_only {
+                resources.clear();
+            } else if !filter.is_empty() {
+                resources.retain(|resource| {
+                    filter.matches(|filter| resource_matches_search_term(resource, filter))
+                });
+            }
+
             RemoteResources {
                 remote: remote_name,
                 resources,
@@ -132,17 +190,15 @@ pub(crate) async fn get_resources_impl(
         remote_resources.push(handle.await?);
     }
 
-    if let Some(search) = search {
-        // FIXME implement more complex filter syntax
-        remote_resources.retain_mut(|res| {
-            if res.remote.contains(&search) {
-                true
-            } else {
-                res.resources
-                    .retain(|res| res.id().contains(&search) || res.name().contains(&search));
-                !res.resources.is_empty()
+    if !filters.is_empty() {
+        remote_resources.retain(|res| {
+            if !res.resources.is_empty() {
+                return true;
             }
-        });
+            filters.matches(|filter| {
+                remote_matches_search_term(&res.remote, Some(res.error.is_none()), filter)
+            })
+        })
     }
 
     Ok(remote_resources)
-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 04/10] ui: add possibility to insert into search box

[Dominik Csapak]

by implementing a 'SearchProvider' context. This enables us to
insert a search term from everywhere. This can be helpful e.g. if we
want to prefill the search box with a specific pattern

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v4
 ui/Cargo.toml               |  1 +
 ui/src/lib.rs               |  3 +++
 ui/src/main.rs              | 17 ++++++++++++-----
 ui/src/search_provider.rs   | 35 +++++++++++++++++++++++++++++++++++
 ui/src/widget/search_box.rs | 26 +++++++++++++++++++++-----
 5 files changed, 72 insertions(+), 10 deletions(-)
 create mode 100644 ui/src/search_provider.rs

diff --git a/ui/Cargo.toml b/ui/Cargo.toml
index f87db6b..1c68909 100644
--- a/ui/Cargo.toml
+++ b/ui/Cargo.toml
@@ -43,6 +43,7 @@ pbs-api-types = "1.0.3"
 pdm-api-types = { version = "0.2", path = "../lib/pdm-api-types" }
 pdm-ui-shared = { version = "0.2", path = "../lib/pdm-ui-shared" }
 pdm-client = { version = "0.2", path = "../lib/pdm-client" }
+pdm-search = { version = "0.2", path = "../lib/pdm-search" }
 
 [patch.crates-io]
 # proxmox-client = { path = "../../proxmox/proxmox-client" }
diff --git a/ui/src/lib.rs b/ui/src/lib.rs
index e3755ec..edb50f9 100644
--- a/ui/src/lib.rs
+++ b/ui/src/lib.rs
@@ -21,6 +21,9 @@ pub use remotes::RemoteConfigPanel;
 mod top_nav_bar;
 pub use top_nav_bar::TopNavBar;
 
+mod search_provider;
+pub use search_provider::SearchProvider;
+
 mod dashboard;
 pub use dashboard::Dashboard;
 use yew_router::prelude::RouterScopeExt;
diff --git a/ui/src/main.rs b/ui/src/main.rs
index 6e2c9b2..be0c10c 100644
--- a/ui/src/main.rs
+++ b/ui/src/main.rs
@@ -22,7 +22,7 @@ use proxmox_yew_comp::{
 
 //use pbs::MainMenu;
 use pdm_api_types::subscription::{RemoteSubscriptionState, RemoteSubscriptions};
-use pdm_ui::{register_pve_tasks, MainMenu, RemoteList, TopNavBar};
+use pdm_ui::{register_pve_tasks, MainMenu, RemoteList, SearchProvider, TopNavBar};
 
 type MsgRemoteList = Result<RemoteList, Error>;
 
@@ -46,6 +46,7 @@ struct DatacenterManagerApp {
     remote_list: RemoteList,
     remote_list_error: Option<String>,
     remote_list_timeout: Option<Timeout>,
+    search_provider: SearchProvider,
 }
 
 async fn check_subscription() -> Msg {
@@ -166,6 +167,7 @@ impl Component for DatacenterManagerApp {
             remote_list: Vec::new().into(),
             remote_list_error: None,
             remote_list_timeout: None,
+            search_provider: SearchProvider::new(),
         };
 
         this.on_login(ctx, false);
@@ -258,10 +260,15 @@ impl Component for DatacenterManagerApp {
             .with_optional_child(subscription_alert);
 
         let context = self.remote_list.clone();
-
-        DesktopApp::new(
-            html! {<ContextProvider<RemoteList> {context}>{body}</ContextProvider<RemoteList>>},
-        )
+        let search_context = self.search_provider.clone();
+
+        DesktopApp::new(html! {
+            <ContextProvider<SearchProvider> context={search_context}>
+                <ContextProvider<RemoteList> {context}>
+                    {body}
+                </ContextProvider<RemoteList>>
+            </ContextProvider<SearchProvider>>
+        })
         .into()
     }
 }
diff --git a/ui/src/search_provider.rs b/ui/src/search_provider.rs
new file mode 100644
index 0000000..441cc2b
--- /dev/null
+++ b/ui/src/search_provider.rs
@@ -0,0 +1,35 @@
+use yew::Callback;
+
+use pwt::state::{SharedState, SharedStateObserver};
+
+use pdm_search::Search;
+
+#[derive(Clone, PartialEq)]
+pub struct SearchProvider {
+    state: SharedState<String>,
+}
+
+impl SearchProvider {
+    pub fn new() -> Self {
+        Self {
+            state: SharedState::new("".into()),
+        }
+    }
+
+    pub fn add_listener(
+        &self,
+        cb: impl Into<Callback<SharedState<String>>>,
+    ) -> SharedStateObserver<String> {
+        self.state.add_listener(cb)
+    }
+
+    pub fn search(&self, search_term: Search) {
+        **self.state.write() = search_term.to_string();
+    }
+}
+
+pub fn get_search_provider<T: yew::Component>(ctx: &yew::Context<T>) -> Option<SearchProvider> {
+    let (provider, _context_listener) = ctx.link().context(Callback::from(|_| {}))?;
+
+    Some(provider)
+}
diff --git a/ui/src/widget/search_box.rs b/ui/src/widget/search_box.rs
index 0aeedb7..6b2478f 100644
--- a/ui/src/widget/search_box.rs
+++ b/ui/src/widget/search_box.rs
@@ -9,13 +9,15 @@ use yew::{
 };
 
 use pwt::{
-    dom::focus::FocusTracker,
-    dom::IntoHtmlElement,
+    dom::{focus::FocusTracker, IntoHtmlElement},
     prelude::*,
     props::CssLength,
+    state::{SharedState, SharedStateObserver},
     widget::{form::Field, Container},
 };
 
+use crate::search_provider::get_search_provider;
+
 use super::ResourceTree;
 
 #[derive(Properties, PartialEq)]
@@ -35,7 +37,7 @@ impl From<SearchBox> for VNode {
 }
 
 pub enum Msg {
-    ChangeTerm(String),
+    ChangeTerm(String, bool), // force value
     FocusChange(bool),
     ToggleFocus,
 }
@@ -48,6 +50,8 @@ pub struct PdmSearchBox {
     focus: bool,
     global_shortcut_listener: Closure<dyn Fn(KeyboardEvent)>,
     toggle_focus: bool,
+    _observer: Option<SharedStateObserver<String>>,
+    force_value: bool,
 }
 
 impl Component for PdmSearchBox {
@@ -57,6 +61,14 @@ impl Component for PdmSearchBox {
 
     fn create(ctx: &yew::Context<Self>) -> Self {
         let link = ctx.link().clone();
+        let _observer = get_search_provider(ctx).map(|search| {
+            search.add_listener(ctx.link().batch_callback(|value: SharedState<String>| {
+                vec![
+                    Msg::ToggleFocus,
+                    Msg::ChangeTerm(value.read().clone(), true),
+                ]
+            }))
+        });
         Self {
             search_field_ref: Default::default(),
             search_box_ref: Default::default(),
@@ -72,13 +84,16 @@ impl Component for PdmSearchBox {
                     _ => {}
                 }
             })),
+            _observer,
+            force_value: false,
         }
     }
 
     fn update(&mut self, _ctx: &Context<Self>, msg: Self::Message) -> bool {
         match msg {
-            Msg::ChangeTerm(term) => {
+            Msg::ChangeTerm(term, force_value) => {
                 self.search_term = term;
+                self.force_value = force_value;
                 true
             }
             Msg::FocusChange(focus) => {
@@ -122,7 +137,8 @@ impl Component for PdmSearchBox {
                 Field::new()
                     .placeholder(tr!("Search (Ctrl+Space / Ctrl+Shift+F)"))
                     .node_ref(self.search_field_ref.clone())
-                    .on_input(ctx.link().callback(Msg::ChangeTerm)),
+                    .value(self.force_value.then_some(self.search_term.clone()))
+                    .on_input(ctx.link().callback(|term| Msg::ChangeTerm(term, false))),
             )
             .with_child(search_result)
             .into()
-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 05/10] ui: dashboard: remotes panel: open search on click

[Dominik Csapak]

using the SearchProvider, insert a search term into the search box to
find and show either all remotes (when healthy) or the failed ones.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v4
 ui/src/dashboard/remote_panel.rs | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/ui/src/dashboard/remote_panel.rs b/ui/src/dashboard/remote_panel.rs
index 7471fb6..7f5b61d 100644
--- a/ui/src/dashboard/remote_panel.rs
+++ b/ui/src/dashboard/remote_panel.rs
@@ -1,5 +1,6 @@
 use std::rc::Rc;
 
+use pdm_search::{Search, SearchTerm};
 use proxmox_yew_comp::Status;
 use pwt::{
     css,
@@ -14,6 +15,8 @@ use yew::{
 
 use pdm_api_types::resource::ResourcesStatus;
 
+use crate::search_provider::get_search_provider;
+
 #[derive(Properties, PartialEq)]
 /// A panel for showing the overall remotes status
 pub struct RemotePanel {
@@ -38,13 +41,20 @@ impl From<RemotePanel> for VNode {
 struct PdmRemotePanel {}
 
 impl Component for PdmRemotePanel {
-    type Message = &'static str;
+    type Message = Search;
     type Properties = RemotePanel;
 
     fn create(_ctx: &yew::Context<Self>) -> Self {
         Self {}
     }
 
+    fn update(&mut self, ctx: &Context<Self>, msg: Self::Message) -> bool {
+        if let Some(search) = get_search_provider(ctx) {
+            search.search(msg);
+        }
+        false
+    }
+
     fn view(&self, ctx: &yew::Context<Self>) -> yew::Html {
         let props = ctx.props();
         if props.status.is_none() {
@@ -77,6 +87,13 @@ impl Component for PdmRemotePanel {
         };
         Column::new()
             .tabindex(if failure { 0 } else { -1 })
+            .onclick(ctx.link().callback(move |_| create_search_term(failure)))
+            .onkeydown(ctx.link().batch_callback(move |event: KeyboardEvent| {
+                match event.key().as_str() {
+                    "Enter" | " " => Some(create_search_term(failure)),
+                    _ => None,
+                }
+            }))
             .padding(4)
             .class(css::FlexFit)
             .class(css::AlignItems::Center)
@@ -88,3 +105,14 @@ impl Component for PdmRemotePanel {
             .into()
     }
 }
+
+fn create_search_term(failure: bool) -> Search {
+    if failure {
+        Search::with_terms(vec![
+            SearchTerm::new("remote").category(Some("type")),
+            SearchTerm::new("offline").category(Some("status")),
+        ])
+    } else {
+        Search::with_terms(vec![SearchTerm::new("remote").category(Some("type"))])
+    }
+}
-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 06/10] ui: dashboard: guest panel: search for guest states when clicking on them

[Dominik Csapak]

by using the SearchProvider, we create a fitting search term for the
clicked/selected type of guest.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v4
 ui/src/dashboard/guest_panel.rs | 82 +++++++++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 4 deletions(-)

diff --git a/ui/src/dashboard/guest_panel.rs b/ui/src/dashboard/guest_panel.rs
index 8d130c3..1e6a9c7 100644
--- a/ui/src/dashboard/guest_panel.rs
+++ b/ui/src/dashboard/guest_panel.rs
@@ -1,19 +1,26 @@
 use std::rc::Rc;
 
 use pdm_api_types::resource::GuestStatusCount;
+use pdm_search::{Search, SearchTerm};
 use proxmox_yew_comp::GuestState;
 use pwt::{
     prelude::*,
     props::ExtractPrimaryKey,
     state::Store,
     widget::{
-        data_table::{DataTable, DataTableColumn, DataTableHeader},
+        data_table::{
+            DataTable, DataTableColumn, DataTableHeader, DataTableKeyboardEvent,
+            DataTableMouseEvent, DataTableRowRenderArgs,
+        },
         Fa,
     },
 };
-use yew::virtual_dom::{VComp, VNode};
+use yew::{
+    virtual_dom::{Key, VComp, VNode},
+    Properties,
+};
 
-use crate::pve::GuestType;
+use crate::{pve::GuestType, search_provider::get_search_provider};
 
 use super::loading_column;
 
@@ -101,13 +108,20 @@ fn columns(guest_type: GuestType) -> Rc<Vec<DataTableHeader<StatusRow>>> {
 pub struct PdmGuestPanel {}
 
 impl yew::Component for PdmGuestPanel {
-    type Message = String;
+    type Message = Search;
     type Properties = GuestPanel;
 
     fn create(_ctx: &yew::Context<Self>) -> Self {
         Self {}
     }
 
+    fn update(&mut self, ctx: &Context<Self>, msg: Self::Message) -> bool {
+        if let Some(provider) = get_search_provider(ctx) {
+            provider.search(msg);
+        }
+        false
+    }
+
     fn view(&self, ctx: &yew::Context<Self>) -> yew::Html {
         let props = ctx.props();
         if props.status.is_none() {
@@ -137,7 +151,67 @@ impl yew::Component for PdmGuestPanel {
             .striped(false)
             .borderless(true)
             .bordered(false)
+            .row_render_callback(|renderer: &mut DataTableRowRenderArgs<StatusRow>| {
+                renderer.class.push("pwt-pointer");
+            })
+            .on_row_keydown({
+                let store = store.clone();
+                let link = ctx.link().clone();
+                move |event: &mut DataTableKeyboardEvent| match event.key().as_str() {
+                    " " | "Enter" => search_callback(&link, &store, guest_type, &event.record_key),
+                    _ => {}
+                }
+            })
+            .on_row_click({
+                let store = store.clone();
+                let link = ctx.link().clone();
+                move |event: &mut DataTableMouseEvent| {
+                    search_callback(&link, &store, guest_type, &event.record_key);
+                }
+            })
             .show_header(false)
             .into()
     }
 }
+
+fn search_callback(
+    link: &html::Scope<PdmGuestPanel>,
+    store: &Store<StatusRow>,
+    guest_type: GuestType,
+    key: &Key,
+) {
+    if let Some((_, record)) = store.filtered_data().find(|(_, rec)| rec.key() == *key) {
+        let (status, template) = match &*record.record() {
+            StatusRow::State(guest_state, _) => match guest_state {
+                GuestState::Running => (Some("running"), Some(false)),
+                GuestState::Paused => (Some("paused"), Some(false)),
+                GuestState::Stopped => (Some("stopped"), Some(false)),
+                GuestState::Template => (None, Some(true)),
+                GuestState::Unknown => (Some("unknown"), None),
+            },
+            StatusRow::All(_) => (None, None),
+        };
+
+        link.send_message(create_guest_search_term(guest_type, status, template));
+    }
+}
+
+fn create_guest_search_term(
+    guest_type: GuestType,
+    status: Option<&'static str>,
+    template: Option<bool>,
+) -> Search {
+    if status.is_none() && template.is_none() {
+        return Search::with_terms(vec![SearchTerm::new(guest_type).category(Some("type"))]);
+    }
+
+    let mut terms = vec![SearchTerm::new(guest_type).category(Some("type"))];
+
+    if let Some(template) = template {
+        terms.push(SearchTerm::new(template).category(Some("template")));
+    }
+    if let Some(status) = status {
+        terms.push(SearchTerm::new(status).category(Some("status")));
+    }
+    Search::with_terms(terms)
+}
-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 07/10] ui: dashboard: search for nodes when clicking on the nodes panel

[Dominik Csapak]

similar to what we do for the remote panel

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v4
 ui/src/dashboard/mod.rs | 46 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/ui/src/dashboard/mod.rs b/ui/src/dashboard/mod.rs
index 84efb1b..1ac683b 100644
--- a/ui/src/dashboard/mod.rs
+++ b/ui/src/dashboard/mod.rs
@@ -26,9 +26,10 @@ use pwt::{
 
 use pdm_api_types::resource::{GuestStatusCount, NodeStatusCount, ResourcesStatus};
 use pdm_client::types::TopEntity;
+use pdm_search::{Search, SearchTerm};
 use proxmox_client::ApiResponseData;
 
-use crate::{pve::GuestType, remotes::AddWizard, RemoteList};
+use crate::{pve::GuestType, remotes::AddWizard, search_provider::get_search_provider, RemoteList};
 
 mod top_entities;
 pub use top_entities::TopEntities;
@@ -87,6 +88,7 @@ pub enum Msg {
     Reload,
     UpdateConfig(DashboardConfig),
     ConfigWindow(bool),
+    Search(Search),
 }
 
 pub struct PdmDashboard {
@@ -114,13 +116,25 @@ impl PdmDashboard {
             .into()
     }
 
-    fn create_node_panel(&self, icon: &str, title: String, status: &NodeStatusCount) -> Panel {
+    fn create_node_panel(
+        &self,
+        ctx: &yew::Context<Self>,
+        icon: &str,
+        title: String,
+        status: &NodeStatusCount,
+    ) -> Panel {
         let (status_icon, text): (Fa, String) = match status {
             NodeStatusCount {
-                online, offline, ..
+                online,
+                offline,
+                unknown,
             } if *offline > 0 => (
                 Status::Error.into(),
-                tr!("{0} of {1} nodes are offline", offline, online),
+                tr!(
+                    "{0} of {1} nodes are offline",
+                    offline,
+                    online + offline + unknown,
+                ),
             ),
             NodeStatusCount { unknown, .. } if *unknown > 0 => (
                 Status::Warning.into(),
@@ -143,6 +157,23 @@ impl PdmDashboard {
             .with_child(
                 Column::new()
                     .padding(4)
+                    .class("pwt-pointer")
+                    .onclick(ctx.link().callback(move |_| {
+                        Msg::Search(Search::with_terms(vec![
+                            SearchTerm::new("node").category(Some("type"))
+                        ]))
+                    }))
+                    .onkeydown(ctx.link().batch_callback(move |event: KeyboardEvent| {
+                        match event.key().as_str() {
+                            "Enter" | " " => {
+                                Some(Msg::Search(Search::with_terms(vec![SearchTerm::new(
+                                    "node",
+                                )
+                                .category(Some("type"))])))
+                            }
+                            _ => None,
+                        }
+                    }))
                     .class(FlexFit)
                     .class(AlignItems::Center)
                     .class(JustifyContent::Center)
@@ -290,6 +321,12 @@ impl Component for PdmDashboard {
                 self.show_config_window = false;
                 true
             }
+            Msg::Search(search_term) => {
+                if let Some(provider) = get_search_provider(ctx) {
+                    provider.search(search_term.into());
+                }
+                false
+            }
         }
     }
 
@@ -334,6 +371,7 @@ impl Component for PdmDashboard {
                             )),
                     )
                     .with_child(self.create_node_panel(
+                        ctx,
                         "building",
                         tr!("Virtual Environment Nodes"),
                         &self.status.pve_nodes,
-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 08/10] ui: search box: add clear trigger

[Dominik Csapak]

that removes the search term on click. We also need to set the
flex-basis for now, as otherwise the field 'wobbles' when showing or
hiding the trigger.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v4
 ui/src/widget/search_box.rs | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/ui/src/widget/search_box.rs b/ui/src/widget/search_box.rs
index 6b2478f..52bb156 100644
--- a/ui/src/widget/search_box.rs
+++ b/ui/src/widget/search_box.rs
@@ -13,7 +13,7 @@ use pwt::{
     prelude::*,
     props::CssLength,
     state::{SharedState, SharedStateObserver},
-    widget::{form::Field, Container},
+    widget::{form::Field, Container, Trigger},
 };
 
 use crate::search_provider::get_search_provider;
@@ -128,16 +128,28 @@ impl Component for PdmSearchBox {
             .height(400)
             .class("pwt-shadow2");
 
+        let clear_trigger_icon = if self.search_term.is_empty() {
+            ""
+        } else {
+            "fa fa-times"
+        };
+
         Container::new()
             .onfocusin(self.focus_tracker.get_focus_callback(true))
             .onfocusout(self.focus_tracker.get_focus_callback(false))
             .flex(2.0)
+            .style("flex-basis", "230px") // to avoid changing size with trigger
             .min_width(230) // placeholder text
             .with_child(
                 Field::new()
                     .placeholder(tr!("Search (Ctrl+Space / Ctrl+Shift+F)"))
                     .node_ref(self.search_field_ref.clone())
                     .value(self.force_value.then_some(self.search_term.clone()))
+                    .with_trigger(
+                        Trigger::new(clear_trigger_icon)
+                            .onclick(ctx.link().callback(|_| Msg::ChangeTerm("".into(), true))),
+                        true,
+                    )
                     .on_input(ctx.link().callback(|term| Msg::ChangeTerm(term, false))),
             )
             .with_child(search_result)
-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 09/10] ui: dashboard: guest panel: use `List` instead of `DataTable`

[Dominik Csapak]

since this is a relatively simple list, using the datatable is a big
overhead. Converting it to a simple list, while keeping all the
functionality we had before, so we can remove quite a bit of code and
not run into issues with different padding scaling (e.g. with the
relaxed spacing)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v4
 ui/src/dashboard/guest_panel.rs | 208 +++++++++++++-------------------
 ui/src/pve/mod.rs               |  10 ++
 2 files changed, 94 insertions(+), 124 deletions(-)

diff --git a/ui/src/dashboard/guest_panel.rs b/ui/src/dashboard/guest_panel.rs
index 1e6a9c7..7bc1bfc 100644
--- a/ui/src/dashboard/guest_panel.rs
+++ b/ui/src/dashboard/guest_panel.rs
@@ -4,19 +4,12 @@ use pdm_api_types::resource::GuestStatusCount;
 use pdm_search::{Search, SearchTerm};
 use proxmox_yew_comp::GuestState;
 use pwt::{
+    css::{self, TextAlign},
     prelude::*,
-    props::ExtractPrimaryKey,
-    state::Store,
-    widget::{
-        data_table::{
-            DataTable, DataTableColumn, DataTableHeader, DataTableKeyboardEvent,
-            DataTableMouseEvent, DataTableRowRenderArgs,
-        },
-        Fa,
-    },
+    widget::{Container, Fa, List, ListTile},
 };
 use yew::{
-    virtual_dom::{Key, VComp, VNode},
+    virtual_dom::{VComp, VNode},
     Properties,
 };
 
@@ -49,62 +42,6 @@ pub enum StatusRow {
     All(u64),
 }
 
-impl ExtractPrimaryKey for StatusRow {
-    fn extract_key(&self) -> yew::virtual_dom::Key {
-        yew::virtual_dom::Key::from(match self {
-            StatusRow::State(state, _) => match state {
-                GuestState::Running => "running",
-                GuestState::Paused => "paused",
-                GuestState::Stopped => "stopped",
-                GuestState::Template => "template",
-                GuestState::Unknown => "unknown",
-            },
-            StatusRow::All(_) => "all",
-        })
-    }
-}
-
-fn columns(guest_type: GuestType) -> Rc<Vec<DataTableHeader<StatusRow>>> {
-    Rc::new(vec![
-        DataTableColumn::new("icon")
-            .width("3em")
-            .render(move |item: &StatusRow| {
-                match item {
-                    StatusRow::State(state, _) => (*state).into(),
-                    StatusRow::All(_) => match guest_type {
-                        GuestType::Qemu => Fa::new("desktop"),
-                        GuestType::Lxc => Fa::new("cubes"),
-                    },
-                }
-                .fixed_width()
-                .into()
-            })
-            .into(),
-        DataTableColumn::new("text")
-            .flex(5)
-            .render(|item: &StatusRow| {
-                match item {
-                    StatusRow::State(GuestState::Running, _) => tr!("running"),
-                    StatusRow::State(GuestState::Stopped, _) => tr!("stopped"),
-                    StatusRow::State(GuestState::Paused, _) => tr!("paused"),
-                    StatusRow::State(GuestState::Template, _) => tr!("Template"),
-                    StatusRow::State(GuestState::Unknown, _) => tr!("Unknown"),
-                    StatusRow::All(_) => tr!("All"),
-                }
-                .into()
-            })
-            .into(),
-        DataTableColumn::new("count")
-            .flex(1)
-            .justify("right")
-            .render(|item: &StatusRow| match item {
-                StatusRow::State(_, count) => count.into(),
-                StatusRow::All(count) => count.into(),
-            })
-            .into(),
-    ])
-}
-
 pub struct PdmGuestPanel {}
 
 impl yew::Component for PdmGuestPanel {
@@ -124,76 +61,99 @@ impl yew::Component for PdmGuestPanel {
 
     fn view(&self, ctx: &yew::Context<Self>) -> yew::Html {
         let props = ctx.props();
-        if props.status.is_none() {
-            return loading_column().into();
-        }
         let guest_type = props.guest_type;
-        let status = ctx.props().status.clone().unwrap();
+        let status = match &props.status {
+            Some(status) => status,
+            None => return loading_column().into(),
+        };
 
-        let store = Store::new();
-        store.set_data(vec![
+        let data = vec![
             StatusRow::State(GuestState::Running, status.running),
             StatusRow::State(GuestState::Stopped, status.stopped),
             StatusRow::State(GuestState::Template, status.template),
             StatusRow::State(GuestState::Unknown, status.unknown),
             StatusRow::All(status.running + status.stopped + status.template + status.unknown),
-        ]);
-
-        store.set_filter(|rec: &StatusRow| match rec {
-            StatusRow::State(_, count) if *count > 0 => true,
-            StatusRow::State(GuestState::Running | GuestState::Stopped, _) => true,
-            StatusRow::All(_) => true,
-            _ => false,
-        });
-
-        DataTable::new(columns(guest_type), store.clone())
-            .padding(4)
-            .striped(false)
-            .borderless(true)
-            .bordered(false)
-            .row_render_callback(|renderer: &mut DataTableRowRenderArgs<StatusRow>| {
-                renderer.class.push("pwt-pointer");
-            })
-            .on_row_keydown({
-                let store = store.clone();
-                let link = ctx.link().clone();
-                move |event: &mut DataTableKeyboardEvent| match event.key().as_str() {
-                    " " | "Enter" => search_callback(&link, &store, guest_type, &event.record_key),
-                    _ => {}
-                }
-            })
-            .on_row_click({
-                let store = store.clone();
-                let link = ctx.link().clone();
-                move |event: &mut DataTableMouseEvent| {
-                    search_callback(&link, &store, guest_type, &event.record_key);
-                }
-            })
-            .show_header(false)
-            .into()
+        ];
+
+        let tiles: Vec<_> = data
+            .into_iter()
+            .filter_map(|row| create_list_tile(ctx.link(), guest_type, row))
+            .collect();
+
+        let list = List::new(tiles.len() as u64, move |idx: u64| {
+            tiles[idx as usize].clone()
+        })
+        .padding(4)
+        .class(css::Flex::Fill)
+        .grid_template_columns("auto auto 1fr");
+
+        list.into()
     }
 }
 
-fn search_callback(
+fn create_list_tile(
     link: &html::Scope<PdmGuestPanel>,
-    store: &Store<StatusRow>,
     guest_type: GuestType,
-    key: &Key,
-) {
-    if let Some((_, record)) = store.filtered_data().find(|(_, rec)| rec.key() == *key) {
-        let (status, template) = match &*record.record() {
-            StatusRow::State(guest_state, _) => match guest_state {
-                GuestState::Running => (Some("running"), Some(false)),
-                GuestState::Paused => (Some("paused"), Some(false)),
-                GuestState::Stopped => (Some("stopped"), Some(false)),
-                GuestState::Template => (None, Some(true)),
-                GuestState::Unknown => (Some("unknown"), None),
-            },
-            StatusRow::All(_) => (None, None),
-        };
-
-        link.send_message(create_guest_search_term(guest_type, status, template));
-    }
+    status_row: StatusRow,
+) -> Option<ListTile> {
+    let (icon, text, count, status, template) = match status_row {
+        StatusRow::State(guest_state, count) => match guest_state {
+            GuestState::Template | GuestState::Unknown if count == 0 => return None,
+            GuestState::Paused => return None,
+            GuestState::Running => (
+                Fa::from(guest_state),
+                tr!("running"),
+                count,
+                Some("running"),
+                Some(false),
+            ),
+            GuestState::Stopped => (
+                Fa::from(guest_state),
+                tr!("stopped"),
+                count,
+                Some("stopped"),
+                Some(false),
+            ),
+            GuestState::Template => (
+                Fa::from(guest_state),
+                tr!("Template"),
+                count,
+                None,
+                Some(true),
+            ),
+            GuestState::Unknown => (
+                Fa::from(guest_state),
+                tr!("Unknown"),
+                count,
+                Some("unknown"),
+                None,
+            ),
+        },
+        StatusRow::All(count) => (Fa::from(guest_type), tr!("All"), count, None, None),
+    };
+
+    Some(
+        ListTile::new()
+            .tabindex(0)
+            .interactive(true)
+            .with_child(icon)
+            .with_child(Container::new().padding_x(2).with_child(text))
+            .with_child(
+                Container::new()
+                    .class(TextAlign::Right)
+                    // FIXME: replace with `column_gap` to `List` when implemented
+                    .padding_end(2)
+                    .with_child(count),
+            )
+            // FIXME: repalce with on_activate for `ListTile` when implemented
+            .onclick(link.callback(move |_| create_guest_search_term(guest_type, status, template)))
+            .onkeydown(link.batch_callback(
+                move |event: KeyboardEvent| match event.key().as_str() {
+                    "Enter" | " " => Some(create_guest_search_term(guest_type, status, template)),
+                    _ => None,
+                },
+            )),
+    )
 }
 
 fn create_guest_search_term(
diff --git a/ui/src/pve/mod.rs b/ui/src/pve/mod.rs
index a3cf573..47b2192 100644
--- a/ui/src/pve/mod.rs
+++ b/ui/src/pve/mod.rs
@@ -81,6 +81,16 @@ impl Display for GuestType {
     }
 }
 
+impl From<GuestType> for Fa {
+    fn from(val: GuestType) -> Self {
+        let icon = match val {
+            GuestType::Qemu => "desktop",
+            GuestType::Lxc => "cubes",
+        };
+        Fa::new(icon)
+    }
+}
+
 #[derive(PartialEq, Clone, Copy)]
 pub struct GuestInfo {
     pub guest_type: GuestType,
-- 
2.47.2



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

[pdm-devel] [PATCH datacenter-manager v4 10/10] ui: dashboard: guest panel: add search icon for better discoverability

[Dominik Csapak]

of the search function. With this icon, it should be clear that one
can click/press these items.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v4
 ui/src/dashboard/guest_panel.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ui/src/dashboard/guest_panel.rs b/ui/src/dashboard/guest_panel.rs
index 7bc1bfc..e09ea53 100644
--- a/ui/src/dashboard/guest_panel.rs
+++ b/ui/src/dashboard/guest_panel.rs
@@ -85,7 +85,7 @@ impl yew::Component for PdmGuestPanel {
         })
         .padding(4)
         .class(css::Flex::Fill)
-        .grid_template_columns("auto auto 1fr");
+        .grid_template_columns("auto auto 1fr auto");
 
         list.into()
     }
@@ -145,6 +145,7 @@ fn create_list_tile(
                     .padding_end(2)
                     .with_child(count),
             )
+            .with_child(Fa::new("search"))
             // FIXME: repalce with on_activate for `ListTile` when implemented
             .onclick(link.callback(move |_| create_guest_search_term(guest_type, status, template)))
             .onkeydown(link.batch_callback(
-- 
2.47.2



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

Re: [pdm-devel] [PATCH datacenter-manager v4 03/10] server: api: resources: add more complex filter syntax

[Shannon Sterz]

> +fn remote_matches_search_term(remote_name: &str, online: Option<bool>, term: &SearchTerm) -> bool {
> +    match term.category.as_deref() {
> +        Some("remote" | "name" | "id") => remote_name.contains(&term.value),
> +        Some("type") => "remote".starts_with(&term.value),
> +        Some("status") => match online {
> +            None => true,
> +            Some(true) => "online".starts_with(&term.value),
> +            Some(false) => "offline".starts_with(&term.value),
> +        },
> +        None => remote_name.contains(&term.value) || "remote".contains(&term.value),

correct me if im wrong, but wouldn't this mean that any search without a
category would return all remotes given that the term's value is a
substring of "remote"? so if i type "e" i'd get all remotes?

since this has already been a concern for searches with a `type`
category [1] in v3, maybe this here should also be a `starts_with`
match? seems more consistent to me anyway.

[1]: https://lore.proxmox.com/pdm-devel/f992b565-f7e3-4339-80ce-aabd49cef773@proxmox.com/T/#m74b8924dda5ddb32aeee34a6685fa4a769380ef9

-->8 snip 8<--


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

Re: [pdm-devel] [PATCH datacenter-manager v4 03/10] server: api: resources: add more complex filter syntax

[Dominik Csapak]

On 8/28/25 3:53 PM, Shannon Sterz wrote:
>> +fn remote_matches_search_term(remote_name: &str, online: Option<bool>, term: &SearchTerm) -> bool {
>> +    match term.category.as_deref() {
>> +        Some("remote" | "name" | "id") => remote_name.contains(&term.value),
>> +        Some("type") => "remote".starts_with(&term.value),
>> +        Some("status") => match online {
>> +            None => true,
>> +            Some(true) => "online".starts_with(&term.value),
>> +            Some(false) => "offline".starts_with(&term.value),
>> +        },
>> +        None => remote_name.contains(&term.value) || "remote".contains(&term.value),
> 
> correct me if im wrong, but wouldn't this mean that any search without a
> category would return all remotes given that the term's value is a
> substring of "remote"? so if i type "e" i'd get all remotes?
> 
> since this has already been a concern for searches with a `type`
> category [1] in v3, maybe this here should also be a `starts_with`
> match? seems more consistent to me anyway.

ah yes, i overlooked that check

imho it should be

None => remote_name.contains(&term.value) || 
"remote".starts_with(&term_value),

Can send as a follow up or if something else comes i'll include that in a v5

> 
> [1]: https://lore.proxmox.com/pdm-devel/f992b565-f7e3-4339-80ce-aabd49cef773@proxmox.com/T/#m74b8924dda5ddb32aeee34a6685fa4a769380ef9
> 
> -->8 snip 8<--
> 
> 
> _______________________________________________
> pdm-devel mailing list
> pdm-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
> 
> 



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

Re: [pdm-devel] [PATCH datacenter-manager v4 01/10] pdm-api-types: resources: add helper methods for fields

[Wolfgang Bumiller]

On Thu, Aug 28, 2025 at 03:16:00PM +0200, Dominik Csapak wrote:
> namely 'resource_type' and 'status'. All resources have those fields
> in one way or another, so adding a helper that one does not have to
> use `match` on every call site makes code there shorter.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> no changes in v4
>  lib/pdm-api-types/src/resource.rs | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/lib/pdm-api-types/src/resource.rs b/lib/pdm-api-types/src/resource.rs
> index 6227855..0dfadd5 100644
> --- a/lib/pdm-api-types/src/resource.rs
> +++ b/lib/pdm-api-types/src/resource.rs
> @@ -62,6 +62,33 @@ impl Resource {
>              Resource::PbsDatastore(r) => r.name.as_str(),
>          }
>      }
> +
> +    pub fn resource_type(&self) -> &str {

^ This may be nicer as simple `enum` with a `Display`/`FromStr`
implementation.

> +        match self {
> +            Resource::PveStorage(_) => "storage",
> +            Resource::PveQemu(_) => "qemu",
> +            Resource::PveLxc(_) => "lxc",
> +            Resource::PveNode(_) | Resource::PbsNode(_) => "node",
> +            Resource::PbsDatastore(_) => "datastore",
> +        }
> +    }
> +
> +    pub fn status(&self) -> &str {
> +        match self {
> +            Resource::PveStorage(r) => r.status.as_str(),
> +            Resource::PveQemu(r) => r.status.as_str(),
> +            Resource::PveLxc(r) => r.status.as_str(),
> +            Resource::PveNode(r) => r.status.as_str(),
> +            Resource::PbsNode(r) => {
> +                if r.uptime > 0 {
> +                    "online"
> +                } else {
> +                    "offline"
> +                }
> +            }
> +            Resource::PbsDatastore(_) => "online",
> +        }
> +    }
>  }
>  
>  #[api(
> -- 
> 2.47.2


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

Re: [pdm-devel] [PATCH datacenter-manager v4 02/10] lib: add pdm-search crate

[Wolfgang Bumiller]

On Thu, Aug 28, 2025 at 03:16:01PM +0200, 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)

^ Would be good to clarify that (and fix if not) if *no* optional one
matches *but* there *are* matching required ones, it's also a positive
match...

> 
> 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>
> ---
> changes from v3:
> * add missing doc comments
> * more tests
> 
>  Cargo.toml                |   2 +
>  lib/pdm-search/Cargo.toml |  12 ++
>  lib/pdm-search/src/lib.rs | 339 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 353 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 f07733d..ae2816a 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..4854166
> --- /dev/null
> +++ b/lib/pdm-search/src/lib.rs
> @@ -0,0 +1,339 @@
> +//! 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;

↑ you never use any of its methods, for the single trait implementation
it's fine to just `impl std::str::FromStr` below

OTOH you do implement `Display` twice without importing it (which is
fine, don't but importing just `std::fmt` will shorten it all...)


> +
> +use anyhow::bail;
> +
> +#[derive(Clone)]
> +pub struct Search {
> +    required_terms: Vec<SearchTerm>,
> +    optional_terms: Vec<SearchTerm>,

↑ In theory - is partitioning them very useful, or would it be a better
UX if we kept them in a single vec such that `.parse().to_string()`
doesn't reorder them?

> +}
> +
> +impl Default for Search {

A derive should work here

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

Maybe add a note here why we do this, as this is quite painful to read.
We throw away errors because we expect users to produce them? ;-)

> +            .collect()
> +    }
> +}
> +
> +impl Search {
> +    /// Create a new empty [`Search`]
> +    pub fn new() -> Self {
> +        Self::with_terms(Vec::new())
> +    }
> +
> +    /// Returns true if no [`SearchTerm`] exist
> +    pub fn is_empty(&self) -> bool {
> +        self.required_terms.is_empty() && self.optional_terms.is_empty()
> +    }
> +
> +    /// Create a new [`Search`] with the given [`SearchTerm`]s
> +    pub fn with_terms(terms: Vec<SearchTerm>) -> Self {

(
Since we already have `FromIterator`, this could be

    pub fn with_terms<I>(terms: I) -> Self
    where
        I: IntoIterator<Item = SearchTerm>;
)

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

Why does this use `.contains()` but the loop over optional ones uses
`==`?

> +                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}")?;
> +        }

personal opinion: enumerate() to only handle the first element
differently seems a bit much, if you consider that it keeps a counter,
you could keep a "separator" instead which you just update once:

    let mut sep = "";
    for term in &self.required_terms {
        write!(f, "{sep}{term}")?;
        sep = " ";
    }

> +
> +        if !self.required_terms.is_empty() && !self.optional_terms.is_empty() {
> +            write!(f, " ")?;
> +        }

↑ This could then be skipped entirely and we can reuse `sep` inside the
lower loop the same way - if no previous text was there, the first
element will still be using an empty string for `sep` before updating it
to be `" "` for the remaining elements...

Then the entire implementation is just:
    let mut sep = "";
    for term in &self.required_terms {
        write!(f, "{sep}{term}")?;
        sep = " ";
    }
    for term in &self.optional_terms {
        write!(f, "{sep}{term}")?;
        sep = " ";
    }

of if you're feeling fancy:

    let mut sep = "";
    for term in self.required_terms.iter().chain(self.optional_terms.iter()) {
        write!(f, "{sep}{term}")?;
        sep = " ";
    }

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

↑ use `Into<String>`, `ToString` will cause an unnecessary clone when
passing an already owned `String` as it is implemented via `Display`.

> +        Self {
> +            value: term.to_string(),
> +            optional: false,
> +            category: None,
> +        }
> +    }
> +
> +    /// Builder style method to set the category
> +    pub fn category<S: ToString>(mut self, category: Option<S>) -> Self {
> +        self.category = category.map(|s| s.to_string());
> +        self
> +    }
> +
> +    /// Builder style method to mark this [`SearchTerm`] as optional
> +    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> {

↑ use: (mut term: &str)

> +        let mut optional = true;
> +        let mut term: String = s.into();

↑ skip the premature allocation ;-)

> +        if term.starts_with("+") {
> +            optional = false;
> +            term.remove(0);
> +        }

↑
    if let Some(rest) = term.strip_prefix("+") {
        term = rest;
        optional = false;
    }

> +
> +        let (term, category) = if let Some(idx) = term.find(":") {
> +            let mut real_term = term.split_off(idx);
> +            real_term.remove(0); // remove ':'

↑ 2 expensive avoidable ops, see below

> +            (real_term, Some(term))
> +        } else {
> +            (term, None)
> +        };

↑
    let category;
    (term, category) = match term.split_once(':') {
        Some((category, term)) => (term, Some(category)),
        None => (term, None),
    }

> +
> +        if term.is_empty() {
> +            bail!("term cannot be empty");
> +        }
> +
> +        if category == Some("".into()) {

↑ Can avoid this (also, `String::new()` would be cheaper) via:

    if category.is_some_and(str::is_empty) {


> +            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),
> +        ];
> +        for (input, expected) in cases {
> +            assert!(
> +                search.matches(|term| {
> +                    match term.value.as_str() {
> +                        "required1" => input.0,
> +                        "required2" => input.1,
> +                        "optional1" => input.2,
> +                        "optional2" => input.3,
> +                        _ => unreachable!(),
> +                    }
> +                }) == expected
> +            )
> +        }
> +    }
> +
> +    #[test]
> +    fn category_value_required() {
> +        let search: Search = Search::from_iter(vec![SearchTerm::new("foo")]);
> +        assert!(!search.category_value_required("bar", "baz"));
> +
> +        let search: Search = Search::from_iter(vec![SearchTerm::new("foo").optional(true)]);
> +        assert!(!search.category_value_required("bar", "baz"));
> +
> +        let search: Search = Search::from_iter(vec![SearchTerm::new("foo").category(Some("bar"))]);
> +        assert!(!search.category_value_required("bar", "baz"));
> +
> +        let search: Search = Search::from_iter(vec![SearchTerm::new("baz").category(Some("bar"))]);
> +        assert!(search.category_value_required("bar", "baz"));
> +
> +        let search: Search = Search::from_iter(vec![SearchTerm::new("foo")
> +            .optional(true)
> +            .category(Some("bar"))]);
> +        assert!(!search.category_value_required("bar", "baz"));
> +
> +        let search: Search = Search::from_iter(vec![SearchTerm::new("baz")
> +            .optional(true)
> +            .category(Some("bar"))]);
> +        assert!(search.category_value_required("bar", "baz"));
> +
> +        let search: Search = Search::from_iter(vec![
> +            SearchTerm::new("baz").optional(true).category(Some("bar")),
> +            SearchTerm::new("foo").optional(true),
> +        ]);
> +        assert!(!search.category_value_required("bar", "baz"));
> +    }
> +
> +    #[test]
> +    fn test_display() {
> +        let term = SearchTerm::new("foo");
> +        assert_eq!("+foo", &term.to_string());
> +
> +        let term = SearchTerm::new("foo").optional(true);
> +        assert_eq!("foo", &term.to_string());
> +
> +        let term = SearchTerm::new("foo").optional(false);
> +        assert_eq!("+foo", &term.to_string());
> +
> +        let term = SearchTerm::new("foo").category(Some("bar"));
> +        assert_eq!("+bar:foo", &term.to_string());
> +
> +        let term = SearchTerm::new("foo").optional(true).category(Some("bar"));
> +        assert_eq!("bar:foo", &term.to_string());
> +
> +        let term = SearchTerm::new("foo").optional(false).category(Some("bar"));
> +        assert_eq!("+bar:foo", &term.to_string());
> +    }
> +}
> -- 
> 2.47.2


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

Re: [pdm-devel] [PATCH datacenter-manager v4 01/10] pdm-api-types: resources: add helper methods for fields

[Dominik Csapak]

On 9/2/25 4:29 PM, Wolfgang Bumiller wrote:
> On Thu, Aug 28, 2025 at 03:16:00PM +0200, Dominik Csapak wrote:
>> namely 'resource_type' and 'status'. All resources have those fields
>> in one way or another, so adding a helper that one does not have to
>> use `match` on every call site makes code there shorter.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> no changes in v4
>>   lib/pdm-api-types/src/resource.rs | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/lib/pdm-api-types/src/resource.rs b/lib/pdm-api-types/src/resource.rs
>> index 6227855..0dfadd5 100644
>> --- a/lib/pdm-api-types/src/resource.rs
>> +++ b/lib/pdm-api-types/src/resource.rs
>> @@ -62,6 +62,33 @@ impl Resource {
>>               Resource::PbsDatastore(r) => r.name.as_str(),
>>           }
>>       }
>> +
>> +    pub fn resource_type(&self) -> &str {
> 
> ^ This may be nicer as simple `enum` with a `Display`/`FromStr`
> implementation.
> 

mhmm the only use i have of that currently would it make harder to use 
with a separate enum
(also we'd copy the type with `to_string()` once more)

e.g. i use

`resource.resource_type().starts_with(...)`

with a separate enum it would be

`resource.resource_type().to_string().starts_with(...)`

which is more to write and one string copy extra...

>> +        match self {
>> +            Resource::PveStorage(_) => "storage",
>> +            Resource::PveQemu(_) => "qemu",
>> +            Resource::PveLxc(_) => "lxc",
>> +            Resource::PveNode(_) | Resource::PbsNode(_) => "node",
>> +            Resource::PbsDatastore(_) => "datastore",
>> +        }
>> +    }
>> +
>> +    pub fn status(&self) -> &str {
>> +        match self {
>> +            Resource::PveStorage(r) => r.status.as_str(),
>> +            Resource::PveQemu(r) => r.status.as_str(),
>> +            Resource::PveLxc(r) => r.status.as_str(),
>> +            Resource::PveNode(r) => r.status.as_str(),
>> +            Resource::PbsNode(r) => {
>> +                if r.uptime > 0 {
>> +                    "online"
>> +                } else {
>> +                    "offline"
>> +                }
>> +            }
>> +            Resource::PbsDatastore(_) => "online",
>> +        }
>> +    }
>>   }
>>   
>>   #[api(
>> -- 
>> 2.47.2



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

Re: [pdm-devel] [PATCH datacenter-manager v4 01/10] pdm-api-types: resources: add helper methods for fields

[Wolfgang Bumiller]

On Wed, Sep 03, 2025 at 09:38:22AM +0200, Dominik Csapak wrote:
> 
> 
> On 9/2/25 4:29 PM, Wolfgang Bumiller wrote:
> > On Thu, Aug 28, 2025 at 03:16:00PM +0200, Dominik Csapak wrote:
> > > namely 'resource_type' and 'status'. All resources have those fields
> > > in one way or another, so adding a helper that one does not have to
> > > use `match` on every call site makes code there shorter.
> > > 
> > > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> > > ---
> > > no changes in v4
> > >   lib/pdm-api-types/src/resource.rs | 27 +++++++++++++++++++++++++++
> > >   1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/lib/pdm-api-types/src/resource.rs b/lib/pdm-api-types/src/resource.rs
> > > index 6227855..0dfadd5 100644
> > > --- a/lib/pdm-api-types/src/resource.rs
> > > +++ b/lib/pdm-api-types/src/resource.rs
> > > @@ -62,6 +62,33 @@ impl Resource {
> > >               Resource::PbsDatastore(r) => r.name.as_str(),
> > >           }
> > >       }
> > > +
> > > +    pub fn resource_type(&self) -> &str {
> > 
> > ^ This may be nicer as simple `enum` with a `Display`/`FromStr`
> > implementation.
> > 
> 
> mhmm the only use i have of that currently would it make harder to use with
> a separate enum
> (also we'd copy the type with `to_string()` once more)
> 
> e.g. i use
> 
> `resource.resource_type().starts_with(...)`
> 
> with a separate enum it would be
> 
> `resource.resource_type().to_string().starts_with(...)`
> 
> which is more to write and one string copy extra...

The enum would just have a `fn as_str(&self) -> &'static str`...

(Btw. many of these could probably be explicitly `'static`)

But it's not that important for now...

> 
> > > +        match self {
> > > +            Resource::PveStorage(_) => "storage",
> > > +            Resource::PveQemu(_) => "qemu",
> > > +            Resource::PveLxc(_) => "lxc",
> > > +            Resource::PveNode(_) | Resource::PbsNode(_) => "node",
> > > +            Resource::PbsDatastore(_) => "datastore",
> > > +        }
> > > +    }
> > > +
> > > +    pub fn status(&self) -> &str {
> > > +        match self {
> > > +            Resource::PveStorage(r) => r.status.as_str(),
> > > +            Resource::PveQemu(r) => r.status.as_str(),
> > > +            Resource::PveLxc(r) => r.status.as_str(),
> > > +            Resource::PveNode(r) => r.status.as_str(),
> > > +            Resource::PbsNode(r) => {
> > > +                if r.uptime > 0 {
> > > +                    "online"
> > > +                } else {
> > > +                    "offline"
> > > +                }
> > > +            }
> > > +            Resource::PbsDatastore(_) => "online",
> > > +        }
> > > +    }
> > >   }
> > >   #[api(
> > > -- 
> > > 2.47.2
> 


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

Re: [pdm-devel] [PATCH datacenter-manager v4 02/10] lib: add pdm-search crate

[Dominik Csapak]

On 9/2/25 5:32 PM, Wolfgang Bumiller wrote:
> On Thu, Aug 28, 2025 at 03:16:01PM +0200, 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)
> 
> ^ Would be good to clarify that (and fix if not) if *no* optional one
> matches *but* there *are* matching required ones, it's also a positive
> match...

no that's wrong?

if no optional ones have to match they don't influence the result at all?

required terms must all match, and at least one from the set optional ones

for example:

i search for

+foo type:qemu type:lxc

foo is required, type:qemu and type:lxc optional

if one thing would match foo but not type:qemu and not type:lxc it 
should not appear in the result list, because otherwise
i could have written

+foo

and achieve the same result

but i wanted everything with "foo" in it that is from type "qemu" or "lxc"

does that make sense?

Maybe i have to describe it better in the commit message?

> 
>>
>> 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>
>> ---
>> changes from v3:
>> * add missing doc comments
>> * more tests
>>
>>   Cargo.toml                |   2 +
>>   lib/pdm-search/Cargo.toml |  12 ++
>>   lib/pdm-search/src/lib.rs | 339 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 353 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 f07733d..ae2816a 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..4854166
>> --- /dev/null
>> +++ b/lib/pdm-search/src/lib.rs
>> @@ -0,0 +1,339 @@
>> +//! 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;
> 
> ↑ you never use any of its methods, for the single trait implementation
> it's fine to just `impl std::str::FromStr` below
> 
> OTOH you do implement `Display` twice without importing it (which is
> fine, don't but importing just `std::fmt` will shorten it all...)
> 

right, i'll do `use std::fmt` and `fmt::Display` etc.

> 
>> +
>> +use anyhow::bail;
>> +
>> +#[derive(Clone)]
>> +pub struct Search {
>> +    required_terms: Vec<SearchTerm>,
>> +    optional_terms: Vec<SearchTerm>,
> 
> ↑ In theory - is partitioning them very useful, or would it be a better
> UX if we kept them in a single vec such that `.parse().to_string()`
> doesn't reorder them?
> 

mhmm. i have to think about that, but since we might
want to filter a large amount of resource, having it pre-partitioned
can speed up the matching

we could save both, original order and partitioned values

since most of the time a single search won't contain that many terms

(i guess 10-20 at max) so the little extra wasted space
could be a good tradeoff for keeping the order

OTOH, we don't currently parse and display it again

we parse it for user input, and display it for auto generated
searches, in both cases it does not really matter if
we have the original order or not....

>> +}
>> +
>> +impl Default for Search {
> 
> A derive should work here
> 
>> +    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())
> 
> Maybe add a note here why we do this, as this is quite painful to read.
> We throw away errors because we expect users to produce them? ;-)

yes, basically

if a user enters something we cannot sure what we're searching for
there is currently only two cases where that can happen:

the string ':a'; was it meant with an empty category and
the value "a", or the literal string ':a'

the other error case is when the term would be empty
* a single '+' -> could again interpret literally
* an empty string '' -> should not be possible from the user side
   since we split on whitespace

i could adapt it to take that as a literal term though and implement
From<AsRef<str>> instead of FromStr

> 
>> +            .collect()
>> +    }
>> +}
>> +
>> +impl Search {
>> +    /// Create a new empty [`Search`]
>> +    pub fn new() -> Self {
>> +        Self::with_terms(Vec::new())
>> +    }
>> +
>> +    /// Returns true if no [`SearchTerm`] exist
>> +    pub fn is_empty(&self) -> bool {
>> +        self.required_terms.is_empty() && self.optional_terms.is_empty()
>> +    }
>> +
>> +    /// Create a new [`Search`] with the given [`SearchTerm`]s
>> +    pub fn with_terms(terms: Vec<SearchTerm>) -> Self {
> 
> (
> Since we already have `FromIterator`, this could be
> 
>      pub fn with_terms<I>(terms: I) -> Self
>      where
>          I: IntoIterator<Item = SearchTerm>;
> )

true

> 
>> +        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) {
> 
> Why does this use `.contains()` but the loop over optional ones uses
> `==`?

good point

this probably does not belong here at all, but as a helper in the api
where we search

we only use for 'fast skipping' some infos there when we know we only
look for remotes

i'd leave it out here and make a special check in the api. we can still
introduce it if we need it somewhere else too

> 
>> +                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}")?;
>> +        }
> 
> personal opinion: enumerate() to only handle the first element
> differently seems a bit much, if you consider that it keeps a counter,
> you could keep a "separator" instead which you just update once:
> 
>      let mut sep = "";
>      for term in &self.required_terms {
>          write!(f, "{sep}{term}")?;
>          sep = " ";
>      }
> 
yes makes sense

>> +
>> +        if !self.required_terms.is_empty() && !self.optional_terms.is_empty() {
>> +            write!(f, " ")?;
>> +        }
> 
> ↑ This could then be skipped entirely and we can reuse `sep` inside the
> lower loop the same way - if no previous text was there, the first
> element will still be using an empty string for `sep` before updating it
> to be `" "` for the remaining elements...
> 
> Then the entire implementation is just:
>      let mut sep = "";
>      for term in &self.required_terms {
>          write!(f, "{sep}{term}")?;
>          sep = " ";
>      }
>      for term in &self.optional_terms {
>          write!(f, "{sep}{term}")?;
>          sep = " ";
>      }
> 
> of if you're feeling fancy:
> 
>      let mut sep = "";
>      for term in self.required_terms.iter().chain(self.optional_terms.iter()) {
>          write!(f, "{sep}{term}")?;
>          sep = " ";
>      }
> 

thanks i'll see to integrate taht

>> +
>> +        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 {
> 
> ↑ use `Into<String>`, `ToString` will cause an unnecessary clone when
> passing an already owned `String` as it is implemented via `Display`.
> 
>> +        Self {
>> +            value: term.to_string(),
>> +            optional: false,
>> +            category: None,
>> +        }
>> +    }
>> +
>> +    /// Builder style method to set the category
>> +    pub fn category<S: ToString>(mut self, category: Option<S>) -> Self {
>> +        self.category = category.map(|s| s.to_string());
>> +        self
>> +    }
>> +
>> +    /// Builder style method to mark this [`SearchTerm`] as optional
>> +    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> {
> 
> ↑ use: (mut term: &str)
> 
>> +        let mut optional = true;
>> +        let mut term: String = s.into();
> 
> ↑ skip the premature allocation ;-)
> 
>> +        if term.starts_with("+") {
>> +            optional = false;
>> +            term.remove(0);
>> +        }
> 
> ↑
>      if let Some(rest) = term.strip_prefix("+") {
>          term = rest;
>          optional = false;
>      }
> 
>> +
>> +        let (term, category) = if let Some(idx) = term.find(":") {
>> +            let mut real_term = term.split_off(idx);
>> +            real_term.remove(0); // remove ':'
> 
> ↑ 2 expensive avoidable ops, see below
> 
>> +            (real_term, Some(term))
>> +        } else {
>> +            (term, None)
>> +        };
> 
> ↑
>      let category;
>      (term, category) = match term.split_once(':') {
>          Some((category, term)) => (term, Some(category)),
>          None => (term, None),
>      }
> 
>> +
>> +        if term.is_empty() {
>> +            bail!("term cannot be empty");
>> +        }
>> +
>> +        if category == Some("".into()) {
> 
> ↑ Can avoid this (also, `String::new()` would be cheaper) via:
> 
>      if category.is_some_and(str::is_empty) {
> 
> 
>> +            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),
>> +        ];
>> +        for (input, expected) in cases {
>> +            assert!(
>> +                search.matches(|term| {
>> +                    match term.value.as_str() {
>> +                        "required1" => input.0,
>> +                        "required2" => input.1,
>> +                        "optional1" => input.2,
>> +                        "optional2" => input.3,
>> +                        _ => unreachable!(),
>> +                    }
>> +                }) == expected
>> +            )
>> +        }
>> +    }
>> +
>> +    #[test]
>> +    fn category_value_required() {
>> +        let search: Search = Search::from_iter(vec![SearchTerm::new("foo")]);
>> +        assert!(!search.category_value_required("bar", "baz"));
>> +
>> +        let search: Search = Search::from_iter(vec![SearchTerm::new("foo").optional(true)]);
>> +        assert!(!search.category_value_required("bar", "baz"));
>> +
>> +        let search: Search = Search::from_iter(vec![SearchTerm::new("foo").category(Some("bar"))]);
>> +        assert!(!search.category_value_required("bar", "baz"));
>> +
>> +        let search: Search = Search::from_iter(vec![SearchTerm::new("baz").category(Some("bar"))]);
>> +        assert!(search.category_value_required("bar", "baz"));
>> +
>> +        let search: Search = Search::from_iter(vec![SearchTerm::new("foo")
>> +            .optional(true)
>> +            .category(Some("bar"))]);
>> +        assert!(!search.category_value_required("bar", "baz"));
>> +
>> +        let search: Search = Search::from_iter(vec![SearchTerm::new("baz")
>> +            .optional(true)
>> +            .category(Some("bar"))]);
>> +        assert!(search.category_value_required("bar", "baz"));
>> +
>> +        let search: Search = Search::from_iter(vec![
>> +            SearchTerm::new("baz").optional(true).category(Some("bar")),
>> +            SearchTerm::new("foo").optional(true),
>> +        ]);
>> +        assert!(!search.category_value_required("bar", "baz"));
>> +    }
>> +
>> +    #[test]
>> +    fn test_display() {
>> +        let term = SearchTerm::new("foo");
>> +        assert_eq!("+foo", &term.to_string());
>> +
>> +        let term = SearchTerm::new("foo").optional(true);
>> +        assert_eq!("foo", &term.to_string());
>> +
>> +        let term = SearchTerm::new("foo").optional(false);
>> +        assert_eq!("+foo", &term.to_string());
>> +
>> +        let term = SearchTerm::new("foo").category(Some("bar"));
>> +        assert_eq!("+bar:foo", &term.to_string());
>> +
>> +        let term = SearchTerm::new("foo").optional(true).category(Some("bar"));
>> +        assert_eq!("bar:foo", &term.to_string());
>> +
>> +        let term = SearchTerm::new("foo").optional(false).category(Some("bar"));
>> +        assert_eq!("+bar:foo", &term.to_string());
>> +    }
>> +}
>> -- 
>> 2.47.2



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

Re: [pdm-devel] [PATCH datacenter-manager v4 03/10] server: api: resources: add more complex filter syntax

[Wolfgang Bumiller]

On Thu, Aug 28, 2025 at 03:16:02PM +0200, Dominik Csapak wrote:
> by using the new pdm-search crate for the resources api call.
> 
> We have to do 3 filter passes:
> * one fast pass for remotes if the filter are constructed in a way that
>   must filter to 'remote' (in this case we don't have to look at/return
>   the resources at all, and can skip remotes that don't match)
> * a pass for the resources
> * a second pass for the remotes that check if they match for
>   remote/non-remote mixed results
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v3:
> * use starts_with for filtering type/status
>  server/Cargo.toml           |  1 +
>  server/src/api/resources.rs | 80 +++++++++++++++++++++++++++++++------
>  2 files changed, 69 insertions(+), 12 deletions(-)
> 
> diff --git a/server/Cargo.toml b/server/Cargo.toml
> index 24a2e40..ada7c80 100644
> --- a/server/Cargo.toml
> +++ b/server/Cargo.toml
> @@ -74,6 +74,7 @@ proxmox-acme-api = { workspace = true, features = [ "impl" ] }
>  pdm-api-types.workspace = true
>  pdm-buildcfg.workspace = true
>  pdm-config.workspace = true
> +pdm-search.workspace = true
>  
>  pve-api-types = { workspace = true, features = [ "client" ] }
>  pbs-api-types.workspace = true
> diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
> index fb6218a..8114944 100644
> --- a/server/src/api/resources.rs
> +++ b/server/src/api/resources.rs
> @@ -15,12 +15,13 @@ use pdm_api_types::subscription::{
>      NodeSubscriptionInfo, RemoteSubscriptionState, RemoteSubscriptions, SubscriptionLevel,
>  };
>  use pdm_api_types::{Authid, PRIV_RESOURCE_AUDIT};
> +use pdm_search::{Search, SearchTerm};
>  use proxmox_access_control::CachedUserInfo;
>  use proxmox_router::{
>      http_bail, list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap,
>  };
>  use proxmox_rrd_api_types::RrdTimeframe;
> -use proxmox_schema::api;
> +use proxmox_schema::{api, parse_boolean};
>  use proxmox_sortable_macro::sortable;
>  use proxmox_subscription::SubscriptionStatus;
>  use pve_api_types::{ClusterResource, ClusterResourceType};
> @@ -45,6 +46,44 @@ const SUBDIRS: SubdirMap = &sorted!([
>      ),
>  ]);
>  
> +fn resource_matches_search_term(resource: &Resource, term: &SearchTerm) -> bool {
> +    match &term.category {
> +        Some(category) => match category.as_str() {
> +            "type" => resource.resource_type().starts_with(&term.value),
> +            "name" => resource.name().contains(&term.value),

↑ The starts_with vs contains decisions based on category happening in
multiple places makes me think that maybe - since we have a fixed set of
categories anyway - the category should be parsed out into an `enum`
which contains a `matches` method which decides how to do this in a
consistent way, such that we don't end up doing different matches in
different functions?

The `fn remote_matches_search_term` below also has to copy the same
distinction... This is a maintainability hazard which will cause subtle
bugs with weird search behavior...

> +            "id" => resource.id().contains(&term.value),
> +            "status" => resource.status().starts_with(&term.value),
> +            "template" => match resource {
> +                Resource::PveQemu(PveQemuResource { template, .. }, ..)
> +                | Resource::PveLxc(PveLxcResource { template, .. }) => {
> +                    match parse_boolean(&term.value) {
> +                        Ok(boolean) => boolean == *template,
> +                        Err(_) => false,
> +                    }

↑ could be shortened to
    parse_boolean(&term.value).is_ok_and(|v| v == *template)
or
    parse_boolean(&term.value).ok() == Some(*template)

> +                }
> +                _ => false,
> +            },
> +            "remote" => true, // this has to be checked beforehand

If this module gets more complex, for maintainability / to avoid subtle
bugs with such conditions, it may be useful to have this fn return an
`Option<bool>` where this case returns `None`, aka. "don't know how to
check this term" (or some enum describing what exactly it cannot check,
depending on how/where we do verify this)

> +            _ => false,
> +        },
> +        None => resource.name().contains(&term.value) || resource.id().contains(&term.value),
> +    }
> +}
> +
> +fn remote_matches_search_term(remote_name: &str, online: Option<bool>, term: &SearchTerm) -> bool {
> +    match term.category.as_deref() {
> +        Some("remote" | "name" | "id") => remote_name.contains(&term.value),
> +        Some("type") => "remote".starts_with(&term.value),
> +        Some("status") => match online {
> +            None => true,
> +            Some(true) => "online".starts_with(&term.value),
> +            Some(false) => "offline".starts_with(&term.value),
> +        },
> +        None => remote_name.contains(&term.value) || "remote".contains(&term.value),
> +        Some(_) => false,
> +    }
> +}
> +
>  #[api(
>      // FIXME:: see list-like API calls in resource routers, we probably want more fine-grained
>      // checks..
> @@ -104,6 +143,10 @@ pub(crate) async fn get_resources_impl(
>      let (remotes_config, _) = pdm_config::remotes::config()?;
>      let mut join_handles = Vec::new();
>  
> +    let filters = search.map(Search::from).unwrap_or_default();
> +
> +    let remotes_only = filters.category_value_required("type", "remote");
> +
>      for (remote_name, remote) in remotes_config {
>          if let Some(ref auth_id) = opt_auth_id {
>              let remote_privs = user_info.lookup_privs(auth_id, &["resource", &remote_name]);
> @@ -111,12 +154,27 @@ pub(crate) async fn get_resources_impl(
>                  continue;
>              }
>          }
> +
> +        if remotes_only
> +            && !filters.matches(|term| remote_matches_search_term(&remote_name, None, term))
> +        {
> +            continue;
> +        }
> +        let filter = filters.clone();

^ Could wrap filters in an `Arc` to make this cheap.

>          let handle = tokio::spawn(async move {
> -            let (resources, error) = match get_resources_for_remote(remote, max_age).await {
> +            let (mut resources, error) = match get_resources_for_remote(remote, max_age).await {
>                  Ok(resources) => (resources, None),
>                  Err(error) => (Vec::new(), Some(error.to_string())),
>              };
>  
> +            if remotes_only {
> +                resources.clear();

> +            } else if !filter.is_empty() {
> +                resources.retain(|resource| {
> +                    filter.matches(|filter| resource_matches_search_term(resource, filter))
> +                });
> +            }
> +
>              RemoteResources {
>                  remote: remote_name,
>                  resources,
> @@ -132,17 +190,15 @@ pub(crate) async fn get_resources_impl(
>          remote_resources.push(handle.await?);
>      }
>  
> -    if let Some(search) = search {
> -        // FIXME implement more complex filter syntax
> -        remote_resources.retain_mut(|res| {
> -            if res.remote.contains(&search) {
> -                true
> -            } else {
> -                res.resources
> -                    .retain(|res| res.id().contains(&search) || res.name().contains(&search));
> -                !res.resources.is_empty()
> +    if !filters.is_empty() {
> +        remote_resources.retain(|res| {
> +            if !res.resources.is_empty() {
> +                return true;
>              }
> -        });
> +            filters.matches(|filter| {
> +                remote_matches_search_term(&res.remote, Some(res.error.is_none()), filter)
> +            })
> +        })
>      }
>  
>      Ok(remote_resources)
> -- 
> 2.47.2


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

[pdm-devel] superseded: [PATCH datacenter-manager v4 00/10] implement more complex search syntax

[Dominik Csapak]

superseded by v5:
https://lore.proxmox.com/pdm-devel/20250903132351.841830-1-d.csapak@proxmox.com/


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