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

[Fabian Grünbichler]

On September 16, 2021 4:46 pm, Dominik Csapak wrote:
> 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 at 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

not moving it was the reason why I did it that way, but if moving is 
okay, moving is probably better ;)

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

using the schema we get the schema error message, which is probably what we 
want.

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

indeed it does :) only question is whether we want to enable this 
updater behaviour in general for Vec (and if we ever need "add/remove 
element(s)" semantics we do that via a custom type with custom Updater?)

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