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

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

[Dominik Csapak]

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

only change is the fixed test for pve-common 3/3
(did accidentally include the wrong file when sending the patches)

pve-common:

Dominik Csapak (5):
  section config: add test for the schemas
  tools: add is_deeply
  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              | 312 +++++++++++++---
 src/PVE/Tools.pm                      |  31 ++
 test/Makefile                         |   2 +
 test/is_deeply_test.pl                | 142 ++++++++
 test/section_config_separated_test.pl | 489 ++++++++++++++++++++++++++
 test/section_config_test.pl           | 133 +++++++
 9 files changed, 1239 insertions(+), 70 deletions(-)
 create mode 100755 test/is_deeply_test.pl
 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 v4 1/5] 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>
---
 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 v4 2/5] tools: add is_deeply

[Dominik Csapak]

to compare nested hashes/lists and scalar values recursively.
Also includes some tests

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v3:
* rename the testfile is_deeply_test to match Makefile
 src/PVE/Tools.pm       |  31 +++++++++
 test/Makefile          |   1 +
 test/is_deeply_test.pl | 142 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+)
 create mode 100755 test/is_deeply_test.pl

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index b3af2c6..766c809 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -2150,4 +2150,35 @@ sub get_file_hash {
     return lc($digest);
 }
 
+# compare two perl variables recursively, so this works for scalars, nested
+# hashes and nested arrays
+sub is_deeply {
+    my ($a, $b) = @_;
+
+    return 0 if defined($a) != defined($b);
+    return 1 if !defined($a); # both are undef
+
+    my ($ref_a, $ref_b) = (ref($a), ref($b));
+
+    # scalar case
+    return 0 if !$ref_a && !$ref_b && "$a" ne "$b";
+
+    # different types, ok because ref never returns undef, only empty string
+    return 0 if $ref_a ne $ref_b;
+
+    if ($ref_a eq 'HASH') {
+	return 0 if scalar(keys $a->%*) != scalar(keys $b->%*);
+	for my $opt (keys $a->%*) {
+	    return 0 if !is_deeply($a->{$opt}, $b->{$opt});
+	}
+    } elsif ($ref_a eq 'ARRAY') {
+	return 0 if scalar($a->@*) != scalar($b->@*);
+	for (my $i = 0; $i < $a->@*; $i++) {
+	    return 0 if !is_deeply($a->[$i], $b->[$i]);
+	}
+    }
+
+    return 1;
+}
+
 1;
diff --git a/test/Makefile b/test/Makefile
index 82f40ab..b0de1a5 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		\
+	is_deeply_test.test		\
 
 all:
 
diff --git a/test/is_deeply_test.pl b/test/is_deeply_test.pl
new file mode 100755
index 0000000..f546b36
--- /dev/null
+++ b/test/is_deeply_test.pl
@@ -0,0 +1,142 @@
+#!/usr/bin/perl
+
+use lib '../src';
+
+use strict;
+use warnings;
+
+use Test::More;
+use PVE::Tools;
+
+my $tests = [
+    {
+	name => 'both undef',
+	a => undef,
+	b => undef,
+	expected => 1,
+    },
+    {
+	name => 'empty string',
+	a => '',
+	b => '',
+	expected => 1,
+    },
+    {
+	name => 'empty string and undef',
+	a => '',
+	b => undef,
+	expected => 0,
+    },
+    {
+	name => '0 and undef',
+	a => 0,
+	b => undef,
+	expected => 0,
+    },
+    {
+	name => 'equal strings',
+	a => 'test',
+	b => 'test',
+	expected => 1,
+    },
+    {
+	name => 'unequal strings',
+	a => 'test',
+	b => 'tost',
+	expected => 0,
+    },
+    {
+	name => 'equal numerics',
+	a => 42,
+	b => 42,
+	expected => 1,
+    },
+    {
+	name => 'unequal numerics',
+	a => 42,
+	b => 420,
+	expected => 0,
+    },
+    {
+	name => 'equal arrays',
+	a => ['foo', 'bar'],
+	b => ['foo', 'bar'],
+	expected => 1,
+    },
+    {
+	name => 'equal empty arrays',
+	a => [],
+	b => [],
+	expected => 1,
+    },
+    {
+	name => 'unequal arrays',
+	a => ['foo', 'bar'],
+	b => ['bar', 'foo'],
+	expected => 0,
+    },
+    {
+	name => 'equal empty hashes',
+	a => { },
+	b => { },
+	expected => 1,
+    },
+    {
+	name => 'equal hashes',
+	a => { foo => 'bar' },
+	b => { foo => 'bar' },
+	expected => 1,
+    },
+    {
+	name => 'unequal hashes',
+	a => { foo => 'bar' },
+	b => { bar => 'foo' },
+	expected => 0,
+    },
+    {
+	name => 'equal nested hashes',
+	a => {
+	    foo => 'bar',
+	    bar => 1,
+	    list => ['foo', 'bar'],
+	    properties => {
+		baz => 'boo',
+	    },
+	},
+	b => {
+	    foo => 'bar',
+	    bar => 1,
+	    list => ['foo', 'bar'],
+	    properties => {
+		baz => 'boo',
+	    },
+	},
+	expected => 1,
+    },
+    {
+	name => 'unequal nested hashes',
+	a => {
+	    foo => 'bar',
+	    bar => 1,
+	    list => ['foo', 'bar'],
+	    properties => {
+		baz => 'boo',
+	    },
+	},
+	b => {
+	    foo => 'bar',
+	    bar => 1,
+	    list => ['foo', 'bar'],
+	    properties => {
+		baz => undef,
+	    },
+	},
+	expected => 0,
+    },
+];
+
+for my $test ($tests->@*) {
+    is (PVE::Tools::is_deeply($test->{a}, $test->{b}), $test->{expected}, $test->{name});
+}
+
+done_testing();
-- 
2.30.2

[pve-devel] [PATCH common v4 3/5] 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>
---
 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 v4 4/5] 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>
---
 src/PVE/SectionConfig.pm | 312 ++++++++++++++++++++++++++++++++-------
 1 file changed, 255 insertions(+), 57 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 5690476..43575ff 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1,3 +1,56 @@
+# This pacakge provides a way to have multiple (often similar) types of entries
+# in the same config file, each in its own section, thus Section Config
+#
+# The intended structure is to have a single 'base' plugin that inherits from
+# this class and provides meaningful defaults in its '$defaultData', e.g. a
+# default list of properties in its propertyList (most often only 'id' and
+# 'type')
+#
+# Each 'real' plugin then has it's own package that should inherit from the
+# 'base' plugin and returns it's wanted properties in the 'properties' method,
+# its type in the 'type' method and the used options in the 'options' method.
+# The options method declares if the property is 'optional' or 'fixed' like so:
+#
+# ````
+# sub options {
+#     return {
+#         optional1 => { optional => 1 },
+#         fixed1 => { fixed => 1 },
+#         requiredButNotFixed => {},
+#     };
+# }
+# ```
+#
+# 'fixed' options can be set on create, but not changed afterwards.
+#
+# To actually use it, you have to register the plugins and init the 'base'
+# plugin, like so:
+#
+# ```
+# PVE::Dummy::Plugin1->register();
+# PVE::Dummy::Plugin2->register();
+# PVE::Dummy::Plugin3->register();
+# PVE::Dummy::BasePlugin->init();
+# ```
+#
+# There are two modes in how to use it, the 'unified' mode and the 'isolated'
+# mode. In the default unified mode, there is only a global list of properties
+# which the plugins can use, so you cannot define the same propertyname twice
+# in different plugins. Reason for this is to force the use of identical
+# properties for multiple plugins.
+#
+# The second way is to use the 'isolated' mode, which can be achieved by
+# calling init with `1` as its parameter like this:
+#
+# ```
+# PVE::Dummy::BasePlugin->init(1);
+# ```
+#
+# With this, each plugin get's their own isolated list of properties which it
+# can use. Note that in this mode, you only have to specify the property in the
+# options method when it is either 'fixed' or comes from the global list of
+# properties. All locally defined ones get automatically added to the schema
+# for that plugin.
 package PVE::SectionConfig;
 
 use strict;
@@ -8,6 +61,7 @@ use Digest::SHA;
 
 use PVE::Exception qw(raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::Tools;
 
 my $defaultData = {
     options => {},
@@ -51,6 +105,62 @@ sub plugindata {
     return {};
 }
 
+sub has_isolated_properties {
+    my ($class) = @_;
+
+    my $isolatedPropertyList = $class->private()->{isolatedPropertyList};
+
+    return defined($isolatedPropertyList) && scalar(keys $isolatedPropertyList->%*) > 0;
+}
+
+my sub compare_property {
+    my ($a, $b, $skip_opts) = @_;
+
+    my $merged = {$a->%*, $b->%*};
+    delete $merged->{$_} for $skip_opts->@*;
+
+    for my $opt (keys $merged->%*) {
+	return 0 if !PVE::Tools::is_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 (compare_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 (compare_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 +170,61 @@ sub createSchema {
 
     my $props = $base || {};
 
-    my $copy_property = sub {
-	my ($src) = @_;
+    if (!$class->has_isolated_properties()) {
+	foreach my $p (keys %$propertyList) {
+	    next if $skip_type && $p eq 'type';
 
-	my $res = {};
-	foreach my $k (keys %$src) {
-	    $res->{$k} = $src->{$k};
-	}
+	    if (!$propertyList->{$p}->{optional}) {
+		$props->{$p} = $propertyList->{$p};
+		next;
+	    }
 
-	return $res;
-    };
+	    my $required = 1;
 
-    foreach my $p (keys %$propertyList) {
-	next if $skip_type && $p eq 'type';
+	    my $copts = $class->options();
+	    $required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
 
-	if (!$propertyList->{$p}->{optional}) {
-	    $props->{$p} = $propertyList->{$p};
-	    next;
-	}
-
-	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 +246,61 @@ sub updateSchema {
 
     my $filter_type = $single_class ? $class->type() : undef;
 
-    foreach my $p (keys %$propertyList) {
-	next if $p eq 'type';
+    if (!$class->has_isolated_properties()) {
+	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);
+	    }
+	}
+
+	for my $opt (keys $propertyList->%*) {
+	    next if $props->{$opt};
+	    $props->{$opt} = {$propertyList->{$opt}->%*};
 	}
-	next if !$modifyable;
 
-	$props->{$p} = $propertyList->{$p};
+	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 +320,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 isolatedPropertyList)) {
 	$pdata->{$k} = {} if !$pdata->{$k};
     }
 
     my $plugins = $pdata->{plugins};
     my $propertyList = $pdata->{propertyList};
+    my $isolatedPropertyList = $pdata->{isolatedPropertyList};
 
     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 = $isolatedPropertyList->{$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 +353,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 = $isolatedPropertyList->{$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 $isolatedPropertyList->{$type}->%*) {
+		next if $opts->{$p};
+		$opts->{$p} = {};
+		$opts->{$p}->{optional} = 1 if $isolatedPropertyList->{$type}->{$p}->{optional};
+	    }
 	}
+
 	$pdata->{options}->{$type} = $opts;
     }
 
@@ -241,7 +422,7 @@ sub check_value {
 
     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 +476,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->has_isolated_properties()) {
+	$schema = $pdata->{isolatedPropertyList}->{$type}->{$key};
+    }
+    $schema //= $pdata->{propertyList}->{$key};
+
+    return $schema;
+}
 
 sub parse_config {
     my ($class, $filename, $raw, $allow_unknown) = @_;
@@ -322,7 +517,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 +689,6 @@ sub write_config {
     my ($class, $filename, $cfg, $allow_unknown) = @_;
 
     my $pdata = $class->private();
-    my $propertyList = $pdata->{propertyList};
 
     my $out = '';
 
@@ -516,6 +710,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 +740,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 +758,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 +767,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";
-- 
2.30.2

[pve-devel] [PATCH common v4 5/5] 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>
---
 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 b0de1a5..30ad185 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -7,6 +7,7 @@ TESTS = lock_file.test			\
 	section_config_test.test	\
 	api_parameter_test.test		\
 	is_deeply_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 v4 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>
---
 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

[pve-devel] applied: [PATCH common/widget-toolkit v4] implement oneOf schema

[Thomas Lamprecht]

Am 16/11/2023 um 16:21 schrieb Dominik Csapak:
> this series implementes the oneOf schema for the api, see the individual
> patches for more details and changelog
> 
> only change is the fixed test for pve-common 3/3
> (did accidentally include the wrong file when sending the patches)
> 
> pve-common:
> 
> Dominik Csapak (5):
>   section config: add test for the schemas
>   tools: add is_deeply
>   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              | 312 +++++++++++++---
>  src/PVE/Tools.pm                      |  31 ++
>  test/Makefile                         |   2 +
>  test/is_deeply_test.pl                | 142 ++++++++
>  test/section_config_separated_test.pl | 489 ++++++++++++++++++++++++++
>  test/section_config_test.pl           | 133 +++++++
>  9 files changed, 1239 insertions(+), 70 deletions(-)
>  create mode 100755 test/is_deeply_test.pl
>  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(-)
> 


applied series, thanks!

As talked off-list, I did a few small squashes to ensure that the isolation (vs.
separated) terminology is used consistently, and changed the interface from passing
an, a bit opaque, "boolean" to enable the property isolation, to a more telling
property_isolation => 1 hash entry.