Re: [pve-devel] [PATCH v2 proxmox-backup 02/10] api: add GroupFilter(List) type

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

On 9/15/21 15:41, Fabian Grünbichler wrote:
> at the API level, this is a simple (wrapped) Vec of Strings with a
> verifier function. all users should use the provided helper to get the
> actual GroupFilter enum values, which can't be directly used in the API
> schema because of restrictions of the api macro.
> 
> validation of the schema + parsing into the proper type uses the same fn
> intentionally to avoid running out of sync, even if it means compiling
> the REs twice.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> this is working around some api limitations
> - updated doesn't support a Vec<String> type
> - api-macro doesn't support enum with values
> 
> the validation fn and the parser/converted use the same code to avoid
> getting out of sync, at the expense of parsing potentially expensive REs
> twice..
> 
>   pbs-api-types/src/jobs.rs | 90 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 90 insertions(+)
> 
> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
> index 1526dbc4..94638fe5 100644
> --- a/pbs-api-types/src/jobs.rs
> +++ b/pbs-api-types/src/jobs.rs
> @@ -1,3 +1,7 @@
> +use anyhow::format_err;
> +use std::str::FromStr;
> +
> +use regex::Regex;
>   use serde::{Deserialize, Serialize};
>   
>   use proxmox::const_regex;
> @@ -7,6 +11,7 @@ use proxmox::api::{api, schema::*};
>   use crate::{
>       Userid, Authid, REMOTE_ID_SCHEMA, DRIVE_NAME_SCHEMA, MEDIA_POOL_NAME_SCHEMA,
>       SINGLE_LINE_COMMENT_SCHEMA, PROXMOX_SAFE_ID_FORMAT, DATASTORE_SCHEMA,
> +    BACKUP_GROUP_SCHEMA, BACKUP_TYPE_SCHEMA,
>   };
>   
>   const_regex!{
> @@ -319,6 +324,91 @@ pub struct TapeBackupJobStatus {
>       pub next_media_label: Option<String>,
>   }
>   
> +#[derive(Clone, Debug)]
> +/// Filter for matching `BackupGroup`s, for use with `BackupGroup::filter`.
> +pub enum GroupFilter {
> +    /// BackupGroup type - either `vm`, `ct`, or `host`.
> +    BackupType(String),
> +    /// Full identifier of BackupGroup, including type
> +    Group(String),
> +    /// A regular expression matched against the full identifier of the BackupGroup
> +    Regex(Regex),
> +}

Would it not make sense to have an already parsed "BackupGroup" in
the Group Variant instead of a string? we would have
to move the pub struct BackupGroup here, but i think the impl block can 
stay in pbs-datastore

> +
> +impl std::str::FromStr for GroupFilter {
> +    type Err = anyhow::Error;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        match s.split_once(":") {
> +            Some(("group", value)) => parse_simple_value(value, &BACKUP_GROUP_SCHEMA).map(|_| GroupFilter::Group(value.to_string())),

here you could parse it directly to a 'BackupGroup' then we would not
need the BACKUP_GROUP_SCHEMA

> +            Some(("type", value)) => parse_simple_value(value, &BACKUP_TYPE_SCHEMA).map(|_| GroupFilter::BackupType(value.to_string())),

nit: would it not be enough to match the regex instead of the schema?
(not that i think it'd make a difference)

> +            Some(("regex", value)) => Ok(GroupFilter::Regex(Regex::new(value)?)),
> +            Some((ty, _value)) => Err(format_err!("expected 'group', 'type' or 'regex' prefix, got '{}'", ty)),
> +            None => Err(format_err!("input doesn't match expected format '<group:GROUP||type:<vm|ct|host>|regex:REGEX>'")),
> +        }.map_err(|err| format_err!("'{}' - {}", s, err))
> +    }
> +}
> +
> +// used for serializing below, caution!
> +impl std::fmt::Display for GroupFilter {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        match self {
> +            GroupFilter::BackupType(backup_type) => write!(f, "type:{}", backup_type),
> +            GroupFilter::Group(backup_group) => write!(f, "group:{}", backup_group),
> +            GroupFilter::Regex(regex) => write!(f, "regex:{}", regex.as_str()),
> +        }
> +    }
> +}
> +
> +impl Serialize for GroupFilter {
> +    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
> +    where
> +        S: serde::Serializer,
> +    {
> +        serializer.serialize_str(&self.to_string())
> +    }
> +}
> +
> +impl<'de> Deserialize<'de> for GroupFilter {
> +    fn deserialize<D>(deserializer: D) -> Result<GroupFilter, D::Error>
> +    where
> +        D: serde::Deserializer<'de>,
> +    {
> +        String::deserialize(deserializer).and_then(|string| {
> +            GroupFilter::from_str(&string).map_err(serde::de::Error::custom)
> +        })
> +    }
> +}
> +
> +fn verify_group_filter(input: &str) -> Result<(), anyhow::Error> {
> +    GroupFilter::from_str(input).map(|_| ())
> +}
> +
> +pub const GROUP_FILTER_SCHEMA: Schema = StringSchema::new(
> +    "Group filter based on group identifier ('group:GROUP'), group type ('type:<vm|ct|host>'), or regex ('regex:RE').")
> +    .format(&ApiStringFormat::VerifyFn(verify_group_filter))
> +    .type_text("<type:<vm|ct|host>|group:GROUP|regex:RE>")
> +    .schema();
> +
> +#[api(
> +    type: Array,
> +    description: "List of group filters.",
> +    items: {
> +        schema: GROUP_FILTER_SCHEMA,
> +    },
> +)]
> +#[derive(Serialize, Deserialize, Clone, UpdaterType)]
> +pub struct GroupFilterList(Vec<String>);
> +
> +impl GroupFilterList {
> +    /// converts schema-validated Vec<String> into Vec<GroupFilter>
> +    pub fn filters(self) -> Vec<GroupFilter> {
> +        self.0.iter()
> +             .map(|group_filter| GroupFilter::from_str(group_filter).unwrap())
> +             .collect()
> +    }
> +}
> +

IMHO that has to be easier:

instead of the custom type, i'd do:

const GROUP_FILTER_LIST_SCHEMA: Schema = ArraySchema::new("",
	&GROUP_FILTER_SCHEMA).schema()

the only thing you'd have to do is to convice the updater that
there is an updaterType for it: (in proxmox/src/api/schema.rs of the 
proxmox crate)

impl<T> UpdaterType for Vec<T> {
     type Updater = Option<Self>;
}

that implies though that a user always has to give the complete list
(so no single removal from the list), though i think that's what
we want anyway..

i tested it here, and AFAICS, this works (i can update/list the sync job 
properly)

with that you can also remove the 'filters' fn and can use
Vec<GroupFilter> as the api call parameters.

makes the code a little easier to read

>   #[api(
>       properties: {
>           id: {
>