LGTM, what do you think about the following follow-up:
------8<-----
diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index b2ba9f7..d28143d 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -1878,9 +1878,12 @@ sub generate_typetext {
sub print_property_string {
my ($data, $format, $skip, $path) = @_;
+ my $validator;
if (ref($format) ne 'HASH') {
my $schema = get_format($format);
die "not a valid format: $format\n" if !$schema;
+ # named formats can have validators attached
+ $validator = $format_validators->{$format};
$format = $schema;
}
@@ -1890,6 +1893,8 @@ sub print_property_string {
raise "format error", errors => $errors;
}
+ $res = $validator->($res) if $validator;
+
my ($default_key, $keyAliasProps) = &$find_schema_default_key($format);
my $res = '';
----->8------
to ensure our code calling 'print_property_string' also adheres to the
format the validator enforces?
it might also be nice to mark formats with a validator (at least for the
API dump) to make it obvious that the displayed format might be further
restricted.
I went through the code paths (again) and still think it might be
worthwhile to extend named formats to check_object as well, instead of
just scalar property string values. but that is a bigger follow-up, for
now limiting the scope of these validators to just property strings
seems sensible.
On June 17, 2020 4:44 pm, Stefan Reiter wrote:
> Adds a third, optional parameter to register_format that allows specifying
> a function that will be called after parsing and can validate the parsed
> data. A validator should die on failed validation, and can also change the
> parsed object by returning a modified version of it.
>
> This is useful so one can register a format with its hash, thus allowing
> documentation to be generated automatically, while still enforcing certain
> validation rules.
>
> The validator only needs to be called in parse_property_string, since
> check_format always calls parse_property_string if there is a
> possibility of a validator existing at all. parse_property_string should
> then be called with named formats for best effect, as only then can
> validators be used.
>
> Clean up 'check_format' as well (which pretty much amounts to a rewrite).
> No existing functionality is intentionally changed.
>
> Signed-off-by: Stefan Reiter <s.rei...@proxmox.com>
> ---
> src/PVE/JSONSchema.pm | 87 +++++++++++++++++++++++++++----------------
> 1 file changed, 55 insertions(+), 32 deletions(-)
>
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 84fb694..f987006 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -121,19 +121,26 @@ register_standard_option('pve-snapshot-name', {
> });
>
> my $format_list = {};
> +my $format_validators = {};
>
> sub register_format {
> - my ($format, $code) = @_;
> + my ($name, $format, $validator) = @_;
>
> - die "JSON schema format '$format' already registered\n"
> - if $format_list->{$format};
> + die "JSON schema format '$name' already registered\n"
> + if $format_list->{$name};
>
> - $format_list->{$format} = $code;
> + if ($validator) {
> + die "A \$validator function can only be specified for hash-based
> formats\n"
> + if ref($format) ne 'HASH';
> + $format_validators->{$name} = $validator;
> + }
> +
> + $format_list->{$name} = $format;
> }
>
> sub get_format {
> - my ($format) = @_;
> - return $format_list->{$format};
> + my ($name) = @_;
> + return $format_list->{$name};
> }
>
> my $renderer_hash = {};
> @@ -647,39 +654,47 @@ sub pve_verify_tfa_secret {
> sub check_format {
> my ($format, $value, $path) = @_;
>
> - return parse_property_string($format, $value, $path) if ref($format) eq
> 'HASH';
> - return if $format eq 'regex';
> + if (ref($format) eq 'HASH') {
> + # hash ref cannot have validator/list/opt handling attached
> + return parse_property_string($format, $value, $path);
> + }
>
> - if ($format =~ m/^(.*)-a?list$/) {
> + if (ref($format) eq 'CODE') {
> + # we are the (sole, old-style) validator
> + return $format->($value);
> + }
> +
> + return if $format eq 'regex';
>
> - my $code = $format_list->{$1};
> + my $parsed;
> + $format =~ m/^(.*?)(?:-a?(list|opt))?$/;
> + my ($format_name, $format_type) = ($1, $2 // 'none');
> + my $registered = get_format($format_name);
> + die "undefined format '$format'\n" if !$registered;
>
> - die "undefined format '$format'\n" if !$code;
> + die "'-$format_type' format must have code ref, not hash\n"
> + if $format_type ne 'none' && ref($registered) ne 'CODE';
>
> + if ($format_type eq 'list') {
> # Note: we allow empty lists
> foreach my $v (split_list($value)) {
> - &$code($v);
> + $parsed = $registered->($v);
> }
> -
> - } elsif ($format =~ m/^(.*)-opt$/) {
> -
> - my $code = $format_list->{$1};
> -
> - die "undefined format '$format'\n" if !$code;
> -
> - return if !$value; # allow empty string
> -
> - &$code($value);
> -
> + } elsif ($format_type eq 'opt') {
> + $parsed = $registered->($value) if $value;
> } else {
> -
> - my $code = $format_list->{$format};
> -
> - die "undefined format '$format'\n" if !$code;
> -
> - return parse_property_string($code, $value, $path) if ref($code) eq
> 'HASH';
> - &$code($value);
> + if (ref($registered) eq 'HASH') {
> + # Note: this is the only case where a validator function could be
> + # attached, hence it's safe to handle that in parse_property_string.
> + # We do however have to call it with $format_name instead of
> + # $registered, so it knows about the name (and thus any validators).
> + $parsed = parse_property_string($format, $value, $path);
> + } else {
> + $parsed = $registered->($value);
> + }
> }
> +
> + return $parsed;
> }
>
> sub parse_size {
> @@ -735,9 +750,16 @@ sub parse_property_string {
> $additional_properties = 0 if !defined($additional_properties);
>
> # Support named formats here, too:
> + my $validator;
> if (!ref($format)) {
> - if (my $desc = $format_list->{$format}) {
> - $format = $desc;
> + if (my $reg = get_format($format)) {
> + die "parse_property_string only accepts hash based named formats\n"
> + if ref($reg) ne 'HASH';
> +
> + # named formats can have validators attached
> + $validator = $format_validators->{$format};
> +
> + $format = $reg;
> } else {
> die "unknown format: $format\n";
> }
> @@ -793,6 +815,7 @@ sub parse_property_string {
> raise "format error\n", errors => $errors;
> }
>
> + return $validator->($res) if $validator;
> return $res;
> }
>
> --
> 2.20.1
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
