[pve-devel] [PATCH common v2] schema: only check for cycles during build

[Thomas Lamprecht]

On 11/25/19 11:21 AM, Fabian Grünbichler wrote:
> On November 22, 2019 7:37 pm, Thomas Lamprecht wrote:
>> Do not check for cycles or for self-validation if not in a build
>> environment.
>>
>> The slight drawback is that we also avoid this cycle checks when we
>> do (development) testing through the API and don't remeber to set the
>> PROXMOX_FORCE_SCHEMA_VALIDATION environment variable.
>>
>> But honestyl, I never had a cycle in the API and got noticed by
>> *this* check, as then to_json and the behavior were all the pointers
>> needed to get this.
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>>
>> This time a bit more thought out (thanks Stoiko!)
>>
>>  src/PVE/JSONSchema.pm | 38 +++++++++++++++++++++++++-------------
>>  1 file changed, 25 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
>> index 51c3881..916f703 100644
>> --- a/src/PVE/JSONSchema.pm
>> +++ b/src/PVE/JSONSchema.pm
>> @@ -6,13 +6,21 @@ use Storable; # for dclone
>>  use Getopt::Long;
>>  use Encode::Locale;
>>  use Encode;
>> -use Devel::Cycle -quiet; # todo: remove?
>>  use PVE::Tools qw(split_list $IPV6RE $IPV4RE);
>>  use PVE::Exception qw(raise);
>>  use HTTP::Status qw(:constants);
>>  use Net::IP qw(:PROC);
>>  use Data::Dumper;
>>  
>> +my $do_build_checks;
>> +BEGIN {
>> +    $do_build_checks = $ENV{DEB_BUILD_ARCH} || $ENV{PROXMOX_FORCE_SCHEMA_VALIDATION};
> 
> couldn't we set 'PROXMOX_FORCE_SCHEMA_VALIDATION' in pve-doc-generator's 
> *api-verified targets and drop DEB_BUILD_ARCH here? seems a bit 
> arbitrary, and who knows what exports DEB_BUILD_ARCH into production 
> environments..

Not really realistic on a PVE machine, especially if not DEB, and in
the "worst" case you're as good as we're now - i.e., do some additional
checks and get taught to not system wide or (for root) export
DEB_BUILD_ARCH ;)

I don't want another versioned dependency bump over the whole three,
just to ensure that the cycle checks and initial self-tests are done..
This was re-using something existing.

> 
>> +    if ($do_build_checks) {
>> +	require Devel::Cycle;
>> +	import Devel::Cycle -quiet;
>> +    }
>> +}
>> +
>>  use base 'Exporter';
>>  
>>  our @EXPORT_OK = qw(
>> @@ -1035,14 +1043,16 @@ sub validate {
>>      my $errors = {};
>>      $errmsg = "Parameter verification failed.\n" if !$errmsg;
>>  
>> -    # todo: cycle detection is only needed for debugging, I guess
>> -    # we can disable that in the final release
>> -    # todo: is there a better/faster way to detect cycles?
>> -    my $cycles = 0;
>> -    find_cycle($instance, sub { $cycles = 1 });
>> -    if ($cycles) {
>> -	add_error($errors, undef, "data structure contains recursive cycles");
>> -    } elsif ($schema) {
>> +    # only check when building a package or told to do so, this is costly
>> +    if ($do_build_checks) {
>> +	my $cycles = 0;
>> +	find_cycle($instance, sub { $cycles = 1 });
>> +	if ($cycles) {
>> +	    add_error($errors, undef, "data structure contains recursive cycles");
>> +	}
>> +    }
>> +
>> +    if ($schema) {
>>  	check_prop($instance, $schema, '', $errors);
>>      }
>>  
>> @@ -1400,10 +1410,12 @@ sub validate_method_info {
>>      validate_schema($info->{returns}) if $info->{returns};
>>  }
>>  
>> -# run a self test on load
>> -# make sure we can verify the default schema
>> -validate_schema($default_schema_noref);
>> -validate_schema($method_schema);
>> +if ($do_build_checks) {
>> +    # run a self test on load when building
>> +    # make sure we can verify the default schema
>> +    validate_schema($default_schema_noref);
>> +    validate_schema($method_schema);
>> +}
>>  
>>  # and now some utility methods (used by pve api)
>>  sub method_get_child_link {
>> -- 
>> 2.20.1