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

[Fabian Grünbichler]

On June 24, 2020 10:54 am, Stefan Reiter wrote:
> 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?

yes, sorry!

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

well, it's not a given at all that $data is something that was 
previously/originally returned by parse_property_string. but yes, IMHO 
every object that we transform into a property string should pass the 
checks that that property string parsed back into an object passes (what 
a sentence -.-). or in other words, a validator should not transform a 
valid value into an invalid one ;)

I was not sure whether to include modifications done by the validator or 
not, but I'd tend to include them just to make the interface simpler. 
otherwise we'd have to dclone $data, because even if we throwaway the 
return value $data might have been modified. also, the question is 
whether to run the validator before or after check_object. for almost 
all cases it probably does not make a difference, so we might also just 
re-visit that if we ever find the need to switch the order around.

the main use case for a validator that modifies is probably to enforce a 
single/newer variant where the format itself would accept multiple, and 
that is something that does not hurt in the opposite direction either.

in any case the whole mechanism should probably be documented next to 
register_format's declaration.

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