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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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);