[pve-devel] [PATCH stable-7+master manager] ui: vm selector: handle empty string gracefully

[pve-devel] [PATCH stable-7+master manager] ui: vm selector: handle empty string gracefully

[Fiona Ebner]

which is passed by the backup job window when using selection mode
'all', would be converted to [""] and wrongly add an entry with VMID
0 because the item "" could not be found in the store.

Reported in the community forum:
https://forum.proxmox.com/threads/130164/

Fixes: 7a5ca76a ("fix #4239: ui: show selected but non-existing vmids in backup edit")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 www/manager6/form/VMSelector.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/form/VMSelector.js b/www/manager6/form/VMSelector.js
index 4c0bba13..bf2c8df7 100644
--- a/www/manager6/form/VMSelector.js
+++ b/www/manager6/form/VMSelector.js
@@ -162,7 +162,7 @@ Ext.define('PVE.form.VMSelector', {
     setValue: function(value) {
 	let me = this;
 	if (!Ext.isArray(value)) {
-	    value = value.split(',');
+	    value = value === '' ? [] : value.split(',');
 	}
 
 	let store = me.getStore();
-- 
2.39.2

Re: [pve-devel] [PATCH stable-7+master manager] ui: vm selector: handle empty string gracefully

[Thomas Lamprecht]

Am 06/07/2023 um 15:51 schrieb Fiona Ebner:
> which is passed by the backup job window when using selection mode
> 'all', would be converted to [""] and wrongly add an entry with VMID
> 0 because the item "" could not be found in the store.
> 
> Reported in the community forum:
> https://forum.proxmox.com/threads/130164/
> 
> Fixes: 7a5ca76a ("fix #4239: ui: show selected but non-existing vmids in backup edit")
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  www/manager6/form/VMSelector.js | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/www/manager6/form/VMSelector.js b/www/manager6/form/VMSelector.js
> index 4c0bba13..bf2c8df7 100644
> --- a/www/manager6/form/VMSelector.js
> +++ b/www/manager6/form/VMSelector.js
> @@ -162,7 +162,7 @@ Ext.define('PVE.form.VMSelector', {
>      setValue: function(value) {
>  	let me = this;
>  	if (!Ext.isArray(value)) {
> -	    value = value.split(',');
> +	    value = value === '' ? [] : value.split(',');

more future proof would be doing it (also) on the call sites, as then future callers
are also guarded against it, e.g. something like:

diff --git a/www/manager6/form/VMSelector.js b/www/manager6/form/VMSelector.js
index bf2c8df7..076360ec 100644
--- a/www/manager6/form/VMSelector.js
+++ b/www/manager6/form/VMSelector.js
@@ -135,7 +135,7 @@ Ext.define('PVE.form.VMSelector', {
        let notFound = [];
        let selection = value.map(item => {
            let found = store.findRecord('vmid', item, 0, false, true, true);
-           if (!found) {
+           if (!found && item !== '') {
                notFound.push(item);
            }
            return found;

or if we want to go above and beyond we could even do a regex match for /^[1-9][0-9]{2,}$/
while more expensive even thousands of guests should not really add a noticeable delay,
but just mentioning it for completeness sake.

Re: [pve-devel] [PATCH stable-7+master manager] ui: vm selector: handle empty string gracefully

[Fiona Ebner]

Am 06.07.23 um 18:16 schrieb Thomas Lamprecht:
> Am 06/07/2023 um 15:51 schrieb Fiona Ebner:
>> which is passed by the backup job window when using selection mode
>> 'all', would be converted to [""] and wrongly add an entry with VMID
>> 0 because the item "" could not be found in the store.
>>
>> Reported in the community forum:
>> https://forum.proxmox.com/threads/130164/
>>
>> Fixes: 7a5ca76a ("fix #4239: ui: show selected but non-existing vmids in backup edit")
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>  www/manager6/form/VMSelector.js | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/www/manager6/form/VMSelector.js b/www/manager6/form/VMSelector.js
>> index 4c0bba13..bf2c8df7 100644
>> --- a/www/manager6/form/VMSelector.js
>> +++ b/www/manager6/form/VMSelector.js
>> @@ -162,7 +162,7 @@ Ext.define('PVE.form.VMSelector', {
>>      setValue: function(value) {
>>  	let me = this;
>>  	if (!Ext.isArray(value)) {
>> -	    value = value.split(',');
>> +	    value = value === '' ? [] : value.split(',');
> 
> more future proof would be doing it (also) on the call sites, as then future callers

In favor of the "also", because it just feels cleaner to parse the empty
list as an empty array and avoid the invalid '' item in the first place.
And if the future would bring a new usage of the parsed value in
setValue() here, then the above is future-proof ;)

> are also guarded against it, e.g. something like:
> 
> diff --git a/www/manager6/form/VMSelector.js b/www/manager6/form/VMSelector.js
> index bf2c8df7..076360ec 100644
> --- a/www/manager6/form/VMSelector.js
> +++ b/www/manager6/form/VMSelector.js
> @@ -135,7 +135,7 @@ Ext.define('PVE.form.VMSelector', {
>         let notFound = [];
>         let selection = value.map(item => {
>             let found = store.findRecord('vmid', item, 0, false, true, true);
> -           if (!found) {
> +           if (!found && item !== '') {
>                 notFound.push(item);
>             }
>             return found;
> 
> or if we want to go above and beyond we could even do a regex match for /^[1-9][0-9]{2,}$/
> while more expensive even thousands of guests should not really add a noticeable delay,
> but just mentioning it for completeness sake.

Matching wouldn't work if the item is already a number and not a string.
Maybe do the middle way and use Ext.isNumeric?

I'll send a v2 with both changes.