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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id AEB2A99EC9 for ; Tue, 14 Nov 2023 14:45:17 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 91E8931913 for ; Tue, 14 Nov 2023 14:44:47 +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 14:44:46 +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 4FBBD42985 for ; Tue, 14 Nov 2023 14:44:46 +0100 (CET) Date: Tue, 14 Nov 2023 14:44:45 +0100 From: Wolfgang Bumiller To: Dominik Csapak Cc: pve-devel@lists.proxmox.com Message-ID: References: <20231114103340.2850162-1-d.csapak@proxmox.com> <20231114103340.2850162-3-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231114103340.2850162-3-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.102 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [resthandler.pm, clihandler.pm, jsonschema.pm] Subject: Re: [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 13:45:17 -0000 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 they are not optional. > > Signed-off-by: Dominik Csapak > --- > 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);