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

[Friedrich Weber <f.weber@proxmox.com>]

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)?