[pve-devel] [PATCH common/widget-toolkit v2] implement oneOf schema

[pve-devel] [PATCH common/widget-toolkit v2] implement oneOf schema

[Dominik Csapak]

this series implementes the oneOf schema for the api, see the individual
patches for more details and changelog

pve-common:

Dominik Csapak (4):
  section config: add test for the schemas
  json schema: implement 'oneOf' schema
  section config: allow separated property lists for plugins
  section config: add tests for separated property lists

 src/PVE/CLIHandler.pm                 |   2 +-
 src/PVE/JSONSchema.pm                 | 116 +++++-
 src/PVE/RESTHandler.pm                |  82 ++++-
 src/PVE/SectionConfig.pm              | 259 +++++++++++---
 src/PVE/Tools.pm                      |  33 ++
 test/Makefile                         |   1 +
 test/section_config_separated_test.pl | 489 ++++++++++++++++++++++++++
 test/section_config_test.pl           | 133 +++++++
 8 files changed, 1045 insertions(+), 70 deletions(-)
 create mode 100755 test/section_config_separated_test.pl

proxmox-widget-toolkit:

Dominik Csapak (1):
  api-viewer: implement basic oneOf support

 src/api-viewer/APIViewer.js | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH common v2 1/4] section config: add test for the schemas

[Dominik Csapak]

by simply doing an 'is_deeply' on the generated schema with
the current generated schema

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes
 test/section_config_test.pl | 133 ++++++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)

diff --git a/test/section_config_test.pl b/test/section_config_test.pl
index 8117d76..343e4c8 100755
--- a/test/section_config_test.pl
+++ b/test/section_config_test.pl
@@ -142,6 +142,7 @@ use strict;
 use warnings;
 
 use Test::More;
+use PVE::JSONSchema;
 
 Conf::One->register();
 Conf::Two->register();
@@ -250,6 +251,138 @@ EOF
 Conf->expect_fail('unknown-forbidden', $with_unknown_data, $with_unknown_text);
 Conf->expect_success('unknown-allowed', $with_unknown_data, $with_unknown_text, 1);
 
+# schema tests
+my $create_schema = Conf->createSchema();
+my $expected_create_schema = {
+    additionalProperties =>  0,
+    type => 'object',
+    properties =>  {
+	id => {
+	    description => 'ID',
+	    format => 'pve-configid',
+	    maxLength => 64,
+	    type => 'string',
+	},
+	type =>  {
+	    description => 'Section type.',
+	    enum => ['one', 'two'],
+	    type => 'string',
+	},
+	common => {
+	    type => 'string',
+	    description => 'common value',
+	    maxLength => 512,
+	},
+	field1 =>  {
+	    description =>  'Field One',
+	    maximum =>  9,
+	    minimum =>  3,
+	    optional =>  1,
+	    type =>  'integer',
+
+	},
+	'field2'=> {
+	    'description'=> 'Field Two',
+	    'maximum'=> 9,
+	    'minimum'=> 3,
+	    'optional'=> 1,
+	    'type'=> 'integer',
+	},
+	'arrayfield'=> {
+	    'description'=> 'Array Field with property string',
+	    'items'=> {
+		'description'=> 'a property string',
+		'format'=> {
+		    'subfield2'=> {
+			'optional'=> 1,
+			'type'=> 'integer',
+			'minimum'=> 0
+		    },
+		    'subfield1'=> {
+			'description'=> 'first subfield',
+			'type'=> 'string',
+		    },
+		},
+		'type'=> 'string'
+	    },
+	    'optional'=> 1,
+	    'type'=> 'array',
+	},
+	'another'=> {
+	    'description'=> 'Another field',
+	    'optional'=> 1,
+	    'type'=> 'string',
+	},
+    },
+};
+
+is_deeply($create_schema, $expected_create_schema, "create schema test");
+
+my $update_schema = Conf->updateSchema();
+my $expected_update_schema = {
+    additionalProperties => 0,
+    type => 'object',
+    properties => {
+	id => {
+	    description => 'ID',
+	    format => 'pve-configid',
+	    maxLength => 64,
+	    type => 'string',
+	},
+	delete => {
+	    type => 'string', format => 'pve-configid-list',
+	    description => "A list of settings you want to delete.",
+	    maxLength => 4096,
+	    optional => 1,
+	},
+	digest => PVE::JSONSchema::get_standard_option('pve-config-digest'),
+	common => {
+	    description => 'common value',
+	    maxLength => 512,
+	    type => 'string',
+	},
+	field1 => {
+	    description => 'Field One',
+	    maximum => 9,
+	    minimum => 3,
+	    optional => 1,
+	    type => 'integer'
+	},
+	field2 => {
+	    description => 'Field Two',
+	    maximum => 9,
+	    minimum => 3,
+	    optional => 1,
+	    type => 'integer',
+	},
+	arrayfield => {
+	    description => 'Array Field with property string',
+	    items => {
+		type => 'string',
+		description => 'a property string',
+		format => {
+		    subfield2 => {
+			type => 'integer',
+			minimum => 0,
+			optional => 1
+		    },
+		    subfield1 => {
+			description => 'first subfield',
+			type => 'string'
+		    }
+		}
+	    },
+	    optional => 1,
+	    type => 'array',
+	},
+	another => {
+	    description => 'Another field',
+	    optional => 1,
+	    type => 'string',
+	},
+    },
+};
+is_deeply($update_schema, $expected_update_schema, "update schema test");
 
 done_testing();
 
-- 
2.30.2

[pve-devel] [PATCH common v2 2/4] json schema: implement 'oneOf' schema

[Dominik Csapak]

a schema can now have the 'oneOf' property which is an array of regular
schemas. In the default case any of that has to match. If the
'type-property'/'instance-types' are given, only the schema for the specific
type will be checked (and handles as 'additionalProperties' if there is
no matching type)

the field found in 'type-property' has to be on the same level
(so for oneOf the nested schemas should not include that).

Documentation is adapted so that options are grouped per `type-property=value`
after the regular options (with their individual descriptions/types/etc.)

oneOfs without 'type-property'/'instance-tyeps' simply show up twice for
now with an 'or' line in between.

command line parsing is a bit weird for now since Getopt::Long
can't have multiple variants for the same property (but works fine with
pvesh for our current use cases). it gets shown as '--foo <multiple' if
they are not optional.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* removed default for 'type-property'
* use shorter code for checking inner_errors length

 src/PVE/CLIHandler.pm  |   2 +-
 src/PVE/JSONSchema.pm  | 116 ++++++++++++++++++++++++++++++++++++++---
 src/PVE/RESTHandler.pm |  82 +++++++++++++++++++++++++++--
 3 files changed, 187 insertions(+), 13 deletions(-)

diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
index 5c7863a..bb97a7d 100644
--- a/src/PVE/CLIHandler.pm
+++ b/src/PVE/CLIHandler.pm
@@ -433,7 +433,7 @@ my $print_bash_completion = sub {
 		my $res = $d->{completion}->($cmd, $pname, $cur, $args);
 		&$print_result(@$res);
 	    }
-	} elsif ($d->{type} eq 'boolean') {
+	} elsif ($d->{type} && $d->{type} eq 'boolean') {
 	    &$print_result('0', '1');
 	} elsif ($d->{enum}) {
 	    &$print_result(@{$d->{enum}});
diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 49e0d7a..6e1cd11 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -1087,6 +1087,16 @@ sub check_type {
     return undef;
 }
 
+my sub get_instance_type {
+    my ($schema, $key, $value) = @_;
+
+    if (my $type_property = $schema->{$key}->{'type-property'}) {
+	return $value->{$type_property};
+    }
+
+    return undef;
+}
+
 sub check_object {
     my ($path, $schema, $value, $additional_properties, $errors) = @_;
 
@@ -1105,7 +1115,8 @@ sub check_object {
     }
 
     foreach my $k (keys %$schema) {
-	check_prop($value->{$k}, $schema->{$k}, $path ? "$path.$k" : $k, $errors);
+	my $instance_type = get_instance_type($schema, $k, $value);
+	check_prop($value->{$k}, $schema->{$k}, $path ? "$path.$k" : $k, $errors, $instance_type);
     }
 
     foreach my $k (keys %$value) {
@@ -1123,7 +1134,23 @@ sub check_object {
 		}
 	    }
 
-	    next; # value is already checked above
+	    # if it's a oneOf, check if there is a matching type
+	    my $matched_type = 1;
+	    if ($subschema->{oneOf}) {
+		my $instance_type = get_instance_type($schema, $k, $value);
+		$matched_type = 0;
+		for my $alternative ($subschema->{oneOf}->@*) {
+		    if (my $instance_types = $alternative->{'instance-types'}) {
+			if (!grep { $instance_type eq $_ } $instance_types->@*) {
+			    next;
+			}
+		    }
+		    $matched_type = 1;
+		    last;
+		}
+	    }
+
+	    next if $matched_type; # value is already checked above
 	}
 
 	if (defined ($additional_properties) && !$additional_properties) {
@@ -1150,7 +1177,7 @@ sub check_object_warn {
 }
 
 sub check_prop {
-    my ($value, $schema, $path, $errors) = @_;
+    my ($value, $schema, $path, $errors, $instance_type) = @_;
 
     die "internal error - no schema" if !$schema;
     die "internal error" if !$errors;
@@ -1163,6 +1190,58 @@ sub check_prop {
 	return;
     }
 
+    # must pass any of the given schemas
+    my $optional_for_type = 0;
+    if ($schema->{oneOf}) {
+	# in case we have an instance_type given, just check for that variant
+	if ($schema->{'type-property'}) {
+	    $optional_for_type = 1;
+	    for (my $i = 0; $i < scalar($schema->{oneOf}->@*); $i++) {
+		last if !$instance_type; # treat as optional if we don't have a type
+		my $inner_schema = $schema->{oneOf}->[$i];
+
+		if (!defined($inner_schema->{'instance-types'})) {
+		    add_error($errors, $path, "missing 'instance-types' in oneOf alternative");
+		    return;
+		}
+
+		next if !grep { $_ eq $instance_type } $inner_schema->{'instance-types'}->@*;
+		$optional_for_type = $inner_schema->{optional} // 0;
+		check_prop($value, $inner_schema, $path, $errors);
+	    }
+	} else {
+	    my $is_valid = 0;
+	    my $collected_errors = {};
+	    for (my $i = 0; $i < scalar($schema->{oneOf}->@*); $i++) {
+		my $inner_schema = $schema->{oneOf}->[$i];
+		my $inner_errors = {};
+		check_prop($value, $inner_schema, "$path.oneOf[$i]", $inner_errors);
+		if (!$inner_errors->%*) {
+		    $is_valid = 1;
+		    last;
+		}
+
+		for my $inner_path (keys $inner_errors->%*) {
+		    add_error($collected_errors, $inner_path, $inner_errors->{$path});
+		}
+	    }
+
+	    if (!$is_valid) {
+		for my $inner_path (keys $collected_errors->%*) {
+		    add_error($errors, $inner_path, $collected_errors->{$path});
+		}
+	    }
+	}
+    } elsif ($instance_type) {
+	if (!defined($schema->{'instance-types'})) {
+	    add_error($errors, $path, "missing 'instance-types'");
+	    return;
+	}
+	if (grep { $_ eq $instance_type} $schema->{'instance_types'}->@*) {
+	    $optional_for_type = 1;
+	}
+    }
+
     # if it extends another schema, it must pass that schema as well
     if($schema->{extends}) {
 	check_prop($value, $schema->{extends}, $path, $errors);
@@ -1170,7 +1249,7 @@ sub check_prop {
 
     if (!defined ($value)) {
 	return if $schema->{type} && $schema->{type} eq 'null';
-	if (!$schema->{optional} && !$schema->{alias} && !$schema->{group}) {
+	if (!$schema->{optional} && !$schema->{alias} && !$schema->{group} && !$optional_for_type) {
 	    add_error($errors, $path, "property is missing and it is not optional");
 	}
 	return;
@@ -1317,6 +1396,28 @@ my $default_schema_noref = {
 	    },
 	    enum => $schema_valid_types,
 	},
+	oneOf => {
+	    type => 'array',
+	    description => "This represents the alternative options for this Schema instance.",
+	    optional => 1,
+	    items => {
+		type => 'object',
+		description => "A valid option of the properties",
+	    },
+	},
+	'instance-types' => {
+	    type => 'array',
+	    description => "Indicate to which type the parameter (or variant if inside a oneOf) belongs.",
+	    optional => 1,
+	    items => {
+		type => 'string',
+	    },
+	},
+	'type-property' => {
+	    type => 'string',
+	    description => "The property to check for instance types.",
+	    optional => 1,
+	},
 	optional => {
 	    type => "boolean",
 	    description => "This indicates that the instance property in the instance object is not required.",
@@ -1491,6 +1592,7 @@ my $default_schema = Storable::dclone($default_schema_noref);
 
 $default_schema->{properties}->{properties}->{additionalProperties} = $default_schema;
 $default_schema->{properties}->{additionalProperties}->{properties} = $default_schema->{properties};
+$default_schema->{properties}->{oneOf}->{items}->{properties} = $default_schema->{properties};
 
 $default_schema->{properties}->{items}->{properties} = $default_schema->{properties};
 $default_schema->{properties}->{items}->{additionalProperties} = 0;
@@ -1713,12 +1815,12 @@ sub get_options {
 	    # optional and call the mapping function afterwards.
 	    push @getopt, "$prop:s";
 	    push @interactive, [$prop, $mapping->{func}];
-	} elsif ($pd->{type} eq 'boolean') {
+	} elsif ($pd->{type} && $pd->{type} eq 'boolean') {
 	    push @getopt, "$prop:s";
 	} else {
 	    if ($pd->{format} && $pd->{format} =~ m/-list/) {
 		push @getopt, "$prop=s@";
-	    } elsif ($pd->{type} eq 'array') {
+	    } elsif ($pd->{type} && $pd->{type} eq 'array') {
 		push @getopt, "$prop=s@";
 	    } else {
 		push @getopt, "$prop=s";
@@ -1807,7 +1909,7 @@ sub get_options {
 
     foreach my $p (keys %$opts) {
 	if (my $pd = $schema->{properties}->{$p}) {
-	    if ($pd->{type} eq 'boolean') {
+	    if ($pd->{type} && $pd->{type} eq 'boolean') {
 		if ($opts->{$p} eq '') {
 		    $opts->{$p} = 1;
 		} elsif (defined(my $bool = parse_boolean($opts->{$p}))) {
diff --git a/src/PVE/RESTHandler.pm b/src/PVE/RESTHandler.pm
index b99eb15..7bf6b74 100644
--- a/src/PVE/RESTHandler.pm
+++ b/src/PVE/RESTHandler.pm
@@ -725,12 +725,19 @@ sub getopt_usage {
     my $idx_param = {}; # -vlan\d+ -scsi\d+
 
     my $opts = '';
+
+    my $type_specific_opts = {};
+
     foreach my $k (sort keys %$prop) {
 	next if $arg_hash->{$k};
 	next if defined($fixed_param->{$k});
 
 	my $type_text = $prop->{$k}->{type} || 'string';
 
+	if ($prop->{$k}->{oneOf}) {
+	    $type_text = 'multiple';
+	}
+
 	my $param_map = {};
 
 	if (defined($param_cb)) {
@@ -749,10 +756,51 @@ sub getopt_usage {
 	    }
 	}
 
+	my $is_optional = $prop->{$k}->{optional} // 0;
+
+	if (my $type_property = $prop->{$k}->{'type-property'}) {
+	    # save type specific descriptions for later
+	    my $type_schema = $prop->{$type_property};
+	    if ($prop->{$k}->{oneOf}) {
+		# it's optional if there are less options than types
+		$is_optional = 1 if scalar($type_schema->{enum}->@*) > scalar($prop->{$k}->{oneOf}->@*);
+		for my $alternative ($prop->{$k}->{oneOf}->@*) {
+		    # it's optional if at least one variant is optional
+		    $is_optional = 1 if $alternative->{optional};
+		    for my $type ($alternative->{'instance-types'}->@*) {
+			my $key = "${type_property}=${type}";
+			$type_specific_opts->{$key} //= "";
+			$type_specific_opts->{$key}
+			    .= $get_property_description->($base, 'arg', $alternative, $format, $param_map->{$k});
+		    }
+		}
+	    } elsif (my $types = $prop->{$k}->{'instance-types'}) {
+		# it's optional if not all types has that option
+		$is_optional = 1 if scalar($type_schema->{enum}->@*) > scalar($types->@*);
+		for my $type ($types->@*) {
+		    my $key = "${type_property}=${type}";
+		    $type_specific_opts->{$key} //= "";
+		    $type_specific_opts->{$key}
+			.= $get_property_description->($base, 'arg', $prop->{$k}, $format, $param_map->{$k});
+		}
+	    }
+	} elsif ($prop->{$k}->{oneOf}) {
+	    my $res = [];
+	    for my $alternative ($prop->{$k}->{oneOf}->@*) {
+		# it's optional if at least one variant is optional
+		$is_optional = 1 if $alternative->{optional};
+		push $res->@*, $get_property_description->($base, 'arg', $alternative, $format, $param_map->{$k});
+	    }
+	    if ($format eq 'asciidoc') {
+		$opts .= join("\n\nor\n\n", $res->@*);
+	    } else {
+		$opts .= join("  or\n\n", $res->@*);
+	    }
+	} else {
+	    $opts .= $get_property_description->($base, 'arg', $prop->{$k}, $format, $param_map->{$k});
+	}
 
-	$opts .= $get_property_description->($base, 'arg', $prop->{$k}, $format, $param_map->{$k});
-
-	if (!$prop->{$k}->{optional}) {
+	if (!$is_optional) {
 	    $args .= " " if $args;
 	    $args .= "--$base <$type_text>"
 	}
@@ -780,7 +828,7 @@ sub getopt_usage {
 	    $out .= "\n$desc\n\n";
 	} elsif ($format eq 'full') {
 	    my $desc = Text::Wrap::wrap('  ', '  ', ($info->{description}));
-	    $out .= "\n$desc\n";
+	    $out .= "\n$desc\n\n";
 	}
     }
 
@@ -788,6 +836,23 @@ sub getopt_usage {
 
     $out .= $opts if $opts;
 
+    if (scalar(keys $type_specific_opts->%*)) {
+	if ($format eq 'asciidoc') {
+	    $out .= "\n\n\n`Conditional options:`\n\n";
+	} else {
+	    $out .= " Conditional options:\n\n";
+	}
+    }
+
+    for my $type_opts (sort keys $type_specific_opts->%*) {
+	if ($format eq 'asciidoc') {
+	    $out .= "`[$type_opts]` ;;\n\n";
+	} else {
+	    $out .= " [$type_opts]\n\n";
+	}
+	$out .= $type_specific_opts->{$type_opts};
+    }
+
     return $out;
 }
 
@@ -825,7 +890,14 @@ sub dump_properties {
 	    }
 	}
 
-	$raw .= $get_property_description->($base, $style, $phash, $format);
+	if ($phash->{oneOf}) {
+	    for my $alternative ($phash->{oneOf}->@*) {
+		$raw .= $get_property_description->($base, $style, $alternative, $format);
+	    }
+	} else {
+	    $raw .= $get_property_description->($base, $style, $phash, $format);
+	}
+
 
 	next if $style ne 'config';
 
-- 
2.30.2

[pve-devel] [PATCH common v2 3/4] section config: allow separated property lists for plugins

[Dominik Csapak]

when using 'init(1)'. This saves the property lists per type instead of
a big one, and using create/updateSchema creates a new schema with the
options as 'oneOf' and/or 'instance-types' (depending if the schemas
match).

with that, we change how we work with the options hash:

it's not needed anymore to specify options that are specified in the
type specific propertyList, except if it's 'fixed => 1' (since that does
not exist in the schema)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* don't use the __global hack anymore, we now construct the
  plugin-specific options hash in `init`
* all 'global' properties are added in the api as they are
* improved the compare function and moved to PVE::Tools as a generic
  recursive variable compare
* removed copy_property
* optimized the genrated schema: when a property is used by all types,
  the 'type-property' and 'instance-types' is removed, this makes the
  docs a bit shorter when many of such options exist
* added an is_separated call

 src/PVE/SectionConfig.pm | 259 ++++++++++++++++++++++++++++++---------
 src/PVE/Tools.pm         |  33 +++++
 2 files changed, 235 insertions(+), 57 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 5690476..eb39b41 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -8,6 +8,7 @@ use Digest::SHA;
 
 use PVE::Exception qw(raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::Tools qw(compare_deeply);
 
 my $defaultData = {
     options => {},
@@ -51,6 +52,61 @@ sub plugindata {
     return {};
 }
 
+sub is_separated {
+    my ($class) = @_;
+
+    return $class->private()->{propertyListSeparated}->%* > 0;
+}
+
+my sub cmp_property {
+    my ($a, $b, $skip_opts) = @_;
+
+    my $merged = {$a->%*, $b->%*};
+    for my $opt ($skip_opts->@*) {
+	delete $merged->{$opt};
+    }
+    for my $opt (keys $merged->%*) {
+	return 0 if !compare_deeply($a->{$opt}, $b->{$opt});
+    }
+
+    return 1;
+};
+
+my sub add_property {
+    my ($props, $key, $prop, $type) = @_;
+
+    if (!defined($props->{$key})) {
+	$props->{$key} = $prop;
+	return;
+    }
+
+    if (!defined($props->{$key}->{oneOf})) {
+	if (cmp_property($props->{$key}, $prop, ['instance-types'])) {
+	    push $props->{$key}->{'instance-types'}->@*, $type;
+	} else {
+	    my $new_prop = delete $props->{$key};
+	    delete $new_prop->{'type-property'};
+	    delete $prop->{'type-property'};
+	    $props->{$key} = {
+		'type-property' => 'type',
+		oneOf => [
+		    $new_prop,
+		    $prop,
+		],
+	    };
+	}
+    } else {
+	for my $existing_prop ($props->{$key}->{oneOf}->@*) {
+	    if (cmp_property($existing_prop, $prop, ['instance-types', 'type-property'])) {
+		push $existing_prop->{'instance-types'}->@*, $type;
+		return;
+	    }
+	}
+
+	push $props->{$key}->{oneOf}->@*, $prop;
+    }
+};
+
 sub createSchema {
     my ($class, $skip_type, $base) = @_;
 
@@ -60,42 +116,61 @@ sub createSchema {
 
     my $props = $base || {};
 
-    my $copy_property = sub {
-	my ($src) = @_;
-
-	my $res = {};
-	foreach my $k (keys %$src) {
-	    $res->{$k} = $src->{$k};
-	}
+    if (!$class->is_separated()) {
+	foreach my $p (keys %$propertyList) {
+	    next if $skip_type && $p eq 'type';
 
-	return $res;
-    };
+	    if (!$propertyList->{$p}->{optional}) {
+		$props->{$p} = $propertyList->{$p};
+		next;
+	    }
 
-    foreach my $p (keys %$propertyList) {
-	next if $skip_type && $p eq 'type';
+	    my $required = 1;
 
-	if (!$propertyList->{$p}->{optional}) {
-	    $props->{$p} = $propertyList->{$p};
-	    next;
-	}
+	    my $copts = $class->options();
+	    $required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
 
-	my $required = 1;
-
-	my $copts = $class->options();
-	$required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
+	    foreach my $t (keys %$plugins) {
+		my $opts = $pdata->{options}->{$t} || {};
+		$required = 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};
+	    }
 
-	foreach my $t (keys %$plugins) {
-	    my $opts = $pdata->{options}->{$t} || {};
-	    $required = 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};
+	    if ($required) {
+		# make a copy, because we modify the optional property
+		my $res = {$propertyList->{$p}->%*}; # shallow copy
+		$res->{optional} = 0;
+		$props->{$p} = $res;
+	    } else {
+		$props->{$p} = $propertyList->{$p};
+	    }
 	}
-
-	if ($required) {
-	    # make a copy, because we modify the optional property
-	    my $res = &$copy_property($propertyList->{$p});
-	    $res->{optional} = 0;
-	    $props->{$p} = $res;
-	} else {
-	    $props->{$p} = $propertyList->{$p};
+    } else {
+	for my $type (sort keys %$plugins) {
+	    my $opts = $pdata->{options}->{$type} || {};
+	    for my $key (sort keys $opts->%*) {
+		my $schema = $class->get_property_schema($type, $key);
+		my $prop = {$schema->%*};
+		$prop->{'instance-types'} = [$type];
+		$prop->{'type-property'} = 'type';
+		$prop->{optional} = 1 if $opts->{$key}->{optional};
+
+		add_property($props, $key, $prop, $type);
+	    }
+	}
+	# add remaining global properties
+	for my $opt (keys $propertyList->%*) {
+	    next if $props->{$opt};
+	    $props->{$opt} = {$propertyList->{$opt}->%*};
+	}
+	for my $opt (keys $props->%*) {
+	    if (my $necessaryTypes = $props->{$opt}->{'instance-types'}) {
+		if ($necessaryTypes->@* == scalar(keys $plugins->%*)) {
+		    delete $props->{$opt}->{'instance-types'};
+		    delete $props->{$opt}->{'type-property'};
+		} else {
+		    $props->{$opt}->{optional} = 1;
+		}
+	    }
 	}
     }
 
@@ -117,30 +192,61 @@ sub updateSchema {
 
     my $filter_type = $single_class ? $class->type() : undef;
 
-    foreach my $p (keys %$propertyList) {
-	next if $p eq 'type';
+    if (!$class->is_separated()) {
+	foreach my $p (keys %$propertyList) {
+	    next if $p eq 'type';
 
-	my $copts = $class->options();
+	    my $copts = $class->options();
 
-	next if defined($filter_type) && !defined($copts->{$p});
+	    next if defined($filter_type) && !defined($copts->{$p});
 
-	if (!$propertyList->{$p}->{optional}) {
-	    $props->{$p} = $propertyList->{$p};
-	    next;
-	}
+	    if (!$propertyList->{$p}->{optional}) {
+		$props->{$p} = $propertyList->{$p};
+		next;
+	    }
 
-	my $modifyable = 0;
+	    my $modifyable = 0;
 
-	$modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
+	    $modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
 
-	foreach my $t (keys %$plugins) {
-	    my $opts = $pdata->{options}->{$t} || {};
-	    next if !defined($opts->{$p});
-	    $modifyable = 1 if !$opts->{$p}->{fixed};
+	    foreach my $t (keys %$plugins) {
+		my $opts = $pdata->{options}->{$t} || {};
+		next if !defined($opts->{$p});
+		$modifyable = 1 if !$opts->{$p}->{fixed};
+	    }
+	    next if !$modifyable;
+
+	    $props->{$p} = $propertyList->{$p};
+	}
+    } else {
+	for my $type (sort keys %$plugins) {
+	    my $opts = $pdata->{options}->{$type} || {};
+	    for my $key (sort keys $opts->%*) {
+		next if $opts->{$key}->{fixed};
+
+		my $schema = $class->get_property_schema($type, $key);
+		my $prop = {$schema->%*};
+		$prop->{'instance-types'} = [$type];
+		$prop->{'type-property'} = 'type';
+		$prop->{optional} = 1;
+
+		add_property($props, $key, $prop, $type);
+	    }
 	}
-	next if !$modifyable;
 
-	$props->{$p} = $propertyList->{$p};
+	for my $opt (keys $propertyList->%*) {
+	    next if $props->{$opt};
+	    $props->{$opt} = {$propertyList->{$opt}->%*};
+	}
+
+	for my $opt (keys $props->%*) {
+	    if (my $necessaryTypes = $props->{$opt}->{'instance-types'}) {
+		if ($necessaryTypes->@* == scalar(keys $plugins->%*)) {
+		    delete $props->{$opt}->{'instance-types'};
+		    delete $props->{$opt}->{'type-property'};
+		}
+	    }
+	}
     }
 
     $props->{digest} = get_standard_option('pve-config-digest');
@@ -160,22 +266,28 @@ sub updateSchema {
 }
 
 sub init {
-    my ($class) = @_;
+    my ($class, $separate_properties) = @_;
 
     my $pdata = $class->private();
 
-    foreach my $k (qw(options plugins plugindata propertyList)) {
+    foreach my $k (qw(options plugins plugindata propertyList propertyListSeparated)) {
 	$pdata->{$k} = {} if !$pdata->{$k};
     }
 
     my $plugins = $pdata->{plugins};
     my $propertyList = $pdata->{propertyList};
+    my $propertyListSeparated = $pdata->{propertyListSeparated};
 
     foreach my $type (keys %$plugins) {
 	my $props = $plugins->{$type}->properties();
 	foreach my $p (keys %$props) {
-	    die "duplicate property '$p'" if defined($propertyList->{$p});
-	    my $res = $propertyList->{$p} = {};
+	    my $res;
+	    if ($separate_properties) {
+		$res = $propertyListSeparated->{$type}->{$p} = {};
+	    } else {
+		die "duplicate property '$p'" if defined($propertyList->{$p});
+		$res = $propertyList->{$p} = {};
+	    }
 	    my $data = $props->{$p};
 	    for my $a (keys %$data) {
 		$res->{$a} = $data->{$a};
@@ -187,8 +299,23 @@ sub init {
     foreach my $type (keys %$plugins) {
 	my $opts = $plugins->{$type}->options();
 	foreach my $p (keys %$opts) {
-	    die "undefined property '$p'" if !$propertyList->{$p};
+	    my $prop;
+	    if ($separate_properties) {
+		$prop = $propertyListSeparated->{$type}->{$p};
+	    }
+	    $prop //= $propertyList->{$p};
+	    die "undefined property '$p'" if !$prop;
 	}
+
+	# automatically the properties to options (if not specified explicitely)
+	if ($separate_properties) {
+	    foreach my $p (keys $propertyListSeparated->{$type}->%*) {
+		next if $opts->{$p};
+		$opts->{$p} = {};
+		$opts->{$p}->{optional} = 1 if $propertyListSeparated->{$type}->{$p}->{optional};
+	    }
+	}
+
 	$pdata->{options}->{$type} = $opts;
     }
 
@@ -237,11 +364,12 @@ sub check_value {
     return $value if $key eq 'type' && $type eq $value;
 
     my $opts = $pdata->{options}->{$type};
+
     die "unknown section type '$type'\n" if !$opts;
 
     die "unexpected property '$key'\n" if !defined($opts->{$key});
 
-    my $schema = $pdata->{propertyList}->{$key};
+    my $schema = $class->get_property_schema($type, $key);
     die "unknown property type\n" if !$schema;
 
     my $ct = $schema->{type};
@@ -295,6 +423,20 @@ sub format_section_header {
     return "$type: $sectionId\n";
 }
 
+sub get_property_schema {
+    my ($class, $type, $key) = @_;
+
+    my $pdata = $class->private();
+    my $opts = $pdata->{options}->{$type};
+
+    my $schema;
+    if ($class->is_separated()) {
+	$schema = $pdata->{propertyListSeparated}->{$type}->{$key};
+    }
+    $schema //= $pdata->{propertyList}->{$key};
+
+    return $schema;
+}
 
 sub parse_config {
     my ($class, $filename, $raw, $allow_unknown) = @_;
@@ -322,7 +464,7 @@ sub parse_config {
     my $is_array = sub {
 	my ($type, $key) = @_;
 
-	my $schema = $pdata->{propertyList}->{$key};
+	my $schema = $class->get_property_schema($type, $key);
 	die "unknown property type\n" if !$schema;
 
 	return $schema->{type} eq 'array';
@@ -494,7 +636,6 @@ sub write_config {
     my ($class, $filename, $cfg, $allow_unknown) = @_;
 
     my $pdata = $class->private();
-    my $propertyList = $pdata->{propertyList};
 
     my $out = '';
 
@@ -516,6 +657,7 @@ sub write_config {
 	my $scfg = $ids->{$sectionId};
 	my $type = $scfg->{type};
 	my $opts = $pdata->{options}->{$type};
+	my $global_opts = $pdata->{options}->{__global};
 
 	die "unknown section type '$type'\n" if !$opts && !$allow_unknown;
 
@@ -545,7 +687,8 @@ sub write_config {
 	if ($scfg->{comment} && !$done_hash->{comment}) {
 	    my $k = 'comment';
 	    my $v = $class->encode_value($type, $k, $scfg->{$k});
-	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
+	    my $prop = $class->get_property_schema($type, $k);
+	    $data .= &$format_config_line($prop, $k, $v);
 	}
 
 	$data .= "\tdisable\n" if $scfg->{disable} && !$done_hash->{disable};
@@ -562,7 +705,8 @@ sub write_config {
 	    die "section '$sectionId' - missing value for required option '$k'\n"
 		if !defined ($v);
 	    $v = $class->encode_value($type, $k, $v);
-	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
+	    my $prop = $class->get_property_schema($type, $k);
+	    $data .= &$format_config_line($prop, $k, $v);
 	}
 
 	foreach my $k (@option_keys) {
@@ -570,7 +714,8 @@ sub write_config {
 	    my $v = $scfg->{$k};
 	    next if !defined($v);
 	    $v = $class->encode_value($type, $k, $v);
-	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
+	    my $prop = $class->get_property_schema($type, $k);
+	    $data .= &$format_config_line($prop, $k, $v);
 	}
 
 	$out .= "$data\n";
diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index b3af2c6..23ed069 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -36,6 +36,7 @@ no warnings 'portable'; # Support for 64-bit ints required
 our @EXPORT_OK = qw(
 $IPV6RE
 $IPV4RE
+compare_deeply
 lock_file
 lock_file_full
 run_command
@@ -2150,4 +2151,36 @@ sub get_file_hash {
     return lc($digest);
 }
 
+# compare two perl variables recursively, so this works for scalars, nested
+# hashes and nested arrays
+sub compare_deeply {
+    my ($a, $b) = @_;
+
+    return 0 if defined($a) != defined($b);
+    return 1 if !defined($a); # both are undef
+
+    my $ref_a = ref($a);
+    my $ref_b = ref($b);
+
+    # scalar case
+    return 0 if !$ref_a && !$ref_b && "$a" ne "$b";
+
+    # different types
+    return 0 if $ref_a ne $ref_b;
+
+    if ($ref_a eq 'HASH') {
+	return 0 if $a->%* != $b->%*;
+	for my $opt (keys $a->%*) {
+	    return 0 if !compare_deeply->($a->{$opt}, $b->{$opt});
+	}
+    } elsif ($ref_a eq 'ARRAY') {
+	return 0 if $a->@* != $b->@*;
+	for (my $i = 0; $i < $a->@*; $i++) {
+	    return 0 if !compare_deeply->($a->[$i], $b->[$i]);
+	}
+    }
+
+    return 1;
+}
+
 1;
-- 
2.30.2

[pve-devel] [PATCH common v2 4/4] section config: add tests for separated property lists

[Dominik Csapak]

more or less a copy from the normal section config test, but now with
properties defined multiple times as well as conflicting options

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* changed generated api schema to reflect optimizations in SectionConfig
 test/Makefile                         |   1 +
 test/section_config_separated_test.pl | 489 ++++++++++++++++++++++++++
 2 files changed, 490 insertions(+)
 create mode 100755 test/section_config_separated_test.pl

diff --git a/test/Makefile b/test/Makefile
index 82f40ab..3e9fef2 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -6,6 +6,7 @@ TESTS = lock_file.test			\
 	format_test.test		\
 	section_config_test.test	\
 	api_parameter_test.test		\
+	section_config_separated_test.test\
 
 all:
 
diff --git a/test/section_config_separated_test.pl b/test/section_config_separated_test.pl
new file mode 100755
index 0000000..0d4e4fe
--- /dev/null
+++ b/test/section_config_separated_test.pl
@@ -0,0 +1,489 @@
+#!/usr/bin/perl
+
+use lib '../src';
+
+package Conf;
+use strict;
+use warnings;
+
+use Test::More;
+
+use base qw(PVE::SectionConfig);
+
+my $defaultData = {
+    propertyList => {
+	type => { description => "Section type." },
+	id => {
+	    description => "ID",
+	    type => 'string',
+	    format => 'pve-configid',
+	    maxLength => 64,
+	},
+	common => {
+	    type => 'string',
+	    description => 'common value',
+	    maxLength => 512,
+	},
+    },
+};
+
+sub private {
+    return $defaultData;
+}
+
+sub expect_success {
+    my ($class, $filename, $expected, $raw, $allow_unknown) = @_;
+
+    my $res = $class->parse_config($filename, $raw, $allow_unknown);
+    delete $res->{digest};
+
+    is_deeply($res, $expected, $filename);
+
+    my $written = $class->write_config($filename, $res, $allow_unknown);
+    my $res2 = $class->parse_config($filename, $written, $allow_unknown);
+    delete $res2->{digest};
+
+    is_deeply($res, $res2, "$filename - verify rewritten data");
+}
+
+sub expect_fail {
+    my ($class, $filename, $expected, $raw) = @_;
+
+    eval { $class->parse_config($filename, $raw) };
+    die "test '$filename' succeeded unexpectedly\n" if !$@;
+    ok(1, "$filename should fail to parse");
+}
+
+package Conf::One;
+use strict;
+use warnings;
+
+use base 'Conf';
+
+sub type {
+    return 'one';
+}
+
+sub properties {
+    return {
+	field1 => {
+	    description => 'Field One',
+	    type => 'integer',
+	    minimum => 3,
+	    maximum => 9,
+	},
+	field2 => {
+	    description => 'Field Two',
+	    type => 'integer',
+	    minimum => 10,
+	    maximum => 19,
+	},
+	another => {
+	    description => 'Another field',
+	    type => 'string',
+	    optional => 1,
+	},
+	arrayfield => {
+	    description => "Array Field with property string",
+	    optional => 1,
+	    type => 'array',
+	    items => {
+		type => 'string',
+		description => 'a property string',
+		format => {
+		    subfield1 => {
+			type => 'string',
+			description => 'first subfield'
+		    },
+		    subfield2 => {
+			type => 'integer',
+			minimum => 0,
+			optional => 1,
+		    },
+		},
+	    },
+	},
+    };
+}
+
+sub options {
+    return {
+	common => { optional => 1 },
+    };
+}
+
+package Conf::Two;
+use strict;
+use warnings;
+
+use base 'Conf';
+
+sub type {
+    return 'two';
+}
+
+sub properties {
+    return {
+	field2 => {
+	    description => 'Field Two but different',
+	    type => 'integer',
+	    minimum => 3,
+	    maximum => 9,
+	},
+	another => {
+	    description => 'Another field',
+	    type => 'string',
+	},
+	arrayfield => {
+	    optional => 1,
+	    description => "Array Field with property string",
+	    type => 'array',
+	    items => {
+		type => 'string',
+		description => 'a property string',
+		format => {
+		    subfield1 => {
+			type => 'string',
+			description => 'first subfield'
+		    },
+		    subfield2 => {
+			type => 'integer',
+			minimum => 0,
+			optional => 1,
+		    },
+		},
+	    },
+	},
+    };
+}
+
+sub options {
+    return {
+	common => { optional => 1 },
+    };
+}
+
+package main;
+
+use strict;
+use warnings;
+
+use Test::More;
+
+Conf::One->register();
+Conf::Two->register();
+Conf->init(1);
+
+# FIXME: allow development debug warnings?!
+local $SIG{__WARN__} = sub { die @_; };
+
+my sub enum {
+    my $n = 1;
+    return { map { $_ => $n++ } @_ };
+}
+
+Conf->expect_success(
+    'separated-test1',
+    {
+	ids => {
+	    t1 => {
+		type => 'one',
+		common => 'foo',
+		field1 => 3,
+		field2 => 10,
+		arrayfield => [ 'subfield1=test' ],
+	    },
+	    t2 => {
+		type => 'one',
+		common => 'foo2',
+		field1 => 4,
+		field2 => 15,
+		another => 'more-text',
+	    },
+	    t3 => {
+		type => 'two',
+		field2 => 5,
+		another => 'even more text',
+	    },
+	},
+	order => { t1 => 1, t2 => 2, t3 => 3 },
+    },
+    <<"EOF");
+one: t1
+	common foo
+	field1 3
+	field2 10
+	arrayfield subfield1=test
+
+one: t2
+	common foo2
+	field1 4
+	field2 15
+	another more-text
+
+two: t3
+	field2 5
+	another even more text
+EOF
+
+my $with_unknown_data = {
+    ids => {
+	t1 => {
+	    type => 'one',
+	    common => 'foo',
+	    field1 => 3,
+	    field2 => 10,
+	},
+	t2 => {
+	    type => 'one',
+	    common => 'foo2',
+	    field1 => 4,
+	    field2 => 15,
+	    another => 'more-text',
+	},
+	t3 => {
+	    type => 'two',
+	    field2 => 5,
+	    another => 'even more text',
+	    arrayfield => [
+		'subfield1=test,subfield2=2',
+		'subfield1=test2',
+	    ],
+	},
+	invalid => {
+	    type => 'bad',
+	    common => 'omg',
+	    unknownfield => 'shouldnotbehere',
+	    unknownarray => ['entry1', 'entry2'],
+	},
+    },
+    order => enum(qw(t1 t2 invalid t3)),
+};
+my $with_unknown_text = <<"EOF";
+one: t1
+	common foo
+	field1 3
+	field2 10
+
+one: t2
+	common foo2
+	field1 4
+	field2 15
+	another more-text
+
+bad: invalid
+	common omg
+	unknownfield shouldnotbehere
+	unknownarray entry1
+	unknownarray entry2
+
+two: t3
+	field2 5
+	another even more text
+	arrayfield subfield1=test,subfield2=2
+	arrayfield subfield1=test2
+EOF
+
+my $wrong_field_schema_data = {
+    ids => {
+	t1 => {
+	    type => 'one',
+	    common => 'foo',
+	    field1 => 3,
+	    field2 => 5, # this should fail
+	},
+    },
+    order => enum(qw(t1)),
+};
+
+my $wrong_field_schema_text = <<"EOF";
+one: t1
+	common foo
+	field1 3
+	field2 5
+EOF
+
+Conf->expect_fail('separated-wrong-field-schema', $wrong_field_schema_data, $wrong_field_schema_text);
+Conf->expect_fail('separated-unknown-forbidden', $with_unknown_data, $with_unknown_text);
+Conf->expect_success('separated-unknown-allowed', $with_unknown_data, $with_unknown_text, 1);
+
+# schema tests
+my $create_schema = Conf->createSchema();
+my $expected_create_schema = {
+    additionalProperties => 0,
+    type => 'object',
+    properties => {
+	id => {
+	    description => "ID",
+	    type => 'string',
+	    format => 'pve-configid',
+	    maxLength => 64,
+	},
+	type => {
+	    description => 'Section type.',
+	    enum => [ 'one', 'two' ],
+	    type => 'string'
+	},
+	common => {
+	    maxLength => 512,
+	    optional => 1,
+	    type => 'string',
+	    description => 'common value'
+	},
+	field1 => {
+	    type => 'integer',
+	    'type-property' => 'type',
+	    'instance-types' => [ 'one' ],
+	    maximum => 9,
+	    optional => 1,
+	    minimum => 3,
+	    description => 'Field One'
+	},
+	field2 => {
+	    oneOf => [
+		{
+		    description => 'Field Two',
+		    optional => 1,
+		    minimum => 10,
+		    'instance-types' => [ 'one' ],
+		    type => 'integer',
+		    maximum => 19
+		},
+		{
+		    optional => 1,
+		    minimum => 3,
+		    description => 'Field Two but different',
+		    type => 'integer',
+		    'instance-types' => [ 'two' ],
+		    maximum => 9
+		}
+	    ],
+	    'type-property' => 'type'
+	},
+	arrayfield => {
+	    items => {
+		type => 'string',
+		format => {
+		    subfield1 => {
+			description => 'first subfield',
+			type => 'string'
+		    },
+		    subfield2 => {
+			minimum => 0,
+			type => 'integer',
+			optional => 1
+		    }
+		},
+		description => 'a property string'
+	    },
+	    description => 'Array Field with property string',
+	    type => 'array',
+	    optional => 1
+	},
+	another => {
+	    optional => 1,
+	    type => 'string',
+	    description => 'Another field'
+	},
+    },
+};
+
+is_deeply($create_schema, $expected_create_schema, "separated create schema test");
+
+my $update_schema = Conf->updateSchema();
+my $expected_update_schema = {
+    additionalProperties => 0,
+    type => 'object',
+    properties => {
+	id => {
+	    description => "ID",
+	    type => 'string',
+	    format => 'pve-configid',
+	    maxLength => 64,
+	},
+	type => {
+	    type => 'string',
+	    enum => [ 'one', 'two' ],
+	    description => 'Section type.'
+	},
+	digest => {
+	    optional => 1,
+	    type => 'string',
+	    description => 'Prevent changes if current configuration file has a different digest. This can be used to prevent concurrent modifications.',
+	    maxLength => 64
+	},
+	delete => {
+	    description => 'A list of settings you want to delete.',
+	    maxLength => 4096,
+	    format => 'pve-configid-list',
+	    optional => 1,
+	    type => 'string'
+	},
+	common => {
+	    maxLength => 512,
+	    description => 'common value',
+	    type => 'string',
+	    optional => 1
+	},
+	field1 => {
+	    description => 'Field One',
+	    maximum => 9,
+	    'instance-types' => [ 'one' ],
+	    'type-property' => 'type',
+	    minimum => 3,
+	    optional => 1,
+	    type => 'integer'
+	},
+	field2 => {
+	    'type-property' => 'type',
+	    oneOf => [
+		{
+		    type => 'integer',
+		    minimum => 10,
+		    optional => 1,
+		    maximum => 19,
+		    'instance-types' => [ 'one' ],
+		    description => 'Field Two'
+		},
+		{
+		    description => 'Field Two but different',
+		    maximum => 9,
+		    'instance-types' => [ 'two' ],
+		    minimum => 3,
+		    optional => 1,
+		    type => 'integer'
+		}
+	    ]
+	},
+	arrayfield => {
+	    type => 'array',
+	    optional => 1,
+	    items => {
+		description => 'a property string',
+		type => 'string',
+		format => {
+		    subfield2 => {
+			type => 'integer',
+			minimum => 0,
+			optional => 1
+		    },
+		    subfield1 => {
+			description => 'first subfield',
+			type => 'string'
+		    }
+		}
+	    },
+	    description => 'Array Field with property string'
+	},
+	another => {
+	    description => 'Another field',
+	    optional => 1,
+	    type => 'string'
+	},
+    }
+};
+is_deeply($update_schema, $expected_update_schema, "separated update schema test");
+
+done_testing();
+
+1;
-- 
2.30.2

[pve-devel] [PATCH widget-toolkit v2 1/1] api-viewer: implement basic oneOf support

[Dominik Csapak]

for parameters only for now, also only implement the basic use case we
want to have currently: use in section config apis where we have more
than one type.

we could improve upon that, e.g. by properly grouping the type relevant
options, and also implementing that for return types.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes
 src/api-viewer/APIViewer.js | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/src/api-viewer/APIViewer.js b/src/api-viewer/APIViewer.js
index d9753b2..26a1c08 100644
--- a/src/api-viewer/APIViewer.js
+++ b/src/api-viewer/APIViewer.js
@@ -7,7 +7,7 @@ Ext.onReady(function() {
 	    'name', 'type', 'typetext', 'description', 'verbose_description',
 	    'enum', 'minimum', 'maximum', 'minLength', 'maxLength',
 	    'pattern', 'title', 'requires', 'format', 'default',
-	    'disallow', 'extends', 'links',
+	    'disallow', 'extends', 'links', 'instance-types',
 	    {
 		name: 'optional',
 		type: 'boolean',
@@ -214,6 +214,10 @@ Ext.onReady(function() {
 			},
 			groupField: 'optional',
 			sorters: [
+			    {
+				property: 'instance-types',
+				direction: 'ASC',
+			    },
 			    {
 				property: 'name',
 				direction: 'ASC',
@@ -221,9 +225,27 @@ Ext.onReady(function() {
 			],
 		    });
 
+		    let has_type_properties = false;
+
 		    Ext.Object.each(info.parameters.properties, function(name, pdef) {
-			pdef.name = name;
-			pstore.add(pdef);
+			if (pdef.oneOf) {
+			    pdef.oneOf.forEach((alternative) => {
+				alternative.name = name;
+				pstore.add(alternative);
+				has_type_properties = true;
+			    });
+			} else if (pdef['instance-types']) {
+			    pdef['instance-types'].forEach((type) => {
+				let typePdef = Ext.apply({}, pdef);
+				typePdef.name = name;
+				typePdef['instance-types'] = [type];
+				pstore.add(typePdef);
+				has_type_properties = true;
+			    });
+			} else {
+			    pdef.name = name;
+			    pstore.add(pdef);
+			}
 		    });
 
 		    pstore.sort();
@@ -255,6 +277,12 @@ Ext.onReady(function() {
 				renderer: render_type,
 				flex: 1,
 			    },
+			    {
+				header: 'For Types',
+				dataIndex: 'instance-types',
+				hidden: !has_type_properties,
+				flex: 1,
+			    },
 			    {
 				header: 'Default',
 				dataIndex: 'default',
-- 
2.30.2

Re: [pve-devel] [PATCH common v2 3/4] section config: allow separated property lists for plugins

[Thomas Lamprecht]

Am 15/11/2023 um 14:51 schrieb Dominik Csapak:
> when using 'init(1)'. This saves the property lists per type instead of
> a big one, and using create/updateSchema creates a new schema with the
> options as 'oneOf' and/or 'instance-types' (depending if the schemas
> match).
> 
> with that, we change how we work with the options hash:
> 
> it's not needed anymore to specify options that are specified in the

"it's no longer necessary [...]" would be a bit easier to read.

> type specific propertyList, except if it's 'fixed => 1' (since that does
> not exist in the schema)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * don't use the __global hack anymore, we now construct the
>   plugin-specific options hash in `init`
> * all 'global' properties are added in the api as they are
> * improved the compare function and moved to PVE::Tools as a generic
>   recursive variable compare
> * removed copy_property
> * optimized the genrated schema: when a property is used by all types,
>   the 'type-property' and 'instance-types' is removed, this makes the
>   docs a bit shorter when many of such options exist
> * added an is_separated call
> 
>  src/PVE/SectionConfig.pm | 259 ++++++++++++++++++++++++++++++---------
>  src/PVE/Tools.pm         |  33 +++++
>  2 files changed, 235 insertions(+), 57 deletions(-)
> 
> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> index 5690476..eb39b41 100644
> --- a/src/PVE/SectionConfig.pm
> +++ b/src/PVE/SectionConfig.pm
> @@ -8,6 +8,7 @@ use Digest::SHA;
>  
>  use PVE::Exception qw(raise_param_exc);
>  use PVE::JSONSchema qw(get_standard_option);
> +use PVE::Tools qw(compare_deeply);
>  
>  my $defaultData = {
>      options => {},
> @@ -51,6 +52,61 @@ sub plugindata {
>      return {};
>  }
>  
> +sub is_separated {
> +    my ($class) = @_;
> +
> +    return $class->private()->{propertyListSeparated}->%* > 0;

the propertyListSeparated name is not telling me much about what this is for.
This is for property isolation? Maybe going with the term in that direction
would be better.

"private" is also often used for such things, but we already use that for
another thing IIRC or "shared" vs "exclusive", e.g., shared_schema.
Adding a comment at the top of the file describing describing how the schemas
are (or are not) merged from all plugin-types would help others to easier
understand this too I think.

But from top of my head I have no Real Good™ proposal for what to use, but I do
not really like the term "separated" here and just think that it might be a bit
to hard to understand what it means.
That naming is actually my biggest gripe with the patch (but I didn't look that
closely), most other things are (stylistic) nits that I'd not block this on.


btw. can we be a bit more explicit here:

my $place_holder_name = $class->private()->{placeHolderName};

return defined($place_holder_name) && scalar(keys $place_holder_name->%*) > 0;

> +}
> +
> +my sub cmp_property {

s/cmp/compare/ but not sure if that name fits that well either with the
$skip_opts stuff, but it's small enough to still be fine.

> +    my ($a, $b, $skip_opts) = @_;
> +
> +    my $merged = {$a->%*, $b->%*};
> +    for my $opt ($skip_opts->@*) {
> +	delete $merged->{$opt};
> +    }

no hard feelings, but could be:

delete $merged->{$_} for $skip_opts->@*;

> +    for my $opt (keys $merged->%*) {
> +	return 0 if !compare_deeply($a->{$opt}, $b->{$opt});
> +    }
> +
> +    return 1;
> +};
> +
> +my sub add_property {
> +    my ($props, $key, $prop, $type) = @_;
> +
> +    if (!defined($props->{$key})) {
> +	$props->{$key} = $prop;
> +	return;
> +    }
> +
> +    if (!defined($props->{$key}->{oneOf})) {
> +	if (cmp_property($props->{$key}, $prop, ['instance-types'])) {
> +	    push $props->{$key}->{'instance-types'}->@*, $type;
> +	} else {
> +	    my $new_prop = delete $props->{$key};
> +	    delete $new_prop->{'type-property'};
> +	    delete $prop->{'type-property'};
> +	    $props->{$key} = {
> +		'type-property' => 'type',
> +		oneOf => [
> +		    $new_prop,
> +		    $prop,
> +		],
> +	    };
> +	}
> +    } else {
> +	for my $existing_prop ($props->{$key}->{oneOf}->@*) {
> +	    if (cmp_property($existing_prop, $prop, ['instance-types', 'type-property'])) {
> +		push $existing_prop->{'instance-types'}->@*, $type;
> +		return;
> +	    }
> +	}
> +
> +	push $props->{$key}->{oneOf}->@*, $prop;
> +    }
> +};
> +
>  sub createSchema {
>      my ($class, $skip_type, $base) = @_;
>  
> @@ -60,42 +116,61 @@ sub createSchema {
>  
>      my $props = $base || {};
>  
> -    my $copy_property = sub {
> -	my ($src) = @_;
> -
> -	my $res = {};
> -	foreach my $k (keys %$src) {
> -	    $res->{$k} = $src->{$k};
> -	}
> +    if (!$class->is_separated()) {
> +	foreach my $p (keys %$propertyList) {
> +	    next if $skip_type && $p eq 'type';
>  
> -	return $res;
> -    };
> +	    if (!$propertyList->{$p}->{optional}) {
> +		$props->{$p} = $propertyList->{$p};
> +		next;
> +	    }
>  
> -    foreach my $p (keys %$propertyList) {
> -	next if $skip_type && $p eq 'type';
> +	    my $required = 1;
>  
> -	if (!$propertyList->{$p}->{optional}) {
> -	    $props->{$p} = $propertyList->{$p};
> -	    next;
> -	}
> +	    my $copts = $class->options();
> +	    $required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
>  
> -	my $required = 1;
> -
> -	my $copts = $class->options();
> -	$required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
> +	    foreach my $t (keys %$plugins) {
> +		my $opts = $pdata->{options}->{$t} || {};
> +		$required = 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};
> +	    }
>  
> -	foreach my $t (keys %$plugins) {
> -	    my $opts = $pdata->{options}->{$t} || {};
> -	    $required = 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};
> +	    if ($required) {
> +		# make a copy, because we modify the optional property
> +		my $res = {$propertyList->{$p}->%*}; # shallow copy
> +		$res->{optional} = 0;
> +		$props->{$p} = $res;
> +	    } else {
> +		$props->{$p} = $propertyList->{$p};
> +	    }
>  	}
> -
> -	if ($required) {
> -	    # make a copy, because we modify the optional property
> -	    my $res = &$copy_property($propertyList->{$p});
> -	    $res->{optional} = 0;
> -	    $props->{$p} = $res;
> -	} else {
> -	    $props->{$p} = $propertyList->{$p};
> +    } else {
> +	for my $type (sort keys %$plugins) {
> +	    my $opts = $pdata->{options}->{$type} || {};
> +	    for my $key (sort keys $opts->%*) {
> +		my $schema = $class->get_property_schema($type, $key);
> +		my $prop = {$schema->%*};
> +		$prop->{'instance-types'} = [$type];
> +		$prop->{'type-property'} = 'type';
> +		$prop->{optional} = 1 if $opts->{$key}->{optional};
> +
> +		add_property($props, $key, $prop, $type);
> +	    }
> +	}
> +	# add remaining global properties
> +	for my $opt (keys $propertyList->%*) {
> +	    next if $props->{$opt};
> +	    $props->{$opt} = {$propertyList->{$opt}->%*};
> +	}
> +	for my $opt (keys $props->%*) {
> +	    if (my $necessaryTypes = $props->{$opt}->{'instance-types'}) {
> +		if ($necessaryTypes->@* == scalar(keys $plugins->%*)) {
> +		    delete $props->{$opt}->{'instance-types'};
> +		    delete $props->{$opt}->{'type-property'};
> +		} else {
> +		    $props->{$opt}->{optional} = 1;
> +		}
> +	    }
>  	}
>      }
>  
> @@ -117,30 +192,61 @@ sub updateSchema {
>  
>      my $filter_type = $single_class ? $class->type() : undef;
>  
> -    foreach my $p (keys %$propertyList) {
> -	next if $p eq 'type';
> +    if (!$class->is_separated()) {
> +	foreach my $p (keys %$propertyList) {
> +	    next if $p eq 'type';
>  
> -	my $copts = $class->options();
> +	    my $copts = $class->options();
>  
> -	next if defined($filter_type) && !defined($copts->{$p});
> +	    next if defined($filter_type) && !defined($copts->{$p});
>  
> -	if (!$propertyList->{$p}->{optional}) {
> -	    $props->{$p} = $propertyList->{$p};
> -	    next;
> -	}
> +	    if (!$propertyList->{$p}->{optional}) {
> +		$props->{$p} = $propertyList->{$p};
> +		next;
> +	    }
>  
> -	my $modifyable = 0;
> +	    my $modifyable = 0;
>  
> -	$modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
> +	    $modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
>  
> -	foreach my $t (keys %$plugins) {
> -	    my $opts = $pdata->{options}->{$t} || {};
> -	    next if !defined($opts->{$p});
> -	    $modifyable = 1 if !$opts->{$p}->{fixed};
> +	    foreach my $t (keys %$plugins) {
> +		my $opts = $pdata->{options}->{$t} || {};
> +		next if !defined($opts->{$p});
> +		$modifyable = 1 if !$opts->{$p}->{fixed};
> +	    }
> +	    next if !$modifyable;
> +
> +	    $props->{$p} = $propertyList->{$p};
> +	}
> +    } else {
> +	for my $type (sort keys %$plugins) {
> +	    my $opts = $pdata->{options}->{$type} || {};
> +	    for my $key (sort keys $opts->%*) {
> +		next if $opts->{$key}->{fixed};
> +
> +		my $schema = $class->get_property_schema($type, $key);
> +		my $prop = {$schema->%*};
> +		$prop->{'instance-types'} = [$type];
> +		$prop->{'type-property'} = 'type';
> +		$prop->{optional} = 1;
> +
> +		add_property($props, $key, $prop, $type);
> +	    }
>  	}
> -	next if !$modifyable;
>  
> -	$props->{$p} = $propertyList->{$p};
> +	for my $opt (keys $propertyList->%*) {
> +	    next if $props->{$opt};
> +	    $props->{$opt} = {$propertyList->{$opt}->%*};
> +	}
> +
> +	for my $opt (keys $props->%*) {
> +	    if (my $necessaryTypes = $props->{$opt}->{'instance-types'}) {
> +		if ($necessaryTypes->@* == scalar(keys $plugins->%*)) {
> +		    delete $props->{$opt}->{'instance-types'};
> +		    delete $props->{$opt}->{'type-property'};
> +		}
> +	    }
> +	}
>      }
>  
>      $props->{digest} = get_standard_option('pve-config-digest');
> @@ -160,22 +266,28 @@ sub updateSchema {
>  }
>  
>  sub init {
> -    my ($class) = @_;
> +    my ($class, $separate_properties) = @_;
>  
>      my $pdata = $class->private();
>  
> -    foreach my $k (qw(options plugins plugindata propertyList)) {
> +    foreach my $k (qw(options plugins plugindata propertyList propertyListSeparated)) {
>  	$pdata->{$k} = {} if !$pdata->{$k};
>      }
>  
>      my $plugins = $pdata->{plugins};
>      my $propertyList = $pdata->{propertyList};
> +    my $propertyListSeparated = $pdata->{propertyListSeparated};
>  
>      foreach my $type (keys %$plugins) {
>  	my $props = $plugins->{$type}->properties();
>  	foreach my $p (keys %$props) {
> -	    die "duplicate property '$p'" if defined($propertyList->{$p});
> -	    my $res = $propertyList->{$p} = {};
> +	    my $res;
> +	    if ($separate_properties) {
> +		$res = $propertyListSeparated->{$type}->{$p} = {};
> +	    } else {
> +		die "duplicate property '$p'" if defined($propertyList->{$p});
> +		$res = $propertyList->{$p} = {};
> +	    }
>  	    my $data = $props->{$p};
>  	    for my $a (keys %$data) {
>  		$res->{$a} = $data->{$a};
> @@ -187,8 +299,23 @@ sub init {
>      foreach my $type (keys %$plugins) {
>  	my $opts = $plugins->{$type}->options();
>  	foreach my $p (keys %$opts) {
> -	    die "undefined property '$p'" if !$propertyList->{$p};
> +	    my $prop;
> +	    if ($separate_properties) {
> +		$prop = $propertyListSeparated->{$type}->{$p};
> +	    }
> +	    $prop //= $propertyList->{$p};
> +	    die "undefined property '$p'" if !$prop;
>  	}
> +
> +	# automatically the properties to options (if not specified explicitely)
> +	if ($separate_properties) {
> +	    foreach my $p (keys $propertyListSeparated->{$type}->%*) {
> +		next if $opts->{$p};
> +		$opts->{$p} = {};
> +		$opts->{$p}->{optional} = 1 if $propertyListSeparated->{$type}->{$p}->{optional};
> +	    }
> +	}
> +
>  	$pdata->{options}->{$type} = $opts;
>      }
>  
> @@ -237,11 +364,12 @@ sub check_value {
>      return $value if $key eq 'type' && $type eq $value;
>  
>      my $opts = $pdata->{options}->{$type};
> +

unrelated white space change

>      die "unknown section type '$type'\n" if !$opts;
>  
>      die "unexpected property '$key'\n" if !defined($opts->{$key});
>  
> -    my $schema = $pdata->{propertyList}->{$key};
> +    my $schema = $class->get_property_schema($type, $key);
>      die "unknown property type\n" if !$schema;
>  
>      my $ct = $schema->{type};
> @@ -295,6 +423,20 @@ sub format_section_header {
>      return "$type: $sectionId\n";
>  }
>  
> +sub get_property_schema {
> +    my ($class, $type, $key) = @_;
> +
> +    my $pdata = $class->private();
> +    my $opts = $pdata->{options}->{$type};
> +
> +    my $schema;
> +    if ($class->is_separated()) {
> +	$schema = $pdata->{propertyListSeparated}->{$type}->{$key};
> +    }
> +    $schema //= $pdata->{propertyList}->{$key};
> +
> +    return $schema;
> +}
>  
>  sub parse_config {
>      my ($class, $filename, $raw, $allow_unknown) = @_;
> @@ -322,7 +464,7 @@ sub parse_config {
>      my $is_array = sub {
>  	my ($type, $key) = @_;
>  
> -	my $schema = $pdata->{propertyList}->{$key};
> +	my $schema = $class->get_property_schema($type, $key);
>  	die "unknown property type\n" if !$schema;
>  
>  	return $schema->{type} eq 'array';
> @@ -494,7 +636,6 @@ sub write_config {
>      my ($class, $filename, $cfg, $allow_unknown) = @_;
>  
>      my $pdata = $class->private();
> -    my $propertyList = $pdata->{propertyList};
>  
>      my $out = '';
>  
> @@ -516,6 +657,7 @@ sub write_config {
>  	my $scfg = $ids->{$sectionId};
>  	my $type = $scfg->{type};
>  	my $opts = $pdata->{options}->{$type};
> +	my $global_opts = $pdata->{options}->{__global};
>  
>  	die "unknown section type '$type'\n" if !$opts && !$allow_unknown;
>  
> @@ -545,7 +687,8 @@ sub write_config {
>  	if ($scfg->{comment} && !$done_hash->{comment}) {
>  	    my $k = 'comment';
>  	    my $v = $class->encode_value($type, $k, $scfg->{$k});
> -	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
> +	    my $prop = $class->get_property_schema($type, $k);
> +	    $data .= &$format_config_line($prop, $k, $v);
>  	}
>  
>  	$data .= "\tdisable\n" if $scfg->{disable} && !$done_hash->{disable};
> @@ -562,7 +705,8 @@ sub write_config {
>  	    die "section '$sectionId' - missing value for required option '$k'\n"
>  		if !defined ($v);
>  	    $v = $class->encode_value($type, $k, $v);
> -	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
> +	    my $prop = $class->get_property_schema($type, $k);
> +	    $data .= &$format_config_line($prop, $k, $v);
>  	}
>  
>  	foreach my $k (@option_keys) {
> @@ -570,7 +714,8 @@ sub write_config {
>  	    my $v = $scfg->{$k};
>  	    next if !defined($v);
>  	    $v = $class->encode_value($type, $k, $v);
> -	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
> +	    my $prop = $class->get_property_schema($type, $k);
> +	    $data .= &$format_config_line($prop, $k, $v);
>  	}
>  
>  	$out .= "$data\n";
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index b3af2c6..23ed069 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -36,6 +36,7 @@ no warnings 'portable'; # Support for 64-bit ints required
>  our @EXPORT_OK = qw(
>  $IPV6RE
>  $IPV4RE
> +compare_deeply

do we really need to allow export for a single use-case?

>  lock_file
>  lock_file_full
>  run_command
> @@ -2150,4 +2151,36 @@ sub get_file_hash {
>      return lc($digest);
>  }
>  
> +# compare two perl variables recursively, so this works for scalars, nested
> +# hashes and nested arrays
> +sub compare_deeply {

could mirror the one from Test::More and name it is_deeply

Also, could be added in it's own commit with a handful of unit-tests thrown in.

> +    my ($a, $b) = @_;
> +
> +    return 0 if defined($a) != defined($b);
> +    return 1 if !defined($a); # both are undef
> +
> +    my $ref_a = ref($a);
> +    my $ref_b = ref($b);

nit: could be 

my ($ref_a, $ref_b) = (ref($a), ref($b));

Less for the single line saved, but it would IMO fit this method (but
just a tiny subjective nit)

> +
> +    # scalar case
> +    return 0 if !$ref_a && !$ref_b && "$a" ne "$b";
> +
> +    # different types
> +    return 0 if $ref_a ne $ref_b;

this can result in a undef warning, as, e.g., $ref_a equaling 'HASH' and $ref_b
equaling undef is not caught by the check before this.

Maybe add a 

return 0 if defined($ref_a) != defined($ref_b);

directly after assigning those variables?

> +
> +    if ($ref_a eq 'HASH') {
> +	return 0 if $a->%* != $b->%*;

nit: we often use `keys %$a)` for getting the size, as that makes it a bit more explicit

> +	for my $opt (keys $a->%*) {
> +	    return 0 if !compare_deeply->($a->{$opt}, $b->{$opt});

why the -> call style for the method, maybe this was a code ref inside a variable once?

> +	}
> +    } elsif ($ref_a eq 'ARRAY') {
> +	return 0 if $a->@* != $b->@*;

same nit here w.r.t. (not) using scalar

> +	for (my $i = 0; $i < $a->@*; $i++) {
> +	    return 0 if !compare_deeply->($a->[$i], $b->[$i]);

same here w.r.t method call style

> +	}
> +    }
> +
> +    return 1;
> +}
> +
>  1;