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.csa...@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);
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
