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 5E2D89A942 for ; Wed, 10 May 2023 13:48:20 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4716F27FB4 for ; Wed, 10 May 2023 13:48:20 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 10 May 2023 13:48:18 +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 7DB1647F90 for ; Wed, 10 May 2023 13:48:18 +0200 (CEST) Date: Wed, 10 May 2023 13:48:11 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230510081829.752401-1-d.csapak@proxmox.com> In-Reply-To: <20230510081829.752401-1-d.csapak@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1683718319.ft0ifv3hyf.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.075 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, sectionconfig.pm] Subject: Re: [pve-devel] [RFC PATCH common] section config: implement array support 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: Wed, 10 May 2023 11:48:20 -0000 On May 10, 2023 10:18 am, Dominik Csapak wrote: > enables section configs in the style of: >=20 > ---- > type: id > property value > property value2 > property value3 > ---- >=20 > can be combined with property strings >=20 > the provided createSchema just uses the name of the property but the > schema of the inner item >=20 > the api call is supposed to check if the overall entry of the section > config already exists and if yes, insert a new entry into the array >=20 > the updateSchema works very similar, but has the following addition: >=20 > if it's not a property string, there is an extra 'X-index' parameter > that indicates the index of the element to be updated (for property > strings, the api call has to figure out which entry to update, but there > we have most likely something resembling an id anyway) >=20 > for deletion, we don't provide a schema generator, but a good approach > would be to have optional parameters for the array id/index and only > deleting the relevant array entries then in PBS/Rust, an optional array (Vec) type gets a wholesale updater derived (SyncJobConfig has one, POM config as well). > also adds a test case for such array fields >=20 > Signed-off-by: Dominik Csapak > --- > another idea i had with the schemas was to extract the seperate options > of the property string into the api, but that was weird for the > following reasons: >=20 > * it's asymmetric if we don't automatically use print_property_string or > * we'd have to parse those directly with parse_property_string > (that would be a major change, since all users that use non-array > property strings don't expect that) > * or have it inconsistent between non-array and array property > strings... >=20 > this way we get a property string from the api and the config parsing, > and the config writer expects property strings, so easy to implement and > consistent, for the cost of having all api calls use parse/print > manually (which won't be that many hopefully) >=20 > src/PVE/SectionConfig.pm | 88 ++++++++++++++++++++++++++++++------- > test/section_config_test.pl | 26 +++++++++++ > 2 files changed, 98 insertions(+), 16 deletions(-) >=20 > diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm > index f36cede..baace66 100644 > --- a/src/PVE/SectionConfig.pm > +++ b/src/PVE/SectionConfig.pm > @@ -51,6 +51,17 @@ sub plugindata { > return {}; > } > =20 > +my $copy_property =3D sub { > + my ($src) =3D @_; > + > + my $res =3D {}; > + foreach my $k (keys %$src) { > + $res->{$k} =3D $src->{$k}; > + } > + > + return $res; > +}; > + > sub createSchema { > my ($class, $skip_type) =3D @_; > =20 > @@ -60,20 +71,15 @@ sub createSchema { > =20 > my $props =3D {}; > =20 > - my $copy_property =3D sub { > - my ($src) =3D @_; > - > - my $res =3D {}; > - foreach my $k (keys %$src) { > - $res->{$k} =3D $src->{$k}; > - } > - > - return $res; > - }; > - > foreach my $p (keys %$propertyList) { > next if $skip_type && $p eq 'type'; > =20 > + if ($propertyList->{$p}->{type} eq 'array') { > + $props->{$p} =3D $copy_property->($propertyList->{$p}->{items}); > + $props->{$p}->{optional} =3D $propertyList->{$p}->{optional} // 0; is this congruent with the rust SectionConfig schema? IIRC there were some extra checks there w.r.t. keys set both on the array and the item? > + next; > + } > + > if (!$propertyList->{$p}->{optional}) { > $props->{$p} =3D $propertyList->{$p}; > next; > @@ -124,6 +130,24 @@ sub updateSchema { > =20 > next if defined($filter_type) && !defined($copts->{$p}); > =20 > + if ($propertyList->{$p}->{type} eq 'array') { > + $props->{$p} =3D $copy_property->($propertyList->{$p}->{items}); > + $props->{$p}->{optional} =3D $propertyList->{$p}->{optional} // 0; > + > + # add index for all non property strings > + if (!defined($propertyList->{$p}->{items}->{format})) { > + $props->{$p}->{requires} =3D "$p-index"; > + $props->{"$p-index"} =3D { > + type =3D> 'integer', > + minimum =3D> 0, > + description =3D> "Determines which array element to update (0 is t= he first element).", > + optional =3D> $propertyList->{$p}->{optional} // 0, > + }, this is not how it's implemented in our Rust schema.. I guess we could make the index optional always and implement similar support on the rust side as well? no index -> wholesale replacement. index -> partial update? if we do partial updaters for arrays, it would also be nice to have them for property strings as well (although that is of course more work to implement, but it would have more potential for UX improvement) - a property string is just an object after all, so the key is the "index". > + } > + > + next; > + } > + > if (!$propertyList->{$p}->{optional}) { > $props->{$p} =3D $propertyList->{$p}; > next; > @@ -254,7 +278,15 @@ sub check_value { > =20 > if (!$skipSchemaCheck) { > my $errors =3D {}; > - PVE::JSONSchema::check_prop($value, $schema, '', $errors); > + > + my $checkschema =3D $schema; > + > + if ($ct eq 'array') { > + die "no item schema for array" if !defined($schema->{items}); > + $checkschema =3D $schema->{items}; > + } > + > + PVE::JSONSchema::check_prop($value, $checkschema, '', $errors); > if (scalar(keys %$errors)) { > die "$errors->{$key}\n" if $errors->{$key}; > die "$errors->{_root}\n" if $errors->{_root}; > @@ -311,6 +343,15 @@ sub parse_config { > } > }; > =20 > + my $is_array =3D sub { > + my ($type, $key) =3D @_; > + > + my $schema =3D $pdata->{propertyList}->{$key}; > + die "unknown property type\n" if !$schema; > + > + return $schema->{type} eq 'array'; > + }; > + > my $errors =3D []; > while (@lines) { > my $line =3D $nextline->(); > @@ -352,11 +393,19 @@ sub parse_config { > my ($k, $v) =3D ($1, $3); > =20 > eval { > - die "duplicate attribute\n" if defined($config->{$k}); > - if (!$unknown) { > - $v =3D $plugin->check_value($type, $k, $v, $sectionId); > + if ($is_array->($type, $k)) { > + if (!$unknown) { > + $v =3D $plugin->check_value($type, $k, $v, $sectionId); > + } > + $config->{$k} =3D [] if !defined($config->{$k}); > + push $config->{$k}->@*, $v; > + } else { > + die "duplicate attribute\n" if defined($config->{$k}); > + if (!$unknown) { > + $v =3D $plugin->check_value($type, $k, $v, $sectionId); > + } > + $config->{$k} =3D $v; > } > - $config->{$k} =3D $v; > }; > if (my $err =3D $@) { > warn "$errprefix (section '$sectionId') - unable to parse value of '$= k': $err"; > @@ -448,6 +497,13 @@ my $format_config_line =3D sub { > if ($ct eq 'boolean') { > return "\t$key " . ($value ? 1 : 0) . "\n" > if defined($value); > + } elsif ($ct eq 'array') { > + die "property '$key' is not an array" if ref($value) ne 'ARRAY'; > + my $result =3D ''; > + for my $line ($value->@*) { > + $result .=3D "\t$key $line\n" if $value ne ''; should this here encode the value of each item according to the item type? e.g., recursively call format_config_line again with the item schema? > + } > + return $result; > } else { > return "\t$key $value\n" if "$value" ne ''; > } > diff --git a/test/section_config_test.pl b/test/section_config_test.pl > index 22a9643..fc20e4c 100755 > --- a/test/section_config_test.pl > +++ b/test/section_config_test.pl > @@ -105,6 +105,25 @@ sub properties { > minimum =3D> 3, > maximum =3D> 9, > }, > + arrayfield =3D> { > + descritpion =3D> "Array Field with property string", > + type =3D> 'array', > + items =3D> { > + type =3D> 'string', > + descritpion =3D> 'a property string', nit: description spelled wrong here and below ;) > + format =3D> { > + subfield1 =3D> { > + type =3D> 'string', > + descritpion =3D> 'first subfield' > + }, > + subfield2 =3D> { > + type =3D> 'integer', > + minimum =3D> 0, > + optional =3D> 1, > + }, > + }, > + }, > + }, > }; > } > =20 > @@ -113,6 +132,7 @@ sub options { > common =3D> { optional =3D> 1 }, > field2 =3D> {}, > another =3D> {}, > + arrayfield =3D> { optional =3D> 1 }, > }; > } > =20 > @@ -190,6 +210,10 @@ my $with_unknown_data =3D { > type =3D> 'two', > field2 =3D> 5, > another =3D> 'even more text', > + arrayfield =3D> [ > + 'subfield1=3Dtest,subfield2=3D2', > + 'subfield1=3Dtest2', > + ], > }, > invalid =3D> { > type =3D> 'bad', > @@ -214,6 +238,8 @@ bad: invalid > two: t3 > field2 5 > another even more text > + arrayfield subfield1=3Dtest,subfield2=3D2 > + arrayfield subfield1=3Dtest2 > EOF > =20 > Conf->expect_fail('unknown-forbidden', $with_unknown_data, $with_unknown= _text); > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20