From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id AD5236A508 for ; Thu, 16 Sep 2021 16:47:18 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 97BE026E0C for ; Thu, 16 Sep 2021 16:46:48 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 22B3226DFF for ; Thu, 16 Sep 2021 16:46:47 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D09F044905; Thu, 16 Sep 2021 16:46:46 +0200 (CEST) Message-ID: <328852de-e06e-a356-84e4-c8e7795b085f@proxmox.com> Date: Thu, 16 Sep 2021 16:46:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:93.0) Gecko/20100101 Thunderbird/93.0 Content-Language: en-US To: Proxmox VE development discussion , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20210915134157.19762-1-f.gruenbichler@proxmox.com> <20210915134157.19762-3-f.gruenbichler@proxmox.com> From: Dominik Csapak In-Reply-To: <20210915134157.19762-3-f.gruenbichler@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.235 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -1.698 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v2 proxmox-backup 02/10] api: add GroupFilter(List) type X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Sep 2021 14:47:18 -0000 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 > --- > this is working around some api limitations > - updated doesn't support a Vec 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, > } > > +#[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 { > + 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 '|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(&self, serializer: S) -> Result > + where > + S: serde::Serializer, > + { > + serializer.serialize_str(&self.to_string()) > + } > +} > + > +impl<'de> Deserialize<'de> for GroupFilter { > + fn deserialize(deserializer: D) -> Result > + 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:'), or regex ('regex:RE').") > + .format(&ApiStringFormat::VerifyFn(verify_group_filter)) > + .type_text("|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); > + > +impl GroupFilterList { > + /// converts schema-validated Vec into Vec > + pub fn filters(self) -> Vec { > + 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 UpdaterType for Vec { type Updater = Option; } 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 as the api call parameters. makes the code a little easier to read > #[api( > properties: { > id: { >