From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 1D47399790 for ; Tue, 14 Nov 2023 11:33:44 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DD6AA1DEA7 for ; Tue, 14 Nov 2023 11:33:43 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 14 Nov 2023 11:33:42 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 62FA042860 for ; Tue, 14 Nov 2023 11:33:42 +0100 (CET) From: Dominik Csapak To: pve-devel@lists.proxmox.com Date: Tue, 14 Nov 2023 11:33:37 +0100 Message-Id: <20231114103340.2850162-3-d.csapak@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20231114103340.2850162-1-d.csapak@proxmox.com> References: <20231114103340.2850162-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.017 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: [pve-devel] [PATCH common 2/4] json schema: implement 'oneOf' schema X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Nov 2023 10:33:44 -0000 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 --- 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