[pve-devel] [PATCH manager] ui: pool members: avoid setting request parameter for all edit windows

[pve-devel] [PATCH manager] ui: pool members: avoid setting request parameter for all edit windows

[Friedrich Weber]

Currently, after adding a storage to a pool, opening any edit window
will send a GET request with a superfluous `poolid` parameter and
cause a parameter verification error in the GUI. This breaks all edit
windows of the current session. A workaround is to reload the current
browser session.

This happens because the `PVE.pool.AddStorage` component inadvertently
adds `poolid` to an `extraRequestParams` object that is shared by all
instances of `Proxmox.window.Edit`, affecting all edit windows in the
current session. Fix this by instead creating a new object that is
local to the component.

Fixes: cd731902b7a724b1ab747276f9c6343734f1d8cb
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    To check if we have this problem at other places, I did a quick search
    for `extraRequestParams` in PVE+PBS: Seems like for all other usages,
    the object is created fresh already.

 www/manager6/grid/PoolMembers.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index 75f20cab..61e27dff 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -123,7 +123,7 @@ Ext.define('PVE.pool.AddStorage', {
 	me.isAdd = true;
 	me.url = "/pools/";
 	me.method = 'PUT';
-	me.extraRequestParams.poolid = me.pool;
+	me.extraRequestParams = { 'poolid': me.pool };
 
 	Ext.apply(me, {
 	    subject: gettext('Storage'),
-- 
2.39.2

Re: [pve-devel] [PATCH manager] ui: pool members: avoid setting request parameter for all edit windows

[Friedrich Weber]

On 13/03/2024 09:44, Friedrich Weber wrote:
> Currently, after adding a storage to a pool, opening any edit window
> will send a GET request with a superfluous `poolid` parameter and
> cause a parameter verification error in the GUI. This breaks all edit
> windows of the current session. A workaround is to reload the current
> browser session.
> 
> This happens because the `PVE.pool.AddStorage` component inadvertently
> adds `poolid` to an `extraRequestParams` object that is shared by all
> instances of `Proxmox.window.Edit`, affecting all edit windows in the
> current session. Fix this by instead creating a new object that is
> local to the component.
> 
> Fixes: cd731902b7a724b1ab747276f9c6343734f1d8cb
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
> 
> Notes:
>     To check if we have this problem at other places, I did a quick search
>     for `extraRequestParams` in PVE+PBS: Seems like for all other usages,
>     the object is created fresh already.

Small correction (as spotted by Aaron off-list): In `AddVM` in the same
file [1], we also have a line

22        me.extraRequestParams.poolid = me.pool;

This one does not break any edit windows, because the
`extraRequestParams` object is created a few lines above that in the
`Ext.define`, so it is not shared between all `Proxmox.window.Edit`
instances:

9      extraRequestParams: {
10         'allow-move': 1,
11     },

... but if I understand correctly, it *is* shared between all `AddVM`
instances. Again, I think this shouldn't break anything because we reset
`poolid` every time `initComponent` is called, but the pattern is
somewhat error-prone. It might be better to change line 22 to:

22        me.extraRequestParams = { 'allow-move': 1, 'poolid': me.pool };

... and remove lines 9-11, to always create a new object in
`initComponent`. What do you think?

[1]
https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/grid/PoolMembers.js;h=75f20cab;hb=385f48fb#l22

Re: [pve-devel] [PATCH manager] ui: pool members: avoid setting request parameter for all edit windows

[Stefan Sterz]

On Wed Mar 13, 2024 at 9:44 AM CET, Friedrich Weber wrote:
> Currently, after adding a storage to a pool, opening any edit window
> will send a GET request with a superfluous `poolid` parameter and
> cause a parameter verification error in the GUI. This breaks all edit
> windows of the current session. A workaround is to reload the current
> browser session.
>
> This happens because the `PVE.pool.AddStorage` component inadvertently
> adds `poolid` to an `extraRequestParams` object that is shared by all
> instances of `Proxmox.window.Edit`, affecting all edit windows in the
> current session. Fix this by instead creating a new object that is
> local to the component.
>
> Fixes: cd731902b7a724b1ab747276f9c6343734f1d8cb
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
>
> Notes:
>     To check if we have this problem at other places, I did a quick search
>     for `extraRequestParams` in PVE+PBS: Seems like for all other usages,
>     the object is created fresh already.
>
>  www/manager6/grid/PoolMembers.js | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
> index 75f20cab..61e27dff 100644
> --- a/www/manager6/grid/PoolMembers.js
> +++ b/www/manager6/grid/PoolMembers.js
> @@ -123,7 +123,7 @@ Ext.define('PVE.pool.AddStorage', {
>  	me.isAdd = true;
>  	me.url = "/pools/";
>  	me.method = 'PUT';
> -	me.extraRequestParams.poolid = me.pool;
> +	me.extraRequestParams = { 'poolid': me.pool };

note that you don't need to quote `poolid` here as it doesn't contain
hyphens as such. quickly grepping over our codebase it seems that not
quoting keys is preferred if it isn't needed

>
>  	Ext.apply(me, {
>  	    subject: gettext('Storage'),

Re: [pve-devel] [PATCH manager] ui: pool members: avoid setting request parameter for all edit windows

[Friedrich Weber]

On 14/03/2024 15:43, Stefan Sterz wrote:
> On Wed Mar 13, 2024 at 9:44 AM CET, Friedrich Weber wrote:
>> Currently, after adding a storage to a pool, opening any edit window
>> will send a GET request with a superfluous `poolid` parameter and
>> cause a parameter verification error in the GUI. This breaks all edit
>> windows of the current session. A workaround is to reload the current
>> browser session.
>>
>> This happens because the `PVE.pool.AddStorage` component inadvertently
>> adds `poolid` to an `extraRequestParams` object that is shared by all
>> instances of `Proxmox.window.Edit`, affecting all edit windows in the
>> current session. Fix this by instead creating a new object that is
>> local to the component.
>>
>> Fixes: cd731902b7a724b1ab747276f9c6343734f1d8cb
>> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
>> ---
>>
>> Notes:
>>     To check if we have this problem at other places, I did a quick search
>>     for `extraRequestParams` in PVE+PBS: Seems like for all other usages,
>>     the object is created fresh already.
>>
>>  www/manager6/grid/PoolMembers.js | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
>> index 75f20cab..61e27dff 100644
>> --- a/www/manager6/grid/PoolMembers.js
>> +++ b/www/manager6/grid/PoolMembers.js
>> @@ -123,7 +123,7 @@ Ext.define('PVE.pool.AddStorage', {
>>  	me.isAdd = true;
>>  	me.url = "/pools/";
>>  	me.method = 'PUT';
>> -	me.extraRequestParams.poolid = me.pool;
>> +	me.extraRequestParams = { 'poolid': me.pool };
> 
> note that you don't need to quote `poolid` here as it doesn't contain
> hyphens as such. quickly grepping over our codebase it seems that not
> quoting keys is preferred if it isn't needed

You're right, thanks for spotting this (my Python upbringing got the
better of me here).

I'll send a v2 that fixes this, and will probably include a patch that
rewrites the other occurrences of `extraRequestParams`.