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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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