From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 003219188D for ; Thu, 4 Apr 2024 12:54:44 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D34AB37182 for ; Thu, 4 Apr 2024 12:54:44 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 4 Apr 2024 12:54:44 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id DCADF4527E for ; Thu, 4 Apr 2024 12:54:43 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 04 Apr 2024 12:54:42 +0200 Message-Id: To: "Friedrich Weber" , "Proxmox VE development discussion" From: "Stefan Sterz" X-Mailer: aerc 0.17.0-69-g65571b67d7d3-dirty References: <20240403091010.11544-1-f.weber@proxmox.com> <20240403091010.11544-4-f.weber@proxmox.com> <147ee23c-97ae-4cc5-8c1f-ffc2ad4d6773@proxmox.com> In-Reply-To: <147ee23c-97ae-4cc5-8c1f-ffc2ad4d6773@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.075 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Apr 2024 10:54:45 -0000 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 t= o > >>> rule out this class of bug completelly: > >>> > >>> ```js > >>> constructor: function(conf) { > >>> let me =3D this; > >>> me.extraRequestParams =3D {}; > >>> 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 a= lso > >>> fixes the other instances? > >>> > >>> [1]: https://docs-devel.sencha.com/extjs/7.0.0/classic/Ext.window.Win= dow.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 i= s > >> called, `extraRequestParams` with `allow-move` is only defined in > >> `me.__proto__`, so `me.extraRequestParams =3D {}` 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 =3D = {}` > > 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 =3D this; > =3D> me.extraRequestParams =3D {}; > 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=3Dpve-manager.git;a=3Dblob;f=3Dwww/manager6= /grid/PoolMembers.js;h=3D75f20cab;hb=3D4b06efb5#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 =3D this; > > if (!me.extraRequestParams) { > > me.extraRequestParams =3D {}; > > } > > 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 =3D this; > me.extraRequestParams =3D 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.