Re: [pdm-devel] [PATCH datacenter-manager v2 3/9] server: api: resources: add more complex filter syntax

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

comments inline

On 8/25/25 11:00 AM, 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>
> ---
>  server/Cargo.toml           |  1 +
>  server/src/api/resources.rs | 84 +++++++++++++++++++++++++++++++------
>  2 files changed, 73 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 6a8c8ef..027298a 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().contains(&term.value),
> +            "name" => resource.name().contains(&term.value),
> +            "id" => resource.id().contains(&term.value),
> +            "status" => resource.status().contains(&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".contains(&term.value),
> +        Some("status") => match online {
> +            None => true,
> +            Some(true) => "online".contains(&term.value),
> +            Some(false) => "offline".contains(&term.value),
> +        },
> +        None => remote_name.contains(&term.value) || "remote".contains(&term.value),
> +        Some(_) => false,
> +    }
> +}
> +

I wonder, if in the future it would make sense to make SearchTerm
generic over SearchTerm<Category, Value> with String as default for
both. But currently that's probably a YAGNI.


>  #[api(
>      // FIXME:: see list-like API calls in resource routers, we probably want more fine-grained
>      // checks..
> @@ -104,6 +143,14 @@ pub(crate) async fn get_resources_impl(
>      let (remotes_config, _) = pdm_config::remotes::config()?;
>      let mut join_handles = Vec::new();
>  
> +    let mut filters = Search::new();
> +
> +    if let Some(search) = &search {
> +        filters = Search::from(search);
> +    }

nit: if we had a Default implementation we could do

let mut 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 +158,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 +194,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)



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