From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Dominik Csapak <d.csapak@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH v2 proxmox-backup 02/10] api: add GroupFilter(List) type
Date: Fri, 17 Sep 2021 08:39:43 +0200 [thread overview]
Message-ID: <1631860586.e1cb2tjf3e.astroid@nora.none> (raw)
In-Reply-To: <328852de-e06e-a356-84e4-c8e7795b085f@proxmox.com>
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@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: {
>>
>
>
>
next prev parent reply other threads:[~2021-09-17 6:40 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-15 13:41 [pve-devel] [PATCH v2 proxmox-backup 00/10] pull/sync group filter Fabian Grünbichler
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 01/10] api-types: add schema for backup group Fabian Grünbichler
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 02/10] api: add GroupFilter(List) type Fabian Grünbichler
2021-09-16 14:46 ` Dominik Csapak
2021-09-17 6:39 ` Fabian Grünbichler [this message]
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 03/10] BackupGroup: add filter helper Fabian Grünbichler
2021-09-16 14:46 ` Dominik Csapak
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 04/10] pull: use BackupGroup consistently Fabian Grünbichler
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 05/10] pull: allow pulling groups selectively Fabian Grünbichler
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering Fabian Grünbichler
2021-09-16 7:19 ` Thomas Lamprecht
2021-09-16 7:44 ` Fabian Grünbichler
2021-09-17 12:33 ` Dominik Csapak
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 07/10] remote: add backup group scanning Fabian Grünbichler
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 08/10] manager: extend sync/pull completion Fabian Grünbichler
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 09/10] manager: render group filter properly Fabian Grünbichler
2021-09-15 13:41 ` [pve-devel] [PATCH v2 proxmox-backup 10/10] docs: mention group filter in sync docs Fabian Grünbichler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1631860586.e1cb2tjf3e.astroid@nora.none \
--to=f.gruenbichler@proxmox.com \
--cc=d.csapak@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox