Re: [PATCH pve-manager] fix #7942: pvesh: treat schema without properties as empty object

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

On Tue, Sep 01, 2026 at 05:32:04PM +0200, Wolfgang Bumiller wrote:
> On Wed, Aug 19, 2026 at 11:50:23AM +0200, Arthur Bied-Charreton wrote:
> > merge_properties() dispatches on the shape of the target schema by
> > merging into `properties` if present, appending to an `allOf` list, or
> > wrapping a `oneOf` into a new `allOf`. A schema with none of those
> > entries died on the assumption that it is not an object.
> > 
> > A lot of endpoints (e.g., /cluster/firewall/options,
> > /cluster/notifications/matcher-field-values) across our api define the
> > parameters schema as follows:
> > 
> >     { additionalProperties => 0 }
> > 
> > Before 874d160f317a, pvesh treated this as a valid schema allowing no
> > parameters. Since that commit, it fails while loading with the
> > following error:
> > 
> >     unknown schema type (not an object?)
> > 
> > Split the fallback branch into the three cases that can reach it: die
> > with the offending type if the schema is explicitly not an object, die
> > with the offending keys if it contains unexpected keys, otherwise treat
> > `$to` as a valid, empty object and populate `properties` directly.
> > 
> > Fixes: 874d160f317a ("pvesh: support for allOf/oneOf parameter schemas")
> > Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7942
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> >  PVE/CLI/pvesh.pm | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/PVE/CLI/pvesh.pm b/PVE/CLI/pvesh.pm
> > index 0ff481d4..9568e22b 100755
> > --- a/PVE/CLI/pvesh.pm
> > +++ b/PVE/CLI/pvesh.pm
> > @@ -304,6 +304,12 @@ my $our_properties = {
> >      },
> >  };
> >  
> > +my sub unknown_schema_keys {
> > +    my ($schema) = @_;
> > +
> > +    return [grep { $_ ne 'additionalProperties' && $_ ne 'type' } keys $schema->%*];
> > +}
> 
> This does not need to be its own sub.
> 
fair enough, will inline it in v2
> > +
> >  my sub merge_properties {
> >      my ($from, $to) = @_;
> >  
> > @@ -323,7 +329,14 @@ my sub merge_properties {
> >          my $old = { $to->%* };
> >          $to->%* = (allOf => [$old, $from]);
> >      } else {
> > -        die "unknown schema type (not an object?)\n";
> > +        die "unknown schema type '$to->{type}' (not an object)\n"
> > +            if defined($to->{type}) && $to->{type} ne 'object';
> > +
> > +        my $unknown = unknown_schema_keys($to);
> > +        die "unknown schema type, found unexpected keys: " . join(', ', sort($unknown->@*)) . "\n"
> > +            if $unknown->@*;
> 
> I'm not convinced a list like this is the best way to go here, but it
> also won't hurt I suppose.
i'm not very set on this, just thought it would make sense to include
the offending keys in the error message. do you have another option in
mind? 
> Either way - please just inline the helper sub, it's a quite specific
> part of this check and message.
ack, see above
> 
> > +
> > +        $to->{properties} = { $from->{properties}->%* };
> >      }
> >  }
> >  
> > -- 
> > 2.47.3