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

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

[Dominik Csapak]

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

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                 | 117 ++++++-
 src/PVE/RESTHandler.pm                |  82 ++++-
 src/PVE/SectionConfig.pm              | 256 +++++++++++---
 test/Makefile                         |   1 +
 test/section_config_separated_test.pl | 486 ++++++++++++++++++++++++++
 test/section_config_test.pl           | 133 +++++++
 7 files changed, 1006 insertions(+), 71 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 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>
---
 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 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>
---
 src/PVE/CLIHandler.pm  |   2 +-
 src/PVE/JSONSchema.pm  | 117 ++++++++++++++++++++++++++++++++++++++---
 src/PVE/RESTHandler.pm |  82 +++++++++++++++++++++++++++--
 3 files changed, 188 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..61c57b3 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 (scalar(keys $inner_errors->%*) == 0) {
+		    $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,29 @@ 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,
+	    default => 'type',
+	},
 	optional => {
 	    type => "boolean",
 	    description => "This indicates that the instance property in the instance object is not required.",
@@ -1491,6 +1593,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 +1816,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 +1910,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 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).

for that to work there are a few changes in how to use the 'options' hash:
* when wanting to use a property that is not defined in either the local
  list or the global one, the 'type' property has to be given to specify
  which plugin type this should be taken from
* if it's desired to always have some global properties in the schema,
  they have to be given in the 'global' options section now (the perl
  package where the base plugin is defined)

for now, we then save the global options in the section '__global',
which is now a forbidden plugin type, but that should be fine as we
don't use that

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/SectionConfig.pm | 256 ++++++++++++++++++++++++++++++---------
 1 file changed, 198 insertions(+), 58 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 5690476..1e44c3d 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -30,6 +30,9 @@ sub register {
     die "duplicate plugin registration (type = $type)"
 	if defined($pdata->{plugins}->{$type});
 
+    die "invalid plugin type (type = '__global')"
+	if $type eq '__global';
+
     my $plugindata = $class->plugindata();
     $pdata->{plugindata}->{$type} = $plugindata;
     $pdata->{plugins}->{$type} = $class;
@@ -51,6 +54,67 @@ sub plugindata {
     return {};
 }
 
+my sub cmp_property {
+    my ($src, $tgt, $skip_opts) = @_;
+
+    my $merged = {$src->%*, $tgt->%*};
+    for my $opt ($skip_opts->@*) {
+	delete $merged->{$opt};
+    }
+    for my $opt (keys $merged->%*) {
+	return 0 if !defined($src->{$opt}) || !defined($tgt->{$opt}) || $tgt->{$opt} ne $src->{$opt};
+    }
+
+    return 1;
+}
+
+my sub copy_property {
+    my ($src) = @_;
+
+    my $res = {};
+    foreach my $k (keys %$src) {
+	$res->{$k} = $src->{$k};
+    }
+
+    return $res;
+}
+
+
+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) = @_;
 
@@ -58,44 +122,55 @@ sub createSchema {
     my $propertyList = $pdata->{propertyList};
     my $plugins = $pdata->{plugins};
 
-    my $props = $base || {};
-
-    my $copy_property = sub {
-	my ($src) = @_;
+    my $global_options = $pdata->{options}->{__global};
+    my $propertyListSeparated = $pdata->{propertyListSeparated};
 
-	my $res = {};
-	foreach my $k (keys %$src) {
-	    $res->{$k} = $src->{$k};
-	}
+    my $props = $base || {};
 
-	return $res;
-    };
+    if (!defined($global_options)) {
+	foreach my $p (keys %$propertyList) {
+	    next if $skip_type && $p eq 'type';
 
-    foreach my $p (keys %$propertyList) {
-	next if $skip_type && $p eq 'type';
+	    if (!$propertyList->{$p}->{optional}) {
+		$props->{$p} = $propertyList->{$p};
+		next;
+	    }
 
-	if (!$propertyList->{$p}->{optional}) {
-	    $props->{$p} = $propertyList->{$p};
-	    next;
-	}
+	    my $required = 1;
 
-	my $required = 1;
+	    my $copts = $class->options();
+	    $required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
 
-	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 = copy_property($propertyList->{$p});
+		$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 = copy_property($schema);
+		$prop->{'instance-types'} = [$type];
+		$prop->{'type-property'} = 'type';
+		$prop->{optional} = 1 if $opts->{$key}->{optional};
+
+		add_property($props, $key, $prop, $type);
+	    }
+	}
+	for my $opt (keys $global_options->%*) {
+	    $props->{$opt} = copy_property($propertyList->{$opt});
+	    $props->{$opt}->{optional} = 1 if $global_options->{$opt}->{optional};
 	}
     }
 
@@ -113,34 +188,61 @@ sub updateSchema {
     my $propertyList = $pdata->{propertyList};
     my $plugins = $pdata->{plugins};
 
+    my $global_options = $pdata->{options}->{__global};
+    my $propertyListSeparated = $pdata->{propertyListSeparated};
+
     my $props = $base || {};
 
     my $filter_type = $single_class ? $class->type() : undef;
 
-    foreach my $p (keys %$propertyList) {
-	next if $p eq 'type';
+    if (!defined($global_options)) {
+	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};
+	    }
+	    next if !$modifyable;
 
-	foreach my $t (keys %$plugins) {
-	    my $opts = $pdata->{options}->{$t} || {};
-	    next if !defined($opts->{$p});
-	    $modifyable = 1 if !$opts->{$p}->{fixed};
+	    $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 = copy_property($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 $global_options->%*) {
+	    next if $global_options->{$opt}->{fixed};
+
+	    $props->{$opt} = copy_property($propertyList->{$opt});
+	    $props->{$opt}->{optional} = 1 if $global_options->{$opt}->{optional};
+	}
     }
 
     $props->{digest} = get_standard_option('pve-config-digest');
@@ -160,22 +262,33 @@ 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};
     }
 
+    if ($separate_properties) {
+	$pdata->{options}->{__global} = delete $pdata->{options};
+    }
+
     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) {
+		die "duplicate property '$p'" if defined($propertyListSeparated->{$type}->{$p});
+		$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,7 +300,13 @@ 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) {
+		my $opt_type = $opts->{$p}->{type} // $type;
+		$prop = $propertyListSeparated->{$opt_type}->{$p};
+	    }
+	    $prop //= $propertyList->{$p};
+	    die "undefined property '$p'" if !$prop;
 	}
 	$pdata->{options}->{$type} = $opts;
     }
@@ -237,11 +356,13 @@ sub check_value {
     return $value if $key eq 'type' && $type eq $value;
 
     my $opts = $pdata->{options}->{$type};
+    my $global_opts = $pdata->{options}->{__global};
+
     die "unknown section type '$type'\n" if !$opts;
 
-    die "unexpected property '$key'\n" if !defined($opts->{$key});
+    die "unexpected property '$key'\n" if !defined($opts->{$key}) && !defined($global_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 +416,23 @@ 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 $separated = defined($pdata->{options}->{__global});
+
+    my $schema;
+    if ($separated) {
+	my $opt_type = $opts->{$key}->{type} // $type;
+	$schema = $pdata->{propertyListSeparated}->{$opt_type}->{$key};
+    }
+    $schema //= $pdata->{propertyList}->{$key};
+
+    return $schema;
+}
 
 sub parse_config {
     my ($class, $filename, $raw, $allow_unknown) = @_;
@@ -322,7 +460,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 +632,6 @@ sub write_config {
     my ($class, $filename, $cfg, $allow_unknown) = @_;
 
     my $pdata = $class->private();
-    my $propertyList = $pdata->{propertyList};
 
     my $out = '';
 
@@ -545,7 +682,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 +700,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 +709,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 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>
---
 test/Makefile                         |   1 +
 test/section_config_separated_test.pl | 486 ++++++++++++++++++++++++++
 2 files changed, 487 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..234f444
--- /dev/null
+++ b/test/section_config_separated_test.pl
@@ -0,0 +1,486 @@
+#!/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,
+	},
+    },
+    options => {
+	id => {},
+	type => {},
+    },
+};
+
+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,
+	},
+	another => {
+	    description => 'Another field',
+	    type => 'string',
+	},
+	field2 => {
+	    description => 'Field Two',
+	    type => 'integer',
+	    minimum => 10,
+	    maximum => 19,
+	}
+    };
+}
+
+sub options {
+    return {
+	common => { optional => 1 },
+	field1 => {},
+	field2 => {},
+	another => { optional => 1 },
+	arrayfield => { optional => 1, type => 'two' },
+    };
+}
+
+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,
+	},
+	arrayfield => {
+	    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 },
+	field2 => {},
+	another => { type => 'one' },
+	arrayfield => { 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-property' => 'type',
+	    'instance-types' => [ 'one', 'two' ],
+	    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',
+	    'instance-types' => [ 'one', 'two' ],
+	    'type-property' => 'type',
+	    type => 'array',
+	    optional => 1
+	},
+	another => {
+	    optional => 1,
+	    type => 'string',
+	    'type-property' => 'type',
+	    'instance-types' => [ 'one', 'two' ],
+	    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,
+	    'instance-types' => [ 'one', 'two' ],
+	    'type-property' => 'type',
+	    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'
+		    }
+		}
+	    },
+	    'instance-types' => [ 'one', 'two' ],
+	    'type-property' => 'type',
+	    description => 'Array Field with property string'
+	},
+	another => {
+	    description => 'Another field',
+	    'instance-types' => [ 'one', 'two' ],
+	    'type-property' => 'type',
+	    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 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

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

[Wolfgang Bumiller]

mostly LGTM, just minor things

On Tue, Nov 14, 2023 at 11:33:37AM +0100, Dominik Csapak wrote:
> 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  | 117 ++++++++++++++++++++++++++++++++++++++---
>  src/PVE/RESTHandler.pm |  82 +++++++++++++++++++++++++++--
>  3 files changed, 188 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..61c57b3 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -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 (scalar(keys $inner_errors->%*) == 0) {

^ iirc perl can optimize `if (!%$inner_errors)` better

> +		    $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,29 @@ 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,
> +	    default => 'type',

^ AFAICT this default is not the case anymore.

> +	},
>  	optional => {
>  	    type => "boolean",
>  	    description => "This indicates that the instance property in the instance object is not required.",
> @@ -1491,6 +1593,7 @@ my $default_schema = Storable::dclone($default_schema_noref);

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

[Wolfgang Bumiller]

On Tue, Nov 14, 2023 at 11:33:38AM +0100, Dominik Csapak wrote:
> 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).
> 
> for that to work there are a few changes in how to use the 'options' hash:
> * when wanting to use a property that is not defined in either the local
>   list or the global one, the 'type' property has to be given to specify
>   which plugin type this should be taken from
> * if it's desired to always have some global properties in the schema,
>   they have to be given in the 'global' options section now (the perl
>   package where the base plugin is defined)
> 
> for now, we then save the global options in the section '__global',
> which is now a forbidden plugin type, but that should be fine as we
> don't use that
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/SectionConfig.pm | 256 ++++++++++++++++++++++++++++++---------
>  1 file changed, 198 insertions(+), 58 deletions(-)
> 
> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> index 5690476..1e44c3d 100644
> --- a/src/PVE/SectionConfig.pm
> +++ b/src/PVE/SectionConfig.pm
> @@ -30,6 +30,9 @@ sub register {
>      die "duplicate plugin registration (type = $type)"
>  	if defined($pdata->{plugins}->{$type});
>  
> +    die "invalid plugin type (type = '__global')"
> +	if $type eq '__global';
> +
>      my $plugindata = $class->plugindata();
>      $pdata->{plugindata}->{$type} = $plugindata;
>      $pdata->{plugins}->{$type} = $class;
> @@ -51,6 +54,67 @@ sub plugindata {
>      return {};
>  }
>  
> +my sub cmp_property {
> +    my ($src, $tgt, $skip_opts) = @_;

"compare" vs "source/target", I'd argue 'a' and 'b' would be a better
choice for the parameter names.

> +
> +    my $merged = {$src->%*, $tgt->%*};
> +    for my $opt ($skip_opts->@*) {
> +	delete $merged->{$opt};
> +    }
> +    for my $opt (keys $merged->%*) {
> +	return 0 if !defined($src->{$opt}) || !defined($tgt->{$opt}) || $tgt->{$opt} ne $src->{$opt};

^ might be worth checking that they aren't `ref()`s, as you don't want
to `ne` a potential hash here (eg. an `items` in an array schema?)
I'm not sure we have such types in the section config currently, but
IIRC you wanted to add that at some point?
But apart from array/object types we also have `requires => [array]` -
that one in particular should be compared recursively if we use this to
optimize the `instance-types` arrays.

> +    }
> +
> +    return 1;
> +}
> +
> +my sub copy_property {

^ This thing again. 😒

At the very least make its body just:

    return { $_[0]->%* };

Or just kill it.

> +    my ($src) = @_;
> +
> +    my $res = {};
> +    foreach my $k (keys %$src) {
> +	$res->{$k} = $src->{$k};
> +    }
> +
> +    return $res;
> +}
> +
> +
> +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) = @_;
>  
> @@ -58,44 +122,55 @@ sub createSchema {
>      my $propertyList = $pdata->{propertyList};
>      my $plugins = $pdata->{plugins};
>  
> -    my $props = $base || {};
> -
> -    my $copy_property = sub {
> -	my ($src) = @_;
> +    my $global_options = $pdata->{options}->{__global};
> +    my $propertyListSeparated = $pdata->{propertyListSeparated};
>  
> -	my $res = {};
> -	foreach my $k (keys %$src) {
> -	    $res->{$k} = $src->{$k};
> -	}
> +    my $props = $base || {};
>  
> -	return $res;
> -    };
> +    if (!defined($global_options)) {
> +	foreach my $p (keys %$propertyList) {
> +	    next if $skip_type && $p eq 'type';
>  
> -    foreach my $p (keys %$propertyList) {
> -	next if $skip_type && $p eq 'type';
> +	    if (!$propertyList->{$p}->{optional}) {
> +		$props->{$p} = $propertyList->{$p};
> +		next;
> +	    }
>  
> -	if (!$propertyList->{$p}->{optional}) {
> -	    $props->{$p} = $propertyList->{$p};
> -	    next;
> -	}
> +	    my $required = 1;
>  
> -	my $required = 1;
> +	    my $copts = $class->options();
> +	    $required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
>  
> -	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 = copy_property($propertyList->{$p});
> +		$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 = copy_property($schema);
> +		$prop->{'instance-types'} = [$type];
> +		$prop->{'type-property'} = 'type';
> +		$prop->{optional} = 1 if $opts->{$key}->{optional};
> +
> +		add_property($props, $key, $prop, $type);
> +	    }
> +	}
> +	for my $opt (keys $global_options->%*) {
> +	    $props->{$opt} = copy_property($propertyList->{$opt});
> +	    $props->{$opt}->{optional} = 1 if $global_options->{$opt}->{optional};
>  	}
>      }
>  
> @@ -113,34 +188,61 @@ sub updateSchema {
>      my $propertyList = $pdata->{propertyList};
>      my $plugins = $pdata->{plugins};
>  
> +    my $global_options = $pdata->{options}->{__global};
> +    my $propertyListSeparated = $pdata->{propertyListSeparated};
> +
>      my $props = $base || {};
>  
>      my $filter_type = $single_class ? $class->type() : undef;
>  
> -    foreach my $p (keys %$propertyList) {
> -	next if $p eq 'type';
> +    if (!defined($global_options)) {
> +	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};
> +	    }
> +	    next if !$modifyable;
>  
> -	foreach my $t (keys %$plugins) {
> -	    my $opts = $pdata->{options}->{$t} || {};
> -	    next if !defined($opts->{$p});
> -	    $modifyable = 1 if !$opts->{$p}->{fixed};
> +	    $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 = copy_property($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 $global_options->%*) {
> +	    next if $global_options->{$opt}->{fixed};
> +
> +	    $props->{$opt} = copy_property($propertyList->{$opt});
> +	    $props->{$opt}->{optional} = 1 if $global_options->{$opt}->{optional};
> +	}
>      }
>  
>      $props->{digest} = get_standard_option('pve-config-digest');
> @@ -160,22 +262,33 @@ 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};
>      }
>  
> +    if ($separate_properties) {
> +	$pdata->{options}->{__global} = delete $pdata->{options};
> +    }
> +
>      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) {
> +		die "duplicate property '$p'" if defined($propertyListSeparated->{$type}->{$p});

^ Can this even happen? $props is a hash per $type after all, and we're
now namespaced by $type
Should we instead check it against the global options?

> +		$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,7 +300,13 @@ 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) {
> +		my $opt_type = $opts->{$p}->{type} // $type;

^ Similarly, we're coming from a fixed $type and query the `options`
from that very type, does it make sense for there to be a `type`
property present?

> +		$prop = $propertyListSeparated->{$opt_type}->{$p};
> +	    }
> +	    $prop //= $propertyList->{$p};
> +	    die "undefined property '$p'" if !$prop;
>  	}
>  	$pdata->{options}->{$type} = $opts;
>      }
> @@ -237,11 +356,13 @@ sub check_value {
>      return $value if $key eq 'type' && $type eq $value;
>  
>      my $opts = $pdata->{options}->{$type};
> +    my $global_opts = $pdata->{options}->{__global};
> +
>      die "unknown section type '$type'\n" if !$opts;
>  
> -    die "unexpected property '$key'\n" if !defined($opts->{$key});
> +    die "unexpected property '$key'\n" if !defined($opts->{$key}) && !defined($global_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 +416,23 @@ 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 $separated = defined($pdata->{options}->{__global});

^ would it make sense to have this condition as a
`$class->is_separated()` sub?

> +
> +    my $schema;
> +    if ($separated) {
> +	my $opt_type = $opts->{$key}->{type} // $type;
> +	$schema = $pdata->{propertyListSeparated}->{$opt_type}->{$key};
> +    }
> +    $schema //= $pdata->{propertyList}->{$key};
> +
> +    return $schema;
> +}
>  
>  sub parse_config {
>      my ($class, $filename, $raw, $allow_unknown) = @_;
> @@ -322,7 +460,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 +632,6 @@ sub write_config {
>      my ($class, $filename, $cfg, $allow_unknown) = @_;
>  
>      my $pdata = $class->private();
> -    my $propertyList = $pdata->{propertyList};
>  
>      my $out = '';
>  
> @@ -545,7 +682,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 +700,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 +709,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

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

[Wolfgang Bumiller]

On Tue, Nov 14, 2023 at 11:33:39AM +0100, Dominik Csapak wrote:
> 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 | 486 ++++++++++++++++++++++++++
>  2 files changed, 487 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..234f444
> --- /dev/null
> +++ b/test/section_config_separated_test.pl
> @@ -0,0 +1,486 @@
> +#!/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,
> +	},
> +    },
> +    options => {
> +	id => {},
> +	type => {},
> +    },
> +};
> +
> +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,
> +	},
> +	another => {
> +	    description => 'Another field',
> +	    type => 'string',
> +	},
> +	field2 => {
> +	    description => 'Field Two',
> +	    type => 'integer',
> +	    minimum => 10,
> +	    maximum => 19,
> +	}
> +    };
> +}
> +
> +sub options {
> +    return {
> +	common => { optional => 1 },
> +	field1 => {},
> +	field2 => {},
> +	another => { optional => 1 },
> +	arrayfield => { optional => 1, type => 'two' },

Oh...
*That's* how you use `type` here...
Can we not?
I'd *really* prefer to just have `arrayfield` copied into the properties
right here.

Changing an existing property currently requires us to consider all the
types it appears in.
I thought we could now have *distinct* schemas for them without *also*
having to worry about potential reuse from somewhere else :S