[pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config
Thomas Lamprecht
t.lamprecht at proxmox.com
Tue Jun 6 11:12:20 CEST 2023
Am 06/06/2023 um 10:39 schrieb Dominik Csapak:
> a few things were missing for it to work:
> * on the cli, we have to get the option as an array if the type is an
> array
> * the untainting must be done recursively, otherwise, the regex matching
> converts an array hash into the string 'ARRAY(0x123412341234)'
> * JSONSchema::parse_config did not handle array formats specially, but
> we want to allow to specify them multiple time
> * the biggest point: in the RESTHandler, to be compatible with the
> current gui behavior, we have to rewrite two parameter types:
> - when the api defines a '-list' format for a string type, but we get
> a list (because of the changes in http-server), we join the list
> with a comma into a string
> - when the api defines an 'array' type, but we get a scalar value,
> wrap the value in an array (because for www-form-urlencoded, you
> cannot send an array with a single value) add tests for this
> behavior, some of which we want to deprecate and remove in the
> future
>
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> changes from v1:
> * include wolfangs feedback
> * include auto-conversion from string <-> list where appropriate and add
> tests for it
>
> src/PVE/JSONSchema.pm | 12 +++++
> src/PVE/RESTHandler.pm | 61 ++++++++++++++++++----
> test/Makefile | 9 +++-
> test/api_parameter_test.pl | 100 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 172 insertions(+), 10 deletions(-)
> create mode 100755 test/api_parameter_test.pl
>
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 527e409..526fc2b 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -1709,6 +1709,8 @@ sub get_options {
> } else {
> if ($pd->{format} && $pd->{format} =~ m/-a?list/) {
> push @getopt, "$prop=s@";
> + } elsif ($pd->{type} eq 'array') {
> + push @getopt, "$prop=s@";
> } else {
> push @getopt, "$prop=s";
> }
> @@ -1869,6 +1871,16 @@ sub parse_config : prototype($$$;$) {
>
> $value = parse_boolean($value) // $value;
> }
> + if ($schema->{properties}->{$key} &&
> + $schema->{properties}->{$key}->{type} eq 'array') {
code style, and can be fixed up:
for multi-line if's place the closing parenthesis and opening block { on it's own line:
It also doesn't hurt to move all expressions part of the condition in a separate line
(albeit that part is not a rule in our style guide):
if (
$schema->{properties}->{$key}
&& $schema->{properties}->{$key}->{type} eq 'array'
) {
# ...
> +
> + if (defined($cfg->{$key})) {
> + push $cfg->{$key}->@*, $value;
> + } else {
> + $cfg->{$key} = [$value];
> + }
Could be written shorter, but just fine as above
$cfg->{$key} //= [];
push $cfg->{$key}->@*, $value;
> + next;
> + }
> $cfg->{$key} = $value;
> } else {
> warn "ignore config line: $line\n"
> diff --git a/src/PVE/RESTHandler.pm b/src/PVE/RESTHandler.pm
> index db86af2..369e302 100644
> --- a/src/PVE/RESTHandler.pm
> +++ b/src/PVE/RESTHandler.pm
> @@ -426,6 +426,56 @@ sub find_handler {
> return ($handler_class, $method_info);
> }
>
> +my $untaint_recursive;
I got flash backs w.r.t. refcount cycles here keeping all variables, and thus memory
inside the body alive forever, don't we need a weaken?
E.g., like we had to do in PVE::Status::Graphite's assemble.
> +
> +$untaint_recursive = sub {
> + my ($param) = @_;
> +
> + my $ref = ref($param);
> + if ($ref eq 'HASH') {
> + $param->{$_} = $untaint_recursive->($param->{$_}) for keys $param->%*;
> + } elsif ($ref eq 'ARRAY') {
> + for (my $i = 0; $i < scalar($param->@*); $i++) {
> + $param->[$i] = $untaint_recursive->($param->[$i]);
> + }
> + } else {
> + if (defined($param)) {
could be merged into upper branch as elsif, but no hard feelings.
> + my ($newval) = $param =~ /^(.*)$/s;
> + $param = $newval;
> + }
> + }
> +
> + return $param;
> +};
> +
> +# convert arrays to strings where we expect a '-list' format and convert scalar
> +# values to arrays when we expect an array (because of www-form-urlencoded)
> +#
> +# only on the top level, since www-form-urlencoded cannot be nested anyway
> +#
> +# FIXME: change gui/api calls to not rely on this during 8.x, mark the
> +# behaviour deprecated with 9.x, and remove it with 10.x
> +my $convert_params = sub { my ($param, $schema) = @_;
please keep the method paramethers on it's own line.
Also, maybe go for a more telling names, as convert_params could mean everytrhing
and nothing ^^
> +
> + return if !$schema->{properties};
> + return if (ref($param) // '') ne 'HASH';
doesn't this breaks the assignment when used below? I.e.,:
$param = $convert_params->($param, $schema);
or messes with silenting parameters sent to a endpoint without properties, which would
create an extra param error otherwise?
> +
> + for my $key (keys $schema->{properties}->%*) {
> + if (my $value = $param->{$key}) {
> + my $type = $schema->{properties}->{$key}->{type} // '';
> + my $format = $schema->{properties}->{$key}->{format} // '';
> + my $ref = ref($value);
> + if ($ref eq 'ARRAY' && $type eq 'string' && $format =~ m/-list$/) {
Should this also check ref to not be undef, i.e.
if ($ref && $ref eq 'ARRAY' && ...
> + $param->{$key} = join(',', $value->@*);
> + } elsif (!$ref && $type eq 'array') {
> + $param->{$key} = [$value];
> + }
> + }
> + }
> +
> + return $param;
> +};
> +
> sub handle {
> my ($self, $info, $param, $result_verification) = @_;
>
> @@ -437,17 +487,10 @@ sub handle {
>
> if (my $schema = $info->{parameters}) {
> # warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
> + $param = $convert_params->($param, $schema);
> PVE::JSONSchema::validate($param, $schema);
> # untaint data (already validated)
> - my $extra = delete $param->{'extra-args'};
> - while (my ($key, $val) = each %$param) {
> - if (defined($val)) {
> - ($param->{$key}) = $val =~ /^(.*)$/s;
> - } else {
> - $param->{$key} = undef;
> - }
> - }
> - $param->{'extra-args'} = [map { /^(.*)$/ } @$extra] if $extra;
> + $param = $untaint_recursive->($param);
> }
>
> my $result = $func->($param); # the actual API code execution call
More information about the pve-devel
mailing list