[pve-devel] [PATCH manager/widget-toolkit 0/3] ui: avoid UI bugs due to shared extra request params

[pve-devel] [PATCH manager/widget-toolkit 0/3] ui: avoid UI bugs due to shared extra request params

[Friedrich Weber]

Currently, `Proxmox.window.Edit` initializes `extraRequestParams` to
an object that, if not overwritten, is shared between all instances of
subclasses. This bears the danger of modifying the shared object in a
subclass instead of overwriting it, which affects all edit windows of
the current session and can cause hard-to-catch UI bugs [1]

- Patch 1/3 fixes such an UI bug.
- Patch 2/3 (optional) fixes other occurrences of the pattern from 1/3,
  which are not buggy at the moment, but may become in the future.
- Patch 3/3 (optional) changes `Proxmox.window.Edit` to make this
  class of bugs less likely in the future.

Changes from v1:

- Patch 1/3: avoid unnecessary quotes
- Patch 2/3 + 3/3 are new

[1] https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html

manager:

Friedrich Weber (2):
  ui: pool members: avoid setting request parameter for all edit windows
  ui: pool members: avoid sharing object for extra request parameters

 www/manager6/grid/PoolMembers.js | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)


widget-toolkit:

Friedrich Weber (1):
  window: edit: avoid shared object for extra request params

 src/window/Edit.js | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)


Summary over all repositories:
  2 files changed, 10 insertions(+), 0 deletions(-)

-- 
Generated by git-murpp 0.5.0

[pve-devel] [PATCH manager 1/3] 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:
    changes since v1:
    - remove unnecessary quotes (thx Stefan)

 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..af6af1bd 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

[pve-devel] [PATCH manager 2/3] ui: pool members: avoid sharing object for extra request parameters

[Friedrich Weber]

Currently, all instances of `PVE.pool.AddVM` in a session share the
same `extraRequestParams` object. Right now, this does not cause any
problems because only one window can be active at a time, and all
relevant keys are always overwritten.

Still, in order to avoid hard-to-catch bugs due to the shared object
in the future, create a new `extraRequestParams` object for each
instance of `PVE.pool.AddVM`.

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

Notes:
    new in v2

 www/manager6/grid/PoolMembers.js | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index af6af1bd..4ffcde7f 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -6,10 +6,6 @@ Ext.define('PVE.pool.AddVM', {
     isAdd: true,
     isCreate: true,
 
-    extraRequestParams: {
-	'allow-move': 1,
-    },
-
     initComponent: function() {
 	var me = this;
 
@@ -19,7 +15,10 @@ Ext.define('PVE.pool.AddVM', {
 
 	me.url = '/pools/';
 	me.method = 'PUT';
-	me.extraRequestParams.poolid = me.pool;
+	me.extraRequestParams = {
+	    'allow-move': 1,
+	    poolid: me.pool,
+	};
 
 	var vmsField = Ext.create('Ext.form.field.Text', {
 	    name: 'vms',
-- 
2.39.2

[pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params

[Friedrich Weber]

Currently, `Proxmox.window.Edit` initializes `extraRequestParams` to
an object that, if not overwritten, is shared between all instances of
subclasses. This bears the danger of modifying the shared object in a
subclass instead of overwriting it, which affects all edit windows of
the current session and can cause hard-to-catch UI bugs [1].

To avoid such bugs in the future, initialize `extraRequestParams` to
`undefined` instead, which forces subclasses to initialize their own
objects.

Note that bugs of the same kind can still happen if a subclass
initializes `extraRequestParams` to an empty shared object and
inadvertently modifies it, but at least they will be limited to that
particular subclass.

[1] https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html

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

Notes:
    With patch 2/3 applied, I think all occurrences of
    `extraRequestParams` in PVE/PBS create their own object (PMG does not
    seem to use `extraRequestParams`), so this patch should not break
    anything, and if it does, it should be quite noticeable.
    
    new in v2

 src/window/Edit.js | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/window/Edit.js b/src/window/Edit.js
index d4a2b551..27cd8d01 100644
--- a/src/window/Edit.js
+++ b/src/window/Edit.js
@@ -9,7 +9,7 @@ Ext.define('Proxmox.window.Edit', {
 
     // to submit extra params on load and submit, useful, e.g., if not all ID
     // parameters are included in the URL
-    extraRequestParams: {},
+    extraRequestParams: undefined,
 
     resizable: false,
 
@@ -80,7 +80,9 @@ Ext.define('Proxmox.window.Edit', {
 	let me = this;
 
 	let values = {};
-	Ext.apply(values, me.extraRequestParams);
+	if (me.extraRequestParams) {
+	    Ext.apply(values, me.extraRequestParams);
+	}
 
 	let form = me.formPanel.getForm();
 
@@ -209,7 +211,7 @@ Ext.define('Proxmox.window.Edit', {
 	    waitMsgTarget: me,
 	}, options);
 
-	if (Object.keys(me.extraRequestParams).length > 0) {
+	if (me.extraRequestParams && Object.keys(me.extraRequestParams).length > 0) {
 	    let params = newopts.params || {};
 	    Ext.applyIf(params, me.extraRequestParams);
 	    newopts.params = params;
-- 
2.39.2

Re: [pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params

[Stefan Sterz]

On Wed Apr 3, 2024 at 11:10 AM CEST, Friedrich Weber wrote:
> Currently, `Proxmox.window.Edit` initializes `extraRequestParams` to
> an object that, if not overwritten, is shared between all instances of
> subclasses. This bears the danger of modifying the shared object in a
> subclass instead of overwriting it, which affects all edit windows of
> the current session and can cause hard-to-catch UI bugs [1].
>
> To avoid such bugs in the future, initialize `extraRequestParams` to
> `undefined` instead, which forces subclasses to initialize their own
> objects.
>
> Note that bugs of the same kind can still happen if a subclass
> initializes `extraRequestParams` to an empty shared object and
> inadvertently modifies it, but at least they will be limited to that
> particular subclass.
>
> [1] https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html
>
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
>
> Notes:
>     With patch 2/3 applied, I think all occurrences of
>     `extraRequestParams` in PVE/PBS create their own object (PMG does not
>     seem to use `extraRequestParams`), so this patch should not break
>     anything, and if it does, it should be quite noticeable.
>
>     new in v2
>
>  src/window/Edit.js | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/window/Edit.js b/src/window/Edit.js
> index d4a2b551..27cd8d01 100644
> --- a/src/window/Edit.js
> +++ b/src/window/Edit.js
> @@ -9,7 +9,7 @@ Ext.define('Proxmox.window.Edit', {
>
>      // to submit extra params on load and submit, useful, e.g., if not all ID
>      // parameters are included in the URL
> -    extraRequestParams: {},
> +    extraRequestParams: undefined,
>
>      resizable: false,
>
> @@ -80,7 +80,9 @@ Ext.define('Proxmox.window.Edit', {
>  	let me = this;
>
>  	let values = {};
> -	Ext.apply(values, me.extraRequestParams);
> +	if (me.extraRequestParams) {
> +	    Ext.apply(values, me.extraRequestParams);
> +	}
>
>  	let form = me.formPanel.getForm();
>
> @@ -209,7 +211,7 @@ Ext.define('Proxmox.window.Edit', {
>  	    waitMsgTarget: me,
>  	}, options);
>
> -	if (Object.keys(me.extraRequestParams).length > 0) {
> +	if (me.extraRequestParams && Object.keys(me.extraRequestParams).length > 0) {
>  	    let params = newopts.params || {};
>  	    Ext.applyIf(params, me.extraRequestParams);
>  	    newopts.params = params;

i did a quick an dirty test and using a constructor like this seems to
rule out this class of bug completelly:

```js
    constructor: function(conf) {
        let me = this;
        me.extraRequestParams = {};
        me.initConfig(conf);
        me.callParent();
    },
```

basically it configures the edit window as usual, but overwrites the
`extraRequestParams` object for each instance with a new empty object.
so there are no more shared objects :) could you check whether that also
fixes the other instances?

[1]: https://docs-devel.sencha.com/extjs/7.0.0/classic/Ext.window.Window.html#method-constructor

Re: [pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params

[Friedrich Weber]

On 04/04/2024 10:22, Stefan Sterz wrote:
> On Wed Apr 3, 2024 at 11:10 AM CEST, Friedrich Weber wrote:
>> Currently, `Proxmox.window.Edit` initializes `extraRequestParams` to
>> an object that, if not overwritten, is shared between all instances of
>> subclasses. This bears the danger of modifying the shared object in a
>> subclass instead of overwriting it, which affects all edit windows of
>> the current session and can cause hard-to-catch UI bugs [1].
>>
>> To avoid such bugs in the future, initialize `extraRequestParams` to
>> `undefined` instead, which forces subclasses to initialize their own
>> objects.
>>
>> Note that bugs of the same kind can still happen if a subclass
>> initializes `extraRequestParams` to an empty shared object and
>> inadvertently modifies it, but at least they will be limited to that
>> particular subclass.
>>
>> [1] https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html
>>
>> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
>> ---
>>
>> Notes:
>>     With patch 2/3 applied, I think all occurrences of
>>     `extraRequestParams` in PVE/PBS create their own object (PMG does not
>>     seem to use `extraRequestParams`), so this patch should not break
>>     anything, and if it does, it should be quite noticeable.
>>
>>     new in v2
>>
>>  src/window/Edit.js | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/window/Edit.js b/src/window/Edit.js
>> index d4a2b551..27cd8d01 100644
>> --- a/src/window/Edit.js
>> +++ b/src/window/Edit.js
>> @@ -9,7 +9,7 @@ Ext.define('Proxmox.window.Edit', {
>>
>>      // to submit extra params on load and submit, useful, e.g., if not all ID
>>      // parameters are included in the URL
>> -    extraRequestParams: {},
>> +    extraRequestParams: undefined,
>>
>>      resizable: false,
>>
>> @@ -80,7 +80,9 @@ Ext.define('Proxmox.window.Edit', {
>>  	let me = this;
>>
>>  	let values = {};
>> -	Ext.apply(values, me.extraRequestParams);
>> +	if (me.extraRequestParams) {
>> +	    Ext.apply(values, me.extraRequestParams);
>> +	}
>>
>>  	let form = me.formPanel.getForm();
>>
>> @@ -209,7 +211,7 @@ Ext.define('Proxmox.window.Edit', {
>>  	    waitMsgTarget: me,
>>  	}, options);
>>
>> -	if (Object.keys(me.extraRequestParams).length > 0) {
>> +	if (me.extraRequestParams && Object.keys(me.extraRequestParams).length > 0) {
>>  	    let params = newopts.params || {};
>>  	    Ext.applyIf(params, me.extraRequestParams);
>>  	    newopts.params = params;
> 
> i did a quick an dirty test and using a constructor like this seems to
> rule out this class of bug completelly:
> 
> ```js
>     constructor: function(conf) {
>         let me = this;
>         me.extraRequestParams = {};
>         me.initConfig(conf);
>         me.callParent();
>     },
> ```
> 
> basically it configures the edit window as usual, but overwrites the
> `extraRequestParams` object for each instance with a new empty object.
> so there are no more shared objects :) could you check whether that also
> fixes the other instances?
> 
> [1]: https://docs-devel.sencha.com/extjs/7.0.0/classic/Ext.window.Window.html#method-constructor

Nifty, didn't think about a constructor solution. Such a general
solution would be way more elegant, thanks for suggesting it!

However, this particular constructor seems to break the pattern of
defining `extraRequestParams` in the subclass properties, as done by
`PVE.Pool.AddVM` [1]. With the constructor above, the API request done
by `AddVM` seems to be missing the `allow-move` parameter.

Looks like once `PVE.Pool.AddVM` is instantiated and the constructor is
called, `extraRequestParams` with `allow-move` is only defined in
`me.__proto__`, so `me.extraRequestParams = {}` essentially shadows it
with an empty object, losing the `allow-move`.

Do you have an idea how to fix this? Maybe making a copy of
`extraRequestParams` would work (I suppose the overhead of creating a
new object for all edit window (subclass) instances is negligible).

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

Re: [pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params

[Stefan Sterz]

-- >8 snip 8< --
> >
> > i did a quick an dirty test and using a constructor like this seems to
> > rule out this class of bug completelly:
> >
> > ```js
> >     constructor: function(conf) {
> >         let me = this;
> >         me.extraRequestParams = {};
> >         me.initConfig(conf);
> >         me.callParent();
> >     },
> > ```
> >
> > basically it configures the edit window as usual, but overwrites the
> > `extraRequestParams` object for each instance with a new empty object.
> > so there are no more shared objects :) could you check whether that also
> > fixes the other instances?
> >
> > [1]: https://docs-devel.sencha.com/extjs/7.0.0/classic/Ext.window.Window.html#method-constructor
>
> Nifty, didn't think about a constructor solution. Such a general
> solution would be way more elegant, thanks for suggesting it!
>
> However, this particular constructor seems to break the pattern of
> defining `extraRequestParams` in the subclass properties, as done by
> `PVE.Pool.AddVM` [1]. With the constructor above, the API request done
> by `AddVM` seems to be missing the `allow-move` parameter.
>
> Looks like once `PVE.Pool.AddVM` is instantiated and the constructor is
> called, `extraRequestParams` with `allow-move` is only defined in
> `me.__proto__`, so `me.extraRequestParams = {}` essentially shadows it
> with an empty object, losing the `allow-move`.
>

not sure what you mean by that, if an `PVE.Pool.AddVM` is instantiated,
the `extraRequestParams` is already set, so it isn't just in `__proto__`
for me. but yeah, the problem is correct as `me.extraRequestParams = {}`
overwrites the field.

> Do you have an idea how to fix this? Maybe making a copy of
> `extraRequestParams` would work (I suppose the overhead of creating a
> new object for all edit window (subclass) instances is negligible).
>
> [1]
> https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/grid/PoolMembers.js;h=75f20cab;hb=4b06efb5#l9

this worked for me, can you confirm that this also does what it should
for you?

```js
    extraRequestParams: undefined,

    constructor: function(conf) {
        let me = this;
        if (!me.extraRequestParams) {
                me.extraRequestParams = {};
        }
        me.initConfig(conf);
        me.callParent();
    },
```

Re: [pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params

[Friedrich Weber]

On 04/04/2024 11:23, Stefan Sterz wrote:
> -- >8 snip 8< --
>>>
>>> i did a quick an dirty test and using a constructor like this seems to
>>> rule out this class of bug completelly:
>>>
>>> ```js
>>>     constructor: function(conf) {
>>>         let me = this;
>>>         me.extraRequestParams = {};
>>>         me.initConfig(conf);
>>>         me.callParent();
>>>     },
>>> ```
>>>
>>> basically it configures the edit window as usual, but overwrites the
>>> `extraRequestParams` object for each instance with a new empty object.
>>> so there are no more shared objects :) could you check whether that also
>>> fixes the other instances?
>>>
>>> [1]: https://docs-devel.sencha.com/extjs/7.0.0/classic/Ext.window.Window.html#method-constructor
>>
>> Nifty, didn't think about a constructor solution. Such a general
>> solution would be way more elegant, thanks for suggesting it!
>>
>> However, this particular constructor seems to break the pattern of
>> defining `extraRequestParams` in the subclass properties, as done by
>> `PVE.Pool.AddVM` [1]. With the constructor above, the API request done
>> by `AddVM` seems to be missing the `allow-move` parameter.
>>
>> Looks like once `PVE.Pool.AddVM` is instantiated and the constructor is
>> called, `extraRequestParams` with `allow-move` is only defined in
>> `me.__proto__`, so `me.extraRequestParams = {}` essentially shadows it
>> with an empty object, losing the `allow-move`.
>>
> 
> not sure what you mean by that, if an `PVE.Pool.AddVM` is instantiated,
> the `extraRequestParams` is already set, so it isn't just in `__proto__`
> for me. but yeah, the problem is correct as `me.extraRequestParams = {}`
> overwrites the field.

I agree it doesn't matter here, but just for completeness, I meant that
if I set a breakpoint before line 2, so before the overwrite:

```js
    constructor: function(conf) {
        let me = this;
=>        me.extraRequestParams = {};
        me.initConfig(conf);
        me.callParent();
    },
```

... `extraRequestParams` is not a property of `me`, but inherited from
its prototype:

```
>> me.extraRequestParams
Object { "allow-move": 1 }
>> "extraRequestParams" in me
true
>> Object.hasOwn(me, "extraRequestParams")
false
```

Doesn't make a difference for the overwrite, though.

>> Do you have an idea how to fix this? Maybe making a copy of
>> `extraRequestParams` would work (I suppose the overhead of creating a
>> new object for all edit window (subclass) instances is negligible).
>>
>> [1]
>> https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/grid/PoolMembers.js;h=75f20cab;hb=4b06efb5#l9
> 
> this worked for me, can you confirm that this also does what it should
> for you?
> 
> ```js
>     extraRequestParams: undefined,
> 
>     constructor: function(conf) {
>         let me = this;
>         if (!me.extraRequestParams) {
>                 me.extraRequestParams = {};
>         }
>         me.initConfig(conf);
>         me.callParent();
>     },
> ```

It works in the sense that it fixes the bug mentioned in my patch 1/3,
and fixes the lost `allow-move` issue from the previous constructor. But
with this constructor, all instances of `AddVM` share the same
`extraRequestParams` (the body of the `if` never gets executed for
`AddVM` instances), which is the condition that my patch 2/3 tries to
avoid (even though it is currently not buggy).

Maybe we could do:

```js
    extraRequestParams: {},

    constructor: function(conf) {
        let me = this;
	me.extraRequestParams = Ext.clone(me.extraRequestParams);
        me.initConfig(conf);
        me.callParent();
    },
```

... which, if I'm not missing anything, *should* cover everything (with
the cost of allocating unnecessary empty objects)?

Re: [pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params

[Stefan Sterz]

On Thu Apr 4, 2024 at 12:10 PM CEST, Friedrich Weber wrote:
> On 04/04/2024 11:23, Stefan Sterz wrote:
> > -- >8 snip 8< --
> >>>
> >>> i did a quick an dirty test and using a constructor like this seems to
> >>> rule out this class of bug completelly:
> >>>
> >>> ```js
> >>>     constructor: function(conf) {
> >>>         let me = this;
> >>>         me.extraRequestParams = {};
> >>>         me.initConfig(conf);
> >>>         me.callParent();
> >>>     },
> >>> ```
> >>>
> >>> basically it configures the edit window as usual, but overwrites the
> >>> `extraRequestParams` object for each instance with a new empty object.
> >>> so there are no more shared objects :) could you check whether that also
> >>> fixes the other instances?
> >>>
> >>> [1]: https://docs-devel.sencha.com/extjs/7.0.0/classic/Ext.window.Window.html#method-constructor
> >>
> >> Nifty, didn't think about a constructor solution. Such a general
> >> solution would be way more elegant, thanks for suggesting it!
> >>
> >> However, this particular constructor seems to break the pattern of
> >> defining `extraRequestParams` in the subclass properties, as done by
> >> `PVE.Pool.AddVM` [1]. With the constructor above, the API request done
> >> by `AddVM` seems to be missing the `allow-move` parameter.
> >>
> >> Looks like once `PVE.Pool.AddVM` is instantiated and the constructor is
> >> called, `extraRequestParams` with `allow-move` is only defined in
> >> `me.__proto__`, so `me.extraRequestParams = {}` essentially shadows it
> >> with an empty object, losing the `allow-move`.
> >>
> >
> > not sure what you mean by that, if an `PVE.Pool.AddVM` is instantiated,
> > the `extraRequestParams` is already set, so it isn't just in `__proto__`
> > for me. but yeah, the problem is correct as `me.extraRequestParams = {}`
> > overwrites the field.
>
> I agree it doesn't matter here, but just for completeness, I meant that
> if I set a breakpoint before line 2, so before the overwrite:
>
> ```js
>     constructor: function(conf) {
>         let me = this;
> =>        me.extraRequestParams = {};
>         me.initConfig(conf);
>         me.callParent();
>     },
> ```
>
> ... `extraRequestParams` is not a property of `me`, but inherited from
> its prototype:
>
> ```
> >> me.extraRequestParams
> Object { "allow-move": 1 }
> >> "extraRequestParams" in me
> true
> >> Object.hasOwn(me, "extraRequestParams")
> false
> ```
>
> Doesn't make a difference for the overwrite, though.
>

ah yeah, that makes sense, but yeah, it doesn't really matter here i
suppose.

> >> Do you have an idea how to fix this? Maybe making a copy of
> >> `extraRequestParams` would work (I suppose the overhead of creating a
> >> new object for all edit window (subclass) instances is negligible).
> >>
> >> [1]
> >> https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/grid/PoolMembers.js;h=75f20cab;hb=4b06efb5#l9
> >
> > this worked for me, can you confirm that this also does what it should
> > for you?
> >
> > ```js
> >     extraRequestParams: undefined,
> >
> >     constructor: function(conf) {
> >         let me = this;
> >         if (!me.extraRequestParams) {
> >                 me.extraRequestParams = {};
> >         }
> >         me.initConfig(conf);
> >         me.callParent();
> >     },
> > ```
>
> It works in the sense that it fixes the bug mentioned in my patch 1/3,
> and fixes the lost `allow-move` issue from the previous constructor. But
> with this constructor, all instances of `AddVM` share the same
> `extraRequestParams` (the body of the `if` never gets executed for
> `AddVM` instances), which is the condition that my patch 2/3 tries to
> avoid (even though it is currently not buggy).
>
> Maybe we could do:
>
> ```js
>     extraRequestParams: {},
>
>     constructor: function(conf) {
>         let me = this;
> 	me.extraRequestParams = Ext.clone(me.extraRequestParams);
>         me.initConfig(conf);
>         me.callParent();
>     },
> ```
>
> ... which, if I'm not missing anything, *should* cover everything (with
> the cost of allocating unnecessary empty objects)?

yeah looks good to me. cloning shouldn't cost too much here. if we are
really worried we could check whether the object is empty, clone
it in that case and assign an empty object otherwise.

Re: [pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params

[Thomas Lamprecht]

Am 04/04/2024 um 12:10 schrieb Friedrich Weber:
> Maybe we could do:
> 
> ```js
>     extraRequestParams: {},
> 
>     constructor: function(conf) {
>         let me = this;
> 	me.extraRequestParams = Ext.clone(me.extraRequestParams);
>         me.initConfig(conf);
>         me.callParent();
>     },
> ```
> 
> ... which, if I'm not missing anything, *should* cover everything (with
> the cost of allocating unnecessary empty objects)?
> 


yeah, I just wanted to suggest something like that, albeit I first had the
(a few times used):

me.extraRequestParams = Ext.apply({}, me.extraRequestParams ?? {});

pattern in mind. Mostly an slight performance improvement as we can assume
that this is either undefined or an object, while Ext.clone checks for all
different types (in a legacy browser compat way).

Albeit nowadays one might be even better off to use something like the
spread operator:

me.extraRequestParams = {  ...(me.extraRequestParams ?? {}) };

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#spread_in_object_literals

Or maybe even nicer, the Object.assign operator, that does not throw if
the sources are null or undefined:

me.extraRequestParams = Object.assign({}, me.extraRequestParams);

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign

But all those are implementation details (of which I probably would prefer
the Object.assign one the most, albeit no hard feelings), in general your
proposed solution seems to be the best one, w.r.t sane new usage and
backward compat.

btw. hasn't the `submitOptions` config object the same issue?

Re: [pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params

[Stefan Sterz]

On Thu Apr 4, 2024 at 12:54 PM CEST, Stefan Sterz wrote:
> On Thu Apr 4, 2024 at 12:10 PM CEST, Friedrich Weber wrote:
> > On 04/04/2024 11:23, Stefan Sterz wrote:
> > > -- >8 snip 8< --
> > >>>
> > >>> i did a quick an dirty test and using a constructor like this seems to
> > >>> rule out this class of bug completelly:
> > >>>
> > >>> ```js
> > >>>     constructor: function(conf) {
> > >>>         let me = this;
> > >>>         me.extraRequestParams = {};
> > >>>         me.initConfig(conf);
> > >>>         me.callParent();
> > >>>     },
> > >>> ```
> > >>>
> > >>> basically it configures the edit window as usual, but overwrites the
> > >>> `extraRequestParams` object for each instance with a new empty object.
> > >>> so there are no more shared objects :) could you check whether that also
> > >>> fixes the other instances?
> > >>>
> > >>> [1]: https://docs-devel.sencha.com/extjs/7.0.0/classic/Ext.window.Window.html#method-constructor
> > >>
> > >> Nifty, didn't think about a constructor solution. Such a general
> > >> solution would be way more elegant, thanks for suggesting it!
> > >>
> > >> However, this particular constructor seems to break the pattern of
> > >> defining `extraRequestParams` in the subclass properties, as done by
> > >> `PVE.Pool.AddVM` [1]. With the constructor above, the API request done
> > >> by `AddVM` seems to be missing the `allow-move` parameter.
> > >>
> > >> Looks like once `PVE.Pool.AddVM` is instantiated and the constructor is
> > >> called, `extraRequestParams` with `allow-move` is only defined in
> > >> `me.__proto__`, so `me.extraRequestParams = {}` essentially shadows it
> > >> with an empty object, losing the `allow-move`.
> > >>
> > >
> > > not sure what you mean by that, if an `PVE.Pool.AddVM` is instantiated,
> > > the `extraRequestParams` is already set, so it isn't just in `__proto__`
> > > for me. but yeah, the problem is correct as `me.extraRequestParams = {}`
> > > overwrites the field.
> >
> > I agree it doesn't matter here, but just for completeness, I meant that
> > if I set a breakpoint before line 2, so before the overwrite:
> >
> > ```js
> >     constructor: function(conf) {
> >         let me = this;
> > =>        me.extraRequestParams = {};
> >         me.initConfig(conf);
> >         me.callParent();
> >     },
> > ```
> >
> > ... `extraRequestParams` is not a property of `me`, but inherited from
> > its prototype:
> >
> > ```
> > >> me.extraRequestParams
> > Object { "allow-move": 1 }
> > >> "extraRequestParams" in me
> > true
> > >> Object.hasOwn(me, "extraRequestParams")
> > false
> > ```
> >
> > Doesn't make a difference for the overwrite, though.
> >
>
> ah yeah, that makes sense, but yeah, it doesn't really matter here i
> suppose.
>
> > >> Do you have an idea how to fix this? Maybe making a copy of
> > >> `extraRequestParams` would work (I suppose the overhead of creating a
> > >> new object for all edit window (subclass) instances is negligible).
> > >>
> > >> [1]
> > >> https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/grid/PoolMembers.js;h=75f20cab;hb=4b06efb5#l9
> > >
> > > this worked for me, can you confirm that this also does what it should
> > > for you?
> > >
> > > ```js
> > >     extraRequestParams: undefined,
> > >
> > >     constructor: function(conf) {
> > >         let me = this;
> > >         if (!me.extraRequestParams) {
> > >                 me.extraRequestParams = {};
> > >         }
> > >         me.initConfig(conf);
> > >         me.callParent();
> > >     },
> > > ```
> >
> > It works in the sense that it fixes the bug mentioned in my patch 1/3,
> > and fixes the lost `allow-move` issue from the previous constructor. But
> > with this constructor, all instances of `AddVM` share the same
> > `extraRequestParams` (the body of the `if` never gets executed for
> > `AddVM` instances), which is the condition that my patch 2/3 tries to
> > avoid (even though it is currently not buggy).
> >
> > Maybe we could do:
> >
> > ```js
> >     extraRequestParams: {},
> >
> >     constructor: function(conf) {
> >         let me = this;
> > 	me.extraRequestParams = Ext.clone(me.extraRequestParams);
> >         me.initConfig(conf);
> >         me.callParent();
> >     },
> > ```
> >
> > ... which, if I'm not missing anything, *should* cover everything (with
> > the cost of allocating unnecessary empty objects)?
>
> yeah looks good to me. cloning shouldn't cost too much here. if we are
> really worried we could check whether the object is empty, clone
> it in that case and assign an empty object otherwise.
>

i just looked through the code for `Ext.clone` [1], if i'm not mistaken
in modern browsers (which we restrict ourselves to anyway, mostly), this
basically returns an empty object anyway, we would skip a few
assignments and checks, but i doubt performance would improve to the
point where adding such a check makes sense.

[1]: https://docs.sencha.com/extjs/7.0.0/modern/src/Ext.js.html#Ext-method-clone

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

Re: [pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params

[Friedrich Weber]

On 04/04/2024 12:59, Thomas Lamprecht wrote:
> Am 04/04/2024 um 12:10 schrieb Friedrich Weber:
>> Maybe we could do:
>>
>> ```js
>>     extraRequestParams: {},
>>
>>     constructor: function(conf) {
>>         let me = this;
>> 	me.extraRequestParams = Ext.clone(me.extraRequestParams);
>>         me.initConfig(conf);
>>         me.callParent();
>>     },
>> ```
>>
>> ... which, if I'm not missing anything, *should* cover everything (with
>> the cost of allocating unnecessary empty objects)?
>>
> 
> 
> yeah, I just wanted to suggest something like that, albeit I first had the
> (a few times used):
> 
> me.extraRequestParams = Ext.apply({}, me.extraRequestParams ?? {});
> 
> pattern in mind. Mostly an slight performance improvement as we can assume
> that this is either undefined or an object, while Ext.clone checks for all
> different types (in a legacy browser compat way).
> 
> Albeit nowadays one might be even better off to use something like the
> spread operator:
> 
> me.extraRequestParams = {  ...(me.extraRequestParams ?? {}) };
> 
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#spread_in_object_literals
> 
> Or maybe even nicer, the Object.assign operator, that does not throw if
> the sources are null or undefined:
> 
> me.extraRequestParams = Object.assign({}, me.extraRequestParams);
> 
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign
> 
> But all those are implementation details (of which I probably would prefer
> the Object.assign one the most, albeit no hard feelings), in general your
> proposed solution seems to be the best one, w.r.t sane new usage and
> backward compat.

I agree `Object.assign` looks like the nicest solution, and seeing that
we already use it in our codebase, I'll go for this one.

> btw. hasn't the `submitOptions` config object the same issue?

You're right, `submitOptions` also has the potential for the same class
of bugs. The current usages of `submitOptions` look unproblematic. But
since we go for a more general fix for `extraRequestParams` anyway,
let's handle `submitOptions` similarly.

I'll send a v3 (just noticed I forgot the `v2` in the subject for this
one). Thank you and sterzy for your suggestions!