Re: [pdm-devel] [PATCH datacenter-manager v2 01/12] pdm-api-types: views: add ViewFilterConfig type

[Dominik Csapak <d.csapak@proxmox.com>]

some comments inline

On 11/3/25 1:35 PM, Lukas Wagner wrote:
> This type will be used to define view filters.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> 
> Notes:
>      Changes since RFC:
>        - Change config structure, instead of 'include-{x} value' we now do
>          'include {x}:value'
> 
>   lib/pdm-api-types/src/lib.rs   |   2 +
>   lib/pdm-api-types/src/views.rs | 165 +++++++++++++++++++++++++++++++++
>   2 files changed, 167 insertions(+)
>   create mode 100644 lib/pdm-api-types/src/views.rs
> 
> diff --git a/lib/pdm-api-types/src/lib.rs b/lib/pdm-api-types/src/lib.rs
> index 2fb61ef7..a7ba6d89 100644
> --- a/lib/pdm-api-types/src/lib.rs
> +++ b/lib/pdm-api-types/src/lib.rs
> @@ -106,6 +106,8 @@ pub mod subscription;
>   
>   pub mod sdn;
>   
> +pub mod views;
> +
>   const_regex! {
>       // just a rough check - dummy acceptor is used before persisting
>       pub OPENSSL_CIPHERS_REGEX = r"^[0-9A-Za-z_:, +!\-@=.]+$";
> diff --git a/lib/pdm-api-types/src/views.rs b/lib/pdm-api-types/src/views.rs
> new file mode 100644
> index 00000000..b262cc05
> --- /dev/null
> +++ b/lib/pdm-api-types/src/views.rs
> @@ -0,0 +1,165 @@
> +use std::{fmt::Display, str::FromStr};
> +
> +use anyhow::bail;
> +use serde::{Deserialize, Serialize};
> +
> +use proxmox_schema::{api, ApiStringFormat, ArraySchema, Schema, StringSchema, Updater};
> +
> +use crate::{remotes::REMOTE_ID_SCHEMA, resource::ResourceType, VIEW_FILTER_ID_SCHEMA};

you use the VIEW_FILTER_ID_SCHEMA here but it's only introduced in the 
next patch

> +
> +#[api(
> +    properties: {
> +        "id": {
> +            schema: VIEW_FILTER_ID_SCHEMA,
> +        },
> +        "include": {
> +            schema: FILTER_RULE_LIST_SCHEMA,
> +            optional: true,
> +        },
> +        "exclude": {
> +            schema: FILTER_RULE_LIST_SCHEMA,
> +            optional: true,
> +        }
> +    }
> +)]
> +#[derive(Clone, Default, Deserialize, Serialize, Updater)]
> +#[serde(rename_all = "kebab-case")]
> +/// View definition
> +pub struct ViewFilterConfig {
> +    /// View filter name.
> +    pub id: String,
> +
> +    /// List of includes.
> +    #[serde(default, skip_serializing_if = "Vec::is_empty")]
> +    #[updater(serde(skip_serializing_if = "Option::is_none"))]
> +    pub include: Vec<FilterRule>,
> +
> +    /// List of excludes.
> +    #[serde(default, skip_serializing_if = "Vec::is_empty")]
> +    #[updater(serde(skip_serializing_if = "Option::is_none"))]
> +    pub exclude: Vec<FilterRule>,
> +}
> +
> +#[derive(Clone, Debug, PartialEq)]
> +/// Filter rule for includes/excludes.
> +pub enum FilterRule {
> +    /// Match a resource type.
> +    ResourceType(ResourceType),
> +    /// Match a resource pools (for PVE guests).
> +    ResourcePool(String),
> +    /// Match a (global) resource ID, e.g. 'remote/<remote>/guest/<vmid>'.
> +    ResourceId(String),
> +    /// Match a tag (for PVE guests).
> +    Tag(String),
> +    /// Match a remote.
> +    Remote(String),
> +}
> +
> +impl FromStr for FilterRule {
> +    type Err = anyhow::Error;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        Ok(match s.split_once(':') {
> +            Some(("resource-type", value)) => FilterRule::ResourceType(value.parse()?),
> +            Some(("resource-pool", value)) => {
> +                // TODO: Define schema and use it to validate.

couldn't we reuse a schema/regex here ? e.g. PROXMOX_SAFE_ID_REGEX ?

> +                if value.is_empty() {
> +                    bail!("empty resource-pool rule not allowed");
> +                }
> +                FilterRule::ResourcePool(value.to_string())
> +            }
> +            Some(("resource-id", value)) => {
> +                // TODO: Define schema and use it to validate.

same here

> +                if value.is_empty() {
> +                    bail!("empty resource-id rule not allowed");
> +                }
> +                FilterRule::ResourceId(value.to_string())
> +            }
> +            Some(("tag", value)) => {
> +                // TODO: Define schema and use it to validate.

and here

> +                if value.is_empty() {
> +                    bail!("empty tag rule not allowed");
> +                }
> +                FilterRule::Tag(value.to_string())
> +            }
> +            Some(("remote", value)) => {
> +                let _ = REMOTE_ID_SCHEMA.parse_simple_value(value)?;
> +                FilterRule::Remote(value.to_string())
> +            }
> +            Some((ty, _)) => bail!("invalid type: {ty}"),
> +            None => bail!("invalid filter rule: {s}"),
> +        })
> +    }
> +}
> +
> +// used for serializing below, caution!
> +impl Display for FilterRule {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        match self {
> +            FilterRule::ResourceType(resource_type) => write!(f, "resource-type:{resource_type}"),
> +            FilterRule::ResourcePool(pool) => write!(f, "resource-pool:{pool}"),
> +            FilterRule::ResourceId(id) => write!(f, "resource-id:{id}"),
> +            FilterRule::Tag(tag) => write!(f, "tag:{tag}"),
> +            FilterRule::Remote(remote) => write!(f, "remote:{remote}"),
> +        }
> +    }
> +}
> +
> +proxmox_serde::forward_deserialize_to_from_str!(FilterRule);
> +proxmox_serde::forward_serialize_to_display!(FilterRule);
> +
> +fn verify_group_filter(input: &str) -> Result<(), anyhow::Error> {
> +    FilterRule::from_str(input).map(|_| ())
> +}
> +
> +/// Schema for filter rules.
> +pub const FILTER_RULE_SCHEMA: Schema = StringSchema::new("Filter rule for resources.")
> +    .format(&ApiStringFormat::VerifyFn(verify_group_filter))
> +    .type_text(
> +        "resource-type:storage|qemu|lxc|sdn-zone|datastore|node>\
> +            |resource-pool:<pool-name>\
> +            |tag:<tag-name>\
> +            |remote:<remote-name>\
> +            |resource-id:<resource-id>",
> +    )
> +    .schema();
> +
> +/// Schema for list of filter rules.
> +pub const FILTER_RULE_LIST_SCHEMA: Schema =
> +    ArraySchema::new("List of filter rules.", &FILTER_RULE_SCHEMA).schema();

nit: i would find it nicer if theses definitions are at the top of the 
file, before they are used

> +
> +#[cfg(test)]
> +mod test {
> +    use anyhow::Error;
> +
> +    use crate::views::FilterRule;
> +
> +    fn parse_and_check_display(input: &str) -> Result<bool, Error> {
> +        let rule: FilterRule = input.parse()?;
> +
> +        Ok(input == format!("{rule}"))
> +    }
> +
> +    #[test]
> +    fn test_filter_rule() {
> +        assert!(parse_and_check_display("abc").is_err());
> +        assert!(parse_and_check_display("abc:").is_err());
> +
> +        assert!(parse_and_check_display("resource-type:").is_err());
> +        assert!(parse_and_check_display("resource-type:lxc").unwrap());
> +        assert!(parse_and_check_display("resource-type:qemu").unwrap());
> +        assert!(parse_and_check_display("resource-type:abc").is_err());
> +
> +        assert!(parse_and_check_display("resource-pool:").is_err());
> +        assert!(parse_and_check_display("resource-pool:somepool").unwrap());
> +
> +        assert!(parse_and_check_display("resource-id:").is_err());
> +        assert!(parse_and_check_display("resource-id:remote/someremote/guest/100").unwrap());
> +
> +        assert!(parse_and_check_display("tag:").is_err());
> +        assert!(parse_and_check_display("tag:sometag").unwrap());
> +
> +        assert!(parse_and_check_display("remote:someremote").unwrap());
> +        assert!(parse_and_check_display("remote:a").is_err());
> +    }
> +}



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