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.gruenbich...@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: {
>>
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
