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

["Stefan Sterz" <s.sterz@proxmox.com>]

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.