0023627: DalUtil.copy also copies the non-parent-child-one-to-many associations, this is invalid

Description

Snippet from email discussion:

Lets remember what DalUtil.copy does when being called on a UOM row.
It does create a clone of that object so giving us a copy which will
get a new uuid when saved.

Optionally when the parameter cloneChildren is true it does also
recursively clone all children for example all UOMTrl rows for this
UOM are also cloned and the new UOM & UOMTrl cloned are associated
with each other (via the usual dal collections).

Now look at the code doing this:
https://code.openbravo.com/erp/devel/pi/file/6f099465923b/src/org/openbravo/dal/core/DalUtil.java#l321 [^]

If a one-to-many property is marked as a child and the target is not a
view we clone all entries in the list and assign the new clones to the
new association list. So far all ok.

However there is an else part:
If requested to clone children and we encounter a one-to-many property
not being a child (Example: link from UOM to C_Order entries using
that c_uom) then we clone the association list and copy over all
entries of that list.

Exactly that is the questionable part.
Why do is that done?
First that association looks inconsistent, the association list of the
newly cloned DAL-object is pointing to the c_order entries associated
with the old UOM instance. But the lists of those c_order entries does
not point to the new uom object (and it shouldn't).

Also why would we want to mark the newly cloned UOM object to be 'used
by' any c_order entry?

After a quick chat with Martin to us it looks like this is just
functionally wrong and should be fixed.
>>>>>>>>>>>>>>>>>>>>>>
The one-to-many properties you are describing should correspond to an empty list after the copy, it doesn't make sense to copy the old referring objects into it (even if we cloned all the referring objects, and the corresponding foreign-key property was "fixed" in the cloned instance), because you are just creating a new object instance, which shouldn't be referenced by anything in the beginning, basically because it's new (in your example, if you copy a UOM instance, we would be creating a list which contained all the orders pointing to the original UOM, which doesn't make sense at all).
>>>>>>>>>>>>>>>>>>>>>>
Also check:

FYI: i check for users in pi (ignoring src-test) using DalUtil.copy*(bob, true) and did not find many, when working on this perhaps you want to take a quick look at those to validate our assumption that there should be no code using/exploiting this bug:
./modules/org.openbravo.demo.loginpage/src/org/openbravo/demo/loginpage/security/LoginDemoHandler.java: User clonedUser = (User) DalUtil.copy(user, true);
./modules/org.openbravo.demo.loginpage/src/org/openbravo/demo/loginpage/security/LoginDemoHandler.java: WidgetInstance witemp = (WidgetInstance) DalUtil.copy(wi, true);
./modules/org.openbravo.advpaymentmngt/src/org/openbravo/advpaymentmngt/ad_actionbutton/MatchTransaction.java: FIN_BankStatementLine clonedBSLine = (FIN_BankStatementLine) DalUtil.copy(bsl, true
./modules/org.openbravo.advpaymentmngt/src/org/openbravo/advpaymentmngt/ad_actionbutton/MatchTransaction.java: FIN_BankStatementLine clonedBSLine = (FIN_BankStatementLine) DalUtil.copy(bsl, true

Steps To Reproduce

- Get an uom from the database, an uom which is being used by an orderline
- copy it using the OBDal.copy
- see that the one-to-many for c_order lines are also filled, this should not be the case.

Proposed Solution

Do not read/copy the one-to-many associations which are not children.

Add a testcase for this.

Tags

No tags attached.

Attached Files

Relationships [ Relation Graph ] [ Dependency Graph ]

blocks

defect

0023640

3.0MP23

closed

mtaal

API Change: DalUtil.copy also copies the non-parent-child-one-to-many associations, this is invalid

Notes
(0058167) shuehner (administrator) 2013-04-24 11:17	When this is fixed you should notice a decrease of the runtime of the following testcase: EntityXMLIssues.testMantis6213. Note runtime after can still be several seconds because of other inefficiencies in the testcase code, but on a slower system the DalUtil.copy itself took nearly 10seconds

(0058256) hgbot (developer) 2013-04-25 14:21	Repository: erp/devel/pi Changeset: 30185282b7a1058057dae4c3c9585d94da9195c2 Author: Martin Taal <martin.taal <at> openbravo.com> Date: Thu Apr 25 14:20:50 2013 +0200 URL: http://code.openbravo.com/erp/devel/pi/rev/30185282b7a1058057dae4c3c9585d94da9195c2 [^] Fixes issue 23627: DalUtil.copy also copies the non-parent-child-one-to-many associations, this is invalid Prevent copying of non-child one-to-many associations, include testcases --- M src-test/org/openbravo/test/dal/IssuesTest.java M src/org/openbravo/dal/core/DalUtil.java ---

(0058290) hudsonbot (developer) 2013-04-26 23:54	A changeset related to this issue has been promoted main and to the Central Repository, after passing a series of tests. Promotion changeset: https://code.openbravo.com/erp/devel/main/rev/1db7e66bd5c5 [^] Maturity status: Test

(0058308) AugustoMauch (manager) 2013-04-29 18:11	Code reviewed and verified in pi@5e5dd4a8b5c0

Issue History
Date Modified	Username	Field	Change
2013-04-24 10:59	mtaal	New Issue
2013-04-24 10:59	mtaal	Assigned To	=> mtaal
2013-04-24 10:59	mtaal	Modules	=> Core
2013-04-24 10:59	mtaal	Triggers an Emergency Pack	=> No
2013-04-24 11:17	shuehner	Note Added: 0058167
2013-04-24 11:18	shuehner	Issue Monitored: shuehner
2013-04-25 14:16	mtaal	Summary	DalUtil.copy also reads the non-parent-child-one-to-many associations, this is invalid => DalUtil.copy also copies the non-parent-child-one-to-many associations, this is invalid
2013-04-25 14:19	mtaal	Relationship added	blocks 0023640
2013-04-25 14:20	mtaal	Review Assigned To	=> AugustoMauch
2013-04-25 14:21	hgbot	Checkin
2013-04-25 14:21	hgbot	Note Added: 0058256
2013-04-25 14:21	hgbot	Status	new => resolved
2013-04-25 14:21	hgbot	Resolution	open => fixed
2013-04-25 14:21	hgbot	Fixed in SCM revision	=> http://code.openbravo.com/erp/devel/pi/rev/30185282b7a1058057dae4c3c9585d94da9195c2 [^]
2013-04-26 23:54	hudsonbot	Checkin
2013-04-26 23:54	hudsonbot	Note Added: 0058290
2013-04-29 18:11	AugustoMauch	Note Added: 0058308
2013-04-29 18:11	AugustoMauch	Status	resolved => closed
2013-04-29 18:11	AugustoMauch	Fixed in Version	=> 3.0MP23