[pve-devel] [PATCH common v4 3/5] json schema: implement 'oneOf' schema

[Dominik Csapak <d.csapak@proxmox.com>]

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