[pve-devel] [PATCH proxmox-widget-toolkit 1/2] FileBrowser: allow downloading root folder and simplify code

[pve-devel] [PATCH proxmox-widget-toolkit 1/2] FileBrowser: allow downloading root folder and simplify code

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Smoke tested by downloading an entire container archive as zip, extracting and
checking the files. If there was a reason for this to be disabled let me know.

Based on my previous series to move the FileBrowser to the widget toolkit:
https://lists.proxmox.com/pipermail/pbs-devel/2021-April/002590.html

Depends on the proxmox-backup patch to work correctly.

 src/window/FileBrowser.js | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/src/window/FileBrowser.js b/src/window/FileBrowser.js
index d8e75f4..7d82b01 100644
--- a/src/window/FileBrowser.js
+++ b/src/window/FileBrowser.js
@@ -98,23 +98,7 @@ Ext.define("Proxmox.window.FileBrowser", {
 	    if (!selection || selection.length < 1) return;
 
 	    let data = selection[0].data;
-
-	    let canDownload = false;
-	    if (view.downloadUrl) {
-		switch (data.type) {
-		    case 'h':
-		    case 'f':
-			canDownload = true;
-			break;
-		    case 'd':
-			if (data.depth > 1) {
-			    canDownload = true;
-			}
-			break;
-		    default: break;
-		}
-	    }
-
+	    let canDownload = view.downloadUrl && ['h', 'f', 'd'].indexOf(data.type) !== -1;
 	    me.lookup('downloadBtn').setDisabled(!canDownload);
 	},
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-widget-toolkit 1/2] FileBrowser: allow downloading root folder and simplify code

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Smoke tested by downloading an entire container archive as zip, extracting and
checking the files. If there was a reason for this to be disabled let me know.

Based on my previous series to move the FileBrowser to the widget toolkit:
https://lists.proxmox.com/pipermail/pbs-devel/2021-April/002590.html

Depends on the proxmox-backup patch to work correctly.

 src/window/FileBrowser.js | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/src/window/FileBrowser.js b/src/window/FileBrowser.js
index d8e75f4..7d82b01 100644
--- a/src/window/FileBrowser.js
+++ b/src/window/FileBrowser.js
@@ -98,23 +98,7 @@ Ext.define("Proxmox.window.FileBrowser", {
 	    if (!selection || selection.length < 1) return;
 
 	    let data = selection[0].data;
-
-	    let canDownload = false;
-	    if (view.downloadUrl) {
-		switch (data.type) {
-		    case 'h':
-		    case 'f':
-			canDownload = true;
-			break;
-		    case 'd':
-			if (data.depth > 1) {
-			    canDownload = true;
-			}
-			break;
-		    default: break;
-		}
-	    }
-
+	    let canDownload = view.downloadUrl && ['h', 'f', 'd'].indexOf(data.type) !== -1;
 	    me.lookup('downloadBtn').setDisabled(!canDownload);
 	},
 
-- 
2.20.1

[pve-devel] [PATCH proxmox-backup 2/2] api/datastore: allow pxar file download of entire archive

[Stefan Reiter]

Treat filepaths like "/root.pxar.didx" without a trailing slash as
wanting to download the entire archive content instead of erroring. The
zip-creation code already works fine for this scenario.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/api2/admin/datastore.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a3e115f6..97860910 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1385,7 +1385,7 @@ pub fn pxar_file_download(
 
         let mut split = components.splitn(2, |c| *c == b'/');
         let pxar_name = std::str::from_utf8(split.next().unwrap())?;
-        let file_path = split.next().ok_or_else(|| format_err!("filepath looks strange '{}'", filepath))?;
+        let file_path = split.next().unwrap_or(b"/");
         let (manifest, files) = read_backup_index(&datastore, &backup_dir)?;
         for file in files {
             if file.filename == pxar_name && file.crypt_mode == Some(CryptMode::Encrypt) {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/2] api/datastore: allow pxar file download of entire archive

[Stefan Reiter]

Treat filepaths like "/root.pxar.didx" without a trailing slash as
wanting to download the entire archive content instead of erroring. The
zip-creation code already works fine for this scenario.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/api2/admin/datastore.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a3e115f6..97860910 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1385,7 +1385,7 @@ pub fn pxar_file_download(
 
         let mut split = components.splitn(2, |c| *c == b'/');
         let pxar_name = std::str::from_utf8(split.next().unwrap())?;
-        let file_path = split.next().ok_or_else(|| format_err!("filepath looks strange '{}'", filepath))?;
+        let file_path = split.next().unwrap_or(b"/");
         let (manifest, files) = read_backup_index(&datastore, &backup_dir)?;
         for file in files {
             if file.filename == pxar_name && file.crypt_mode == Some(CryptMode::Encrypt) {
-- 
2.20.1

[pve-devel] applied: [pbs-devel] [PATCH proxmox-backup 2/2] api/datastore: allow pxar file download of entire archive

[Thomas Lamprecht]

On 12.04.21 17:32, Stefan Reiter wrote:
> Treat filepaths like "/root.pxar.didx" without a trailing slash as
> wanting to download the entire archive content instead of erroring. The
> zip-creation code already works fine for this scenario.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

But that API is definitively weird in general...

1. old style API definition, should use the #[api()] macro instead
2. perly "params: Value", yeah, no thanks.
3. hard coded return stream type, one should be able to download also a single
   file as zip, and we knew that we wanted .tar then too, so not providing an
   param for that is weird.
4. accessed via /json/ path but never returns json

[pbs-devel] applied: [PATCH proxmox-backup 2/2] api/datastore: allow pxar file download of entire archive

[Thomas Lamprecht]

On 12.04.21 17:32, Stefan Reiter wrote:
> Treat filepaths like "/root.pxar.didx" without a trailing slash as
> wanting to download the entire archive content instead of erroring. The
> zip-creation code already works fine for this scenario.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

But that API is definitively weird in general...

1. old style API definition, should use the #[api()] macro instead
2. perly "params: Value", yeah, no thanks.
3. hard coded return stream type, one should be able to download also a single
   file as zip, and we knew that we wanted .tar then too, so not providing an
   param for that is weird.
4. accessed via /json/ path but never returns json

Re: [pve-devel] [pbs-devel] applied: [PATCH proxmox-backup 2/2] api/datastore: allow pxar file download of entire archive

[Dominik Csapak]

On 4/13/21 08:39, Thomas Lamprecht wrote:
> On 12.04.21 17:32, Stefan Reiter wrote:
>> Treat filepaths like "/root.pxar.didx" without a trailing slash as
>> wanting to download the entire archive content instead of erroring. The
>> zip-creation code already works fine for this scenario.
>>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>   src/api2/admin/datastore.rs | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>
> 
> applied, thanks!
> 
> But that API is definitively weird in general...

just fyi

> 
> 1. old style API definition, should use the #[api()] macro instead

the api macro cannot handle AsyncHttp api calls (yet?), but this is 
required for the stream

> 2. perly "params: Value", yeah, no thanks.

a result from above, without api macro no de-structuring of parameters

> 3. hard coded return stream type, one should be able to download also a single
>     file as zip, and we knew that we wanted .tar then too, so not providing an
>     param for that is weird.

we always can add as much, but until now, generating a zip for a single
file was not really sensible

> 4. accessed via /json/ path but never returns json

all api calls need a formatter to call, should we add a
new one for download type?

> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 

Re: [pbs-devel] applied: [PATCH proxmox-backup 2/2] api/datastore: allow pxar file download of entire archive

[Dominik Csapak]

On 4/13/21 08:39, Thomas Lamprecht wrote:
> On 12.04.21 17:32, Stefan Reiter wrote:
>> Treat filepaths like "/root.pxar.didx" without a trailing slash as
>> wanting to download the entire archive content instead of erroring. The
>> zip-creation code already works fine for this scenario.
>>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>   src/api2/admin/datastore.rs | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>
> 
> applied, thanks!
> 
> But that API is definitively weird in general...

just fyi

> 
> 1. old style API definition, should use the #[api()] macro instead

the api macro cannot handle AsyncHttp api calls (yet?), but this is 
required for the stream

> 2. perly "params: Value", yeah, no thanks.

a result from above, without api macro no de-structuring of parameters

> 3. hard coded return stream type, one should be able to download also a single
>     file as zip, and we knew that we wanted .tar then too, so not providing an
>     param for that is weird.

we always can add as much, but until now, generating a zip for a single
file was not really sensible

> 4. accessed via /json/ path but never returns json

all api calls need a formatter to call, should we add a
new one for download type?

> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 

Re: [pve-devel] [pbs-devel] applied: [PATCH proxmox-backup 2/2] api/datastore: allow pxar file download of entire archive

[Thomas Lamprecht]

On 13.04.21 09:23, Dominik Csapak wrote:
> On 4/13/21 08:39, Thomas Lamprecht wrote:
>> But that API is definitively weird in general...
> 
> just fyi
> 
>>
>> 1. old style API definition, should use the #[api()] macro instead
> 
> the api macro cannot handle AsyncHttp api calls (yet?), but this is required for the stream

that shoudn't be a hard problem, it's a macro it can expand to whatever..

>> 2. perly "params: Value", yeah, no thanks.
> 
> a result from above, without api macro no de-structuring of parameters

see above

> 
>> 3. hard coded return stream type, one should be able to download also a single
>>     file as zip, and we knew that we wanted .tar then too, so not providing an
>>     param for that is weird.
> 
> we always can add as much, but until now, generating a zip for a single
> file was not really sensible

Compression isn't the only benefit a encapsulation like an archive format.

> 
>> 4. accessed via /json/ path but never returns json
> 
> all api calls need a formatter to call, should we add a
> new one for download type?

I know that all paths have a formatter, does not validates misusing JSON
for something completely different ;)

Re: [pbs-devel] applied: [PATCH proxmox-backup 2/2] api/datastore: allow pxar file download of entire archive

[Thomas Lamprecht]

On 13.04.21 09:23, Dominik Csapak wrote:
> On 4/13/21 08:39, Thomas Lamprecht wrote:
>> But that API is definitively weird in general...
> 
> just fyi
> 
>>
>> 1. old style API definition, should use the #[api()] macro instead
> 
> the api macro cannot handle AsyncHttp api calls (yet?), but this is required for the stream

that shoudn't be a hard problem, it's a macro it can expand to whatever..

>> 2. perly "params: Value", yeah, no thanks.
> 
> a result from above, without api macro no de-structuring of parameters

see above

> 
>> 3. hard coded return stream type, one should be able to download also a single
>>     file as zip, and we knew that we wanted .tar then too, so not providing an
>>     param for that is weird.
> 
> we always can add as much, but until now, generating a zip for a single
> file was not really sensible

Compression isn't the only benefit a encapsulation like an archive format.

> 
>> 4. accessed via /json/ path but never returns json
> 
> all api calls need a formatter to call, should we add a
> new one for download type?

I know that all paths have a formatter, does not validates misusing JSON
for something completely different ;)

[pve-devel] applied: [PATCH proxmox-widget-toolkit 1/2] FileBrowser: allow downloading root folder and simplify code

[Thomas Lamprecht]

On 12.04.21 17:32, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Smoke tested by downloading an entire container archive as zip, extracting and
> checking the files. If there was a reason for this to be disabled let me know.
> 
> Based on my previous series to move the FileBrowser to the widget toolkit:
> https://lists.proxmox.com/pipermail/pbs-devel/2021-April/002590.html
> 
> Depends on the proxmox-backup patch to work correctly.
> 
>  src/window/FileBrowser.js | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
>

applied, thanks!

Added a actual config sections with comments for the widget specific params.

[pbs-devel] applied: [pve-devel] [PATCH proxmox-widget-toolkit 1/2] FileBrowser: allow downloading root folder and simplify code

[Thomas Lamprecht]

On 12.04.21 17:32, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Smoke tested by downloading an entire container archive as zip, extracting and
> checking the files. If there was a reason for this to be disabled let me know.
> 
> Based on my previous series to move the FileBrowser to the widget toolkit:
> https://lists.proxmox.com/pipermail/pbs-devel/2021-April/002590.html
> 
> Depends on the proxmox-backup patch to work correctly.
> 
>  src/window/FileBrowser.js | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
>

applied, thanks!

Added a actual config sections with comments for the widget specific params.