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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 0A4746A6A5 for ; Fri, 17 Sep 2021 08:40:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E88982BE99 for ; Fri, 17 Sep 2021 08:39:53 +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 0F01D2BE8E for ; Fri, 17 Sep 2021 08:39:53 +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 D8C78425F9 for ; Fri, 17 Sep 2021 08:39:52 +0200 (CEST) Date: Fri, 17 Sep 2021 08:39:43 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Dominik Csapak , Proxmox VE development discussion References: <20210915134157.19762-1-f.gruenbichler@proxmox.com> <20210915134157.19762-3-f.gruenbichler@proxmox.com> <328852de-e06e-a356-84e4-c8e7795b085f@proxmox.com> In-Reply-To: <328852de-e06e-a356-84e4-c8e7795b085f@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1631860586.e1cb2tjf3e.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.361 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [jobs.rs, schema.rs] 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: Fri, 17 Sep 2021 06:40:24 -0000 On September 16, 2021 4:46 pm, Dominik Csapak wrote: > On 9/15/21 15:41, Fabian Gr=C3=BCnbichler 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. >>=20 >> 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. >>=20 >> Signed-off-by: Fabian Gr=C3=BCnbichler >> --- >> this is working around some api limitations >> - updated doesn't support a Vec type >> - api-macro doesn't support enum with values >>=20 >> 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.. >>=20 >> pbs-api-types/src/jobs.rs | 90 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 90 insertions(+) >>=20 >> 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}; >> =20 >> 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_NA= ME_SCHEMA, >> SINGLE_LINE_COMMENT_SCHEMA, PROXMOX_SAFE_ID_FORMAT, DATASTORE_SCHE= MA, >> + BACKUP_GROUP_SCHEMA, BACKUP_TYPE_SCHEMA, >> }; >> =20 >> const_regex!{ >> @@ -319,6 +324,91 @@ pub struct TapeBackupJobStatus { >> pub next_media_label: Option, >> } >> =20 >> +#[derive(Clone, Debug)] >> +/// Filter for matching `BackupGroup`s, for use with `BackupGroup::filt= er`. >> +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), >> +} >=20 > 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=20 > stay in pbs-datastore not moving it was the reason why I did it that way, but if moving is=20 okay, moving is probably better ;) >=20 >> + >> +impl std::str::FromStr for GroupFilter { >> + type Err =3D anyhow::Error; >> + >> + fn from_str(s: &str) -> Result { >> + match s.split_once(":") { >> + Some(("group", value)) =3D> parse_simple_value(value, &BACK= UP_GROUP_SCHEMA).map(|_| GroupFilter::Group(value.to_string())), >=20 > here you could parse it directly to a 'BackupGroup' then we would not > need the BACKUP_GROUP_SCHEMA >=20 >> + Some(("type", value)) =3D> parse_simple_value(value, &BACKU= P_TYPE_SCHEMA).map(|_| GroupFilter::BackupType(value.to_string())), >=20 > 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= =20 want. >=20 >> + Some(("regex", value)) =3D> Ok(GroupFilter::Regex(Regex::ne= w(value)?)), >> + Some((ty, _value)) =3D> Err(format_err!("expected 'group', = 'type' or 'regex' prefix, got '{}'", ty)), >> + None =3D> Err(format_err!("input doesn't match expected for= mat '|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) =3D> write!(f, "type:{= }", backup_type), >> + GroupFilter::Group(backup_group) =3D> write!(f, "group:{}",= backup_group), >> + GroupFilter::Regex(regex) =3D> 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::cu= stom) >> + }) >> + } >> +} >> + >> +fn verify_group_filter(input: &str) -> Result<(), anyhow::Error> { >> + GroupFilter::from_str(input).map(|_| ()) >> +} >> + >> +pub const GROUP_FILTER_SCHEMA: Schema =3D 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).un= wrap()) >> + .collect() >> + } >> +} >> + >=20 > IMHO that has to be easier: >=20 > instead of the custom type, i'd do: >=20 > const GROUP_FILTER_LIST_SCHEMA: Schema =3D ArraySchema::new("", > &GROUP_FILTER_SCHEMA).schema() >=20 > 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=20 > proxmox crate) >=20 > impl UpdaterType for Vec { > type Updater =3D Option; > } >=20 > 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.. >=20 > i tested it here, and AFAICS, this works (i can update/list the sync job=20 > properly) >=20 > with that you can also remove the 'filters' fn and can use > Vec as the api call parameters. >=20 > makes the code a little easier to read indeed it does :) only question is whether we want to enable this=20 updater behaviour in general for Vec (and if we ever need "add/remove=20 element(s)" semantics we do that via a custom type with custom Updater?) >=20 >> #[api( >> properties: { >> id: { >>=20 >=20 >=20 >=20