[pve-devel] [PATCH v2 common 1/2] JSONSchema: add format validator support and cleanup check_format

[Stefan Reiter]

On 6/23/20 3:39 PM, Fabian Grünbichler wrote:
> 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;

This would have to be $data, I suppose?

> +
>       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?
> 

Fine by me, though I'm not sure how necessary. It implies that 
validators always validate positively on values they have already 
accepted (and potentially modified) once before though. Also I'm not 
sure I would use the validators result for print_property_string, since 
that means that the value the user passes in might not be the one 
printed (if a validator modifies it) - so maybe call the validator but 
throw away it's result?

> 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.
> 

Sounds good to me, I'll look into it.

> 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.
>