Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

while this is already applied, some comments inline, for a possible next
time, and also the big
question if this is even required, after all I can just check the few
compression algorithms easily in the frontend, i.e., offloading a simple
string regex match to the backend seems rather odd to me..

Am 21/09/2023 um 15:09 schrieb Philipp Hufnagl:
> extends the query_url_metadata callback with the functionallity to

s/functionallity/functionality/

> detect used compressions. If a compression is used it tells the ui which
> one
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  PVE/API2/Nodes.pm | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index 5a148d1d..1e8ed09e 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -34,6 +34,7 @@ use PVE::RRD;
>  use PVE::Report;
>  use PVE::SafeSyslog;
>  use PVE::Storage;
> +use PVE::Storage::Plugin;
>  use PVE::Tools;
>  use PVE::pvecfg;
>  
> @@ -1564,7 +1565,13 @@ __PACKAGE__->register_method({
>  		type => 'boolean',
>  		optional => 1,
>  		default => 1,
> -	    }
> +	    },
> +	    'detect-compression' => {
> +		description => "If true an auto detection of used compression will be attempted",

Grammatically and semantically slightly better would be something like:
"If true, the queried filename is checked for a compression extension."



> +		type => 'boolean',
> +		optional => 1,
> +		default => 0,
> +	    },
>  	},
>      },
>      returns => {
> @@ -1583,6 +1590,11 @@ __PACKAGE__->register_method({
>  		type => 'string',
>  		optional => 1,
>  	    },
> +	    compression => {
> +		type => 'string',
> +		enum => $PVE::Storage::Plugin::KNOWN_COMPRESSION_FORMATS,> +		optional => 1,
> +	    },
>  	},
>      },
>      code => sub {
> @@ -1605,6 +1617,7 @@ __PACKAGE__->register_method({
>  		SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE,
>  	    );
>  	}
> +	my $detect_compression = $param->{'detect-compression'} // 0;

It's often best to avoid such intermediate variables if there's just a
single use case and on doesn't needs to "jump through hoops" to get
to the value – e.g., a simple hash access like here if 100% fine, if
the value would to be pulled out of some deeply nested structure, or
assembled or the like, then it might have its merit to use an
intermediate variable.

>  
>  	my $req = HTTP::Request->new(HEAD => $url);
>  	my $res = $ua->request($req);
> @@ -1615,6 +1628,7 @@ __PACKAGE__->register_method({
>  	my $disposition = $res->header("Content-Disposition");
>  	my $type = $res->header("Content-Type");
>  
> +	my $compression;

Keep definition and use closer together (I'd moved this down directly above the if t that sets it)

>  	my $filename;
>  
>  	if ($disposition && ($disposition =~ m/filename="([^"]*)"/ || $disposition =~ m/filename=([^;]*)/)) {
> @@ -1628,10 +1642,16 @@ __PACKAGE__->register_method({
>  	    $type = $1;
>  	}
>  
> +	if ($detect_compression && $filename =~ m!^(.+)\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})$!) {

There are code paths where $filename is not yet defined here, resulting
in a rather ugly warning – so this needs upfront checking too – always
check where the value code path is coming in (yeah, Rust would do that for
you, but most API endpoints are small enough to be able to do so quickly also manually)

> +	    $filename = $1;
> +	    $compression = $2;
> +	}
> +
>  	my $ret = {};
>  	$ret->{filename} = $filename if $filename;
>  	$ret->{size} = $size + 0 if $size;
>  	$ret->{mimetype} = $type if $type;
> +	$ret->{compression} = $compression if $compression;
>  
>  	return $ret;
>      }});