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

Openbravo Issue Tracking System - Openbravo ERP

View Issue Details

Project

Category

View Status

Date Submitted

Last Update

0023627

Openbravo ERP

A. Platform

public

2013-04-24 10:59

2013-04-29 18:11

Reporter

mtaal

Assigned To

mtaal

Priority

normal

Severity

major

Reproducibility

have not tried

Status

closed

Resolution

fixed

Platform

OS Version

Product Version

Target Version

3.0MP23

Fixed in Version

3.0MP23

Merge Request Status

Review Assigned To

AugustoMauch

OBNetwork customer

Web browser

Modules

Core

Support ticket

Regression level

Regression date

Regression introduced in release

Regression introduced by commit

Triggers an Emergency Pack

Summary

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.

Additional Information