[pve-devel] [RFC PATCH common] section config: implement array support

[pve-devel] [RFC PATCH common] section config: implement array support

[Dominik Csapak]

enables section configs in the style of:

----
type: id
    property value
    property value2
    property value3
----

can be combined with property strings

the provided createSchema just uses the name of the property but the
schema of the inner item

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

the updateSchema works very similar, but has the following addition:

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)

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

also adds a test case for such array fields

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
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:

* 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...

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)

 src/PVE/SectionConfig.pm    | 88 ++++++++++++++++++++++++++++++-------
 test/section_config_test.pl | 26 +++++++++++
 2 files changed, 98 insertions(+), 16 deletions(-)

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 {};
 }
 
+my $copy_property = sub {
+    my ($src) = @_;
+
+    my $res = {};
+    foreach my $k (keys %$src) {
+	$res->{$k} = $src->{$k};
+    }
+
+    return $res;
+};
+
 sub createSchema {
     my ($class, $skip_type) = @_;
 
@@ -60,20 +71,15 @@ sub createSchema {
 
     my $props = {};
 
-    my $copy_property = sub {
-	my ($src) = @_;
-
-	my $res = {};
-	foreach my $k (keys %$src) {
-	    $res->{$k} = $src->{$k};
-	}
-
-	return $res;
-    };
-
     foreach my $p (keys %$propertyList) {
 	next if $skip_type && $p eq 'type';
 
+	if ($propertyList->{$p}->{type} eq 'array') {
+	    $props->{$p} = $copy_property->($propertyList->{$p}->{items});
+	    $props->{$p}->{optional} = $propertyList->{$p}->{optional} // 0;
+	    next;
+	}
+
 	if (!$propertyList->{$p}->{optional}) {
 	    $props->{$p} = $propertyList->{$p};
 	    next;
@@ -124,6 +130,24 @@ sub updateSchema {
 
 	next if defined($filter_type) && !defined($copts->{$p});
 
+	if ($propertyList->{$p}->{type} eq 'array') {
+	    $props->{$p} = $copy_property->($propertyList->{$p}->{items});
+	    $props->{$p}->{optional} = $propertyList->{$p}->{optional} // 0;
+
+	    # add index for all non property strings
+	    if (!defined($propertyList->{$p}->{items}->{format})) {
+		$props->{$p}->{requires} = "$p-index";
+		$props->{"$p-index"} = {
+		    type => 'integer',
+		    minimum => 0,
+		    description => "Determines which array element to update (0 is the first element).",
+		    optional => $propertyList->{$p}->{optional} // 0,
+		},
+	    }
+
+	    next;
+	}
+
 	if (!$propertyList->{$p}->{optional}) {
 	    $props->{$p} = $propertyList->{$p};
 	    next;
@@ -254,7 +278,15 @@ sub check_value {
 
     if (!$skipSchemaCheck) {
 	my $errors = {};
-	PVE::JSONSchema::check_prop($value, $schema, '', $errors);
+
+	my $checkschema = $schema;
+
+	if ($ct eq 'array') {
+	    die "no item schema for array" if !defined($schema->{items});
+	    $checkschema = $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 {
 	}
     };
 
+    my $is_array = sub {
+	my ($type, $key) = @_;
+
+	my $schema = $pdata->{propertyList}->{$key};
+	die "unknown property type\n" if !$schema;
+
+	return $schema->{type} eq 'array';
+    };
+
     my $errors = [];
     while (@lines) {
 	my $line = $nextline->();
@@ -352,11 +393,19 @@ sub parse_config {
 		    my ($k, $v) = ($1, $3);
 
 		    eval {
-			die "duplicate attribute\n" if defined($config->{$k});
-			if (!$unknown) {
-			    $v = $plugin->check_value($type, $k, $v, $sectionId);
+			if ($is_array->($type, $k)) {
+			    if (!$unknown) {
+				$v = $plugin->check_value($type, $k, $v, $sectionId);
+			    }
+			    $config->{$k} = [] if !defined($config->{$k});
+			    push $config->{$k}->@*, $v;
+			} else {
+			    die "duplicate attribute\n" if defined($config->{$k});
+			    if (!$unknown) {
+				$v = $plugin->check_value($type, $k, $v, $sectionId);
+			    }
+			    $config->{$k} = $v;
 			}
-			$config->{$k} = $v;
 		    };
 		    if (my $err = $@) {
 			warn "$errprefix (section '$sectionId') - unable to parse value of '$k': $err";
@@ -448,6 +497,13 @@ my $format_config_line = 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 = '';
+	for my $line ($value->@*) {
+	    $result .= "\t$key $line\n" if $value ne '';
+	}
+	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 => 3,
 	    maximum => 9,
 	},
+	arrayfield => {
+	    descritpion => "Array Field with property string",
+	    type => 'array',
+	    items => {
+		type => 'string',
+		descritpion => 'a property string',
+		format => {
+		    subfield1 => {
+			type => 'string',
+			descritpion => 'first subfield'
+		    },
+		    subfield2 => {
+			type => 'integer',
+			minimum => 0,
+			optional => 1,
+		    },
+		},
+	    },
+	},
     };
 }
 
@@ -113,6 +132,7 @@ sub options {
 	common => { optional => 1 },
 	field2 => {},
 	another => {},
+	arrayfield => { optional => 1 },
     };
 }
 
@@ -190,6 +210,10 @@ my $with_unknown_data = {
 	    type => 'two',
 	    field2 => 5,
 	    another => 'even more text',
+	    arrayfield => [
+		'subfield1=test,subfield2=2',
+		'subfield1=test2',
+	    ],
 	},
 	invalid => {
 	    type => 'bad',
@@ -214,6 +238,8 @@ bad: invalid
 two: t3
 	field2 5
 	another even more text
+	arrayfield subfield1=test,subfield2=2
+	arrayfield subfield1=test2
 EOF
 
 Conf->expect_fail('unknown-forbidden', $with_unknown_data, $with_unknown_text);
-- 
2.30.2

Re: [pve-devel] [RFC PATCH common] section config: implement array support

[Fabian Grünbichler]

On May 10, 2023 10:18 am, Dominik Csapak wrote:
> enables section configs in the style of:
> 
> ----
> type: id
>     property value
>     property value2
>     property value3
> ----
> 
> can be combined with property strings
> 
> the provided createSchema just uses the name of the property but the
> schema of the inner item
> 
> 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
> 
> the updateSchema works very similar, but has the following addition:
> 
> 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)
> 
> 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
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> 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:
> 
> * 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...
> 
> 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)
> 
>  src/PVE/SectionConfig.pm    | 88 ++++++++++++++++++++++++++++++-------
>  test/section_config_test.pl | 26 +++++++++++
>  2 files changed, 98 insertions(+), 16 deletions(-)
> 
> 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 {};
>  }
>  
> +my $copy_property = sub {
> +    my ($src) = @_;
> +
> +    my $res = {};
> +    foreach my $k (keys %$src) {
> +	$res->{$k} = $src->{$k};
> +    }
> +
> +    return $res;
> +};
> +
>  sub createSchema {
>      my ($class, $skip_type) = @_;
>  
> @@ -60,20 +71,15 @@ sub createSchema {
>  
>      my $props = {};
>  
> -    my $copy_property = sub {
> -	my ($src) = @_;
> -
> -	my $res = {};
> -	foreach my $k (keys %$src) {
> -	    $res->{$k} = $src->{$k};
> -	}
> -
> -	return $res;
> -    };
> -
>      foreach my $p (keys %$propertyList) {
>  	next if $skip_type && $p eq 'type';
>  
> +	if ($propertyList->{$p}->{type} eq 'array') {
> +	    $props->{$p} = $copy_property->($propertyList->{$p}->{items});
> +	    $props->{$p}->{optional} = $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} = $propertyList->{$p};
>  	    next;
> @@ -124,6 +130,24 @@ sub updateSchema {
>  
>  	next if defined($filter_type) && !defined($copts->{$p});
>  
> +	if ($propertyList->{$p}->{type} eq 'array') {
> +	    $props->{$p} = $copy_property->($propertyList->{$p}->{items});
> +	    $props->{$p}->{optional} = $propertyList->{$p}->{optional} // 0;
> +
> +	    # add index for all non property strings
> +	    if (!defined($propertyList->{$p}->{items}->{format})) {
> +		$props->{$p}->{requires} = "$p-index";
> +		$props->{"$p-index"} = {
> +		    type => 'integer',
> +		    minimum => 0,
> +		    description => "Determines which array element to update (0 is the first element).",
> +		    optional => $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} = $propertyList->{$p};
>  	    next;
> @@ -254,7 +278,15 @@ sub check_value {
>  
>      if (!$skipSchemaCheck) {
>  	my $errors = {};
> -	PVE::JSONSchema::check_prop($value, $schema, '', $errors);
> +
> +	my $checkschema = $schema;
> +
> +	if ($ct eq 'array') {
> +	    die "no item schema for array" if !defined($schema->{items});
> +	    $checkschema = $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 {
>  	}
>      };
>  
> +    my $is_array = sub {
> +	my ($type, $key) = @_;
> +
> +	my $schema = $pdata->{propertyList}->{$key};
> +	die "unknown property type\n" if !$schema;
> +
> +	return $schema->{type} eq 'array';
> +    };
> +
>      my $errors = [];
>      while (@lines) {
>  	my $line = $nextline->();
> @@ -352,11 +393,19 @@ sub parse_config {
>  		    my ($k, $v) = ($1, $3);
>  
>  		    eval {
> -			die "duplicate attribute\n" if defined($config->{$k});
> -			if (!$unknown) {
> -			    $v = $plugin->check_value($type, $k, $v, $sectionId);
> +			if ($is_array->($type, $k)) {
> +			    if (!$unknown) {
> +				$v = $plugin->check_value($type, $k, $v, $sectionId);
> +			    }
> +			    $config->{$k} = [] if !defined($config->{$k});
> +			    push $config->{$k}->@*, $v;
> +			} else {
> +			    die "duplicate attribute\n" if defined($config->{$k});
> +			    if (!$unknown) {
> +				$v = $plugin->check_value($type, $k, $v, $sectionId);
> +			    }
> +			    $config->{$k} = $v;
>  			}
> -			$config->{$k} = $v;
>  		    };
>  		    if (my $err = $@) {
>  			warn "$errprefix (section '$sectionId') - unable to parse value of '$k': $err";
> @@ -448,6 +497,13 @@ my $format_config_line = 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 = '';
> +	for my $line ($value->@*) {
> +	    $result .= "\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 => 3,
>  	    maximum => 9,
>  	},
> +	arrayfield => {
> +	    descritpion => "Array Field with property string",
> +	    type => 'array',
> +	    items => {
> +		type => 'string',
> +		descritpion => 'a property string',

nit: description spelled wrong here and below ;)

> +		format => {
> +		    subfield1 => {
> +			type => 'string',
> +			descritpion => 'first subfield'
> +		    },
> +		    subfield2 => {
> +			type => 'integer',
> +			minimum => 0,
> +			optional => 1,
> +		    },
> +		},
> +	    },
> +	},
>      };
>  }
>  
> @@ -113,6 +132,7 @@ sub options {
>  	common => { optional => 1 },
>  	field2 => {},
>  	another => {},
> +	arrayfield => { optional => 1 },
>      };
>  }
>  
> @@ -190,6 +210,10 @@ my $with_unknown_data = {
>  	    type => 'two',
>  	    field2 => 5,
>  	    another => 'even more text',
> +	    arrayfield => [
> +		'subfield1=test,subfield2=2',
> +		'subfield1=test2',
> +	    ],
>  	},
>  	invalid => {
>  	    type => 'bad',
> @@ -214,6 +238,8 @@ bad: invalid
>  two: t3
>  	field2 5
>  	another even more text
> +	arrayfield subfield1=test,subfield2=2
> +	arrayfield subfield1=test2
>  EOF
>  
>  Conf->expect_fail('unknown-forbidden', $with_unknown_data, $with_unknown_text);
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [RFC PATCH common] section config: implement array support

[Wolfgang Bumiller]

On Wed, May 10, 2023 at 01:48:11PM +0200, Fabian Grünbichler wrote:
> On May 10, 2023 10:18 am, Dominik Csapak wrote:
> > @@ -124,6 +130,24 @@ sub updateSchema {
> >  
> >  	next if defined($filter_type) && !defined($copts->{$p});
> >  
> > +	if ($propertyList->{$p}->{type} eq 'array') {
> > +	    $props->{$p} = $copy_property->($propertyList->{$p}->{items});
> > +	    $props->{$p}->{optional} = $propertyList->{$p}->{optional} // 0;
> > +
> > +	    # add index for all non property strings
> > +	    if (!defined($propertyList->{$p}->{items}->{format})) {
> > +		$props->{$p}->{requires} = "$p-index";
> > +		$props->{"$p-index"} = {
> > +		    type => 'integer',
> > +		    minimum => 0,
> > +		    description => "Determines which array element to update (0 is the first element).",
> > +		    optional => $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".

I'm not convinced this update schema should be there at all.

1)
I think the only place this really affects is the CLI.
Sure, accessing an individual element might "feel nicer" when writing
the UI in *some* ways, but really, the UI usually already has the whole
array already and can just include it. (Plus, these are never actually
that big!)
On the CLI, however, we have to actually type out these parameters, and
they feel a *lot* bigger there.
And how would I:
- add one or multiple values
- insert one or multiple values
- update multiple values
- delete multiple values
The startup times of those perl binaries is already huge, if I have to
reload the whole API for every single element in an array I want to
change, that's quite... bad.

2)
We already have --netX, --mpX, ... which aren't arrays, and if we start
generating schemas like this, it might be worth considering whether we'd
ever want to change that in the future?

3)
Sometimes I kind of wish we'd just use 'json paths' for the keys...
Or, well, generate `--foo.bar` update-schemas for property strings and
introduce support for `patternProperties` (see 10.3.2.2. in the json
schema core spec here[1]), for arrays to support `--foo[3]=bar`, but
generating these things doesn't actually scale too great, eg. think of
an array of property strings...

I mean... It would be actually nice if I could do
`--net0.link-down=true` instead of copying the entire network string... 

[1] https://json-schema.org/draft/2020-12/json-schema-core.html#section-10.3.2.2

Re: [pve-devel] [RFC PATCH common] section config: implement array support

[Dominik Csapak]

thanks for your feedback @fabian, @wolfgang!

so the consensus seems to be to simply expose the array in the api schema and
always have the client send the whole array over, like in pbs updater
(not a problem for my series, since in the gui we have the whole info anyway,
also if one want a custom api can always be created instead of using the
create/updateSchema methods)

I'd adapt my patch, and enable arrays in the pve-http-server instead of our
'-alist' format (which we only ever use in two places AFAICS) and replace those
by an array type

(there are a few things that must change in JSONSchema/CLIHandler to fully support arrays,
but that's only minor things, such as doing the untainting correctly)

i'd then remove support for the '-alist' format completely since it'll not
work anymore (at least in the api). FWICT this isn't even a real api
change, since the client would send the data in exactly the same way as
before, but we'll send the parameters along as arrays instead of \0-separated strings

Any other opinions @Thomas?

does that work for everybody?