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

[Stoiko Ivanov]

On Fri, 22 Nov 2019 19:37:20 +0100
Thomas Lamprecht <t.lamprecht at proxmox.com> 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.

do agree - fwiw - this check helps in the CLIHandler e.g. for 
`pmgconfig dkim_record` (after manually adding a trivial cycle)
But this is pathologic and  - However this is also not caught during
build-time (we check for cycles in the provided data - not the schema)

keeping the cycle check while building does have merit if someone
start plugging hash-references (without copying) in the api-definitions
and eventually a cycle forms IIUC

summing-up: looks good to me!


> 
> 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};
> +    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 {