| RE: [eclipselink-users] Application Developer Responsibilitieswhenoverridingjava.lang.Object.clone() in EclipseLink JPA mapped classes? |
Hi Tom, Thanks for your response. I was optimistic that your suggestion would in fact resolve the issue, but alas, that was not the case, the error persists even after using clonedEmployee.setDepartment(null); within the clone() implementation in Employee.java. I am now wondering if there is in fact a defect in EclipseLink 1.0.1 in this area. Since we are using field based access due to the placement of annotations on the fields themselves, I would think clonedEmployee.department = null or clonedEmployee.setDepartment(null) would work, but neither do. I have attached a .zip file that contains a JUnit test that demonstrates this problem. The source files included mimic the class hierarchy where we saw the original problem in our code base. The class names were changed to Department and Employee to pick a simple real life example. Upon extraction of the .zip file, you will find a README.txt file that will provide the remainder of instruction necessary to reproduce my test results. You could just review the clone() implementations in Department and Employee respectively to start if you like, they are quite simple. I am happy to create a defect against EclipseLink 1.0.1, but was deferring that exercise until I was certain it wasn't a faulty implementation in one or more of our overriding clone() methods. Please take a look when you have a chance and let me know how I should proceed. Thanks again, Doug -----Original Message----- From: eclipselink-users-bounces@xxxxxxxxxxx [mailto:eclipselink-users-bounces@xxxxxxxxxxx] On Behalf Of Tom Ware Sent: Friday, November 14, 2008 6:24 AM To: EclipseLink User Discussions Subject: Re: [eclipselink-users] Application Developer Responsibilitieswhenoverridingjava.lang.Object.clone() in EclipseLink JPA mapped classes? Hi Doug, The reason you are seeing an issue is fairly subtle. Your clone methods do something that is not recommended by the JPA specification. They directly access instance variables of an instance of an object from outside of that object. Here is what I mean: In this line: clonedEmployee.department = (Department) this.department.clone(); You are in an instance of "this", but you are directly accessing an instance variable of "clonedEmployee". We cannot weave this code correctly. I suggest changing to this: clonedEmployee.setDepartment((Department) this.department.clone()); With this change, the weaved code in the setDepartment() method will take care of the valueholders and the instance variable access will be within clonedEmployee through it's setDepartment() method. -Tom Gschwind, Doug wrote: > Hi Tom, > > I have some further refinements on topic to discuss. My hypothesis is > that one of our clone() methods does not provide the opportunity for the > EclipseLink weaving to execute, and thus yields undesired results. So, I > am hoping you can review the following and let me know if you think this > clone() method is incorrect, or if there is some possible defect in > EclipseLink (we are using 1.0.1). > > Suppose we have a OneToMany from Department to Employee, and a back > pointing ManyToOne from Employee to Department, and EclipseLink weaving > is enabled for both of these JPA mapped classes. Lets also assume that > both Department and Employee extend Object (but in my test case they are > a level or two subclassed from Object). Consider the following > overriding clone() methods on Department and Employee: > > Department.java > --------------- > @OneToMany > private List<Employee> employees; > > @Override > public Object clone() > { > Department clonedDepartment = (Department) super.clone(); > > clonedDepartment.employees = new ArrayList<Employee>(); > > for (Employee sourceEmployee : this.employees) > { > Employee clonedEmployee = (Employee) sourceEmployee.clone(); > clonedDepartment.addEmployee(clonedEmployee); > clonedEmployee.setDepartment(result); > } > > return clonedDepartment; > } > > Employee.java > ------------- > @ManyToOne > private Department department; > > @Override > public Object clone() > { > Employee clonedEmployee = (Employee) super.clone(); > > // Cannot include the following line due to infinite recursion > // but it seems we need a way to allow the underlying ValueHolder > // that supports the department instance to be cloned. How to do > // that without calling clone() against the Department instance? > //clonedEmployee.department = (Department) this.department.clone(); > > return clonedEmployee; > } > > When a Department (D) with three Employees (E1,E2,E3) is cloned, the > cloned Department (D') instance has three Employee clone instances > (E1',E2',E3'), but the original three Employee instances all now point > to the Department clone (D') instead of the original Department instance > (D). The original Department instance should be treated as read only. > > This would make sense if the ValueHolder for the Department in each > Employee instance is in fact the same ValueHolder reference due to the > shallow copy results of super.clone() in Employee.clone(). Changing the > department reference in one (e.g. E1, E1') would change the reference > for both. > > Clearly if the Employee clone() method were implemented to construct a > new Employee instance via an Employee constructor for instance, this > problem would not be possible due to the different ValueHolder instances > held in the newly constructed Employee instance. We have some investment > in the clone() framework though, so we are attempting to not disturb > that if possible. > > Let me know your thoughts. It feels to me that the ValueHolder on the > Employee class for the Department needs to be copied (but not its > contents as we will be resetting the Department reference immediately > after Employee.clone() returns control to Department.clone()), but not > sure how to get there. > > Thanks, > > Doug > > -----Original Message----- > From: eclipselink-users-bounces@xxxxxxxxxxx > [mailto:eclipselink-users-bounces@xxxxxxxxxxx] On Behalf Of Tom Ware > Sent: Thursday, November 13, 2008 10:57 AM > To: EclipseLink User Discussions > Subject: Re: [eclipselink-users] Application Developer Responsibilities > whenoverriding java.lang.Object.clone() in EclipseLink JPA mapped > classes? > > Hi Doug, > > As long as the clone methods on the targets of your relationships are > > implemented correctly, you should be fine with cloning the way you > suggest. > > When weaving is enabled, EclipseLink does weave in a > _persistence_post_clone() method. That code is added after the call to > super.clone() and does some clean-up of the weaved code. > > Are you seeing problems? > > -Tom > > Gschwind, Doug wrote: >> Hello everyone, >> >> >> >> I didn't find any information on the wiki on this topic, and that may >> simply be due to no change in the contract regarding how overriding >> implementations of clone() should be implemented in EclipseLink JPA >> mapped classes. However, I am looking for confirmation of that. >> >> >> >> Suppose I have a JPA mapped class, with relationships to other JPA >> mapped classes as follows: >> >> >> >> @Entity >> >> public class Container extends Object >> >> { >> >> @OneToOne >> >> private A a; >> >> >> >> @ManyToOne >> >> private B b; >> >> >> >> @OneToMany >> >> private List<C> cs; >> >> >> >> @ManyToMany >> >> private List<D> ds; >> >> } >> >> >> >> Now suppose I wish to create instances of Container by cloning >> persistent instances. What I am looking for are any differences in the > >> overriding clone() implementation if the Container class were defined > as >> above, or if the Container class was not JPA mapped and had none of > the >> above annotations. Would the following method suffice within the JPA >> mapped Container class, assuming the overriding implementation of >> clone() on the JPA mapped A, B, C, and D classes were equally > appropriate: >> >> >> @Override >> >> public Object clone() >> >> { >> >> Container result = (Container) super.clone(); >> >> >> >> result.a = (A) this.a.clone(); >> >> result.b = (B) this.b.clone(); >> >> >> >> result.cs = new ArrayList<C>(this.cs.size()); >> >> for (C sourceC : this.cs) >> >> { >> >> C cloneC = (C) sourceC.clone(); >> >> result.cs.add(cloneC); >> >> cloneC.setContainer(result); >> >> } >> >> >> >> result.ds = new ArrayList<D>(this.ds.size()); >> >> for (D sourceD : this.ds) >> >> { >> >> D cloneD = (D) sourceD.clone(); >> >> result.ds.add(cloneD); >> >> cloneD.setContainer(result); >> >> } >> >> } >> >> >> >> If not, what differences in the above implementation would be >> recommended for use in the JPA mapped Container class? >> >> >> >> Thanks, >> >> >> >> Doug >> >> >> >> >> >> >> >> >> The contents of this electronic mail message and any attachments are > confidential, possibly privileged and intended >> for the addressee(s) only. Only the addressee(s) may read, > disseminate, retain or otherwise use this message. If >> received in error, please immediately inform the sender and then > delete this message without disclosing its contents >> to anyone. >> >> >> > ------------------------------------------------------------------------ >> _______________________________________________ >> eclipselink-users mailing list >> eclipselink-users@xxxxxxxxxxx >> https://dev.eclipse.org/mailman/listinfo/eclipselink-users > _______________________________________________ > eclipselink-users mailing list > eclipselink-users@xxxxxxxxxxx > https://dev.eclipse.org/mailman/listinfo/eclipselink-users > _______________________________________________ > eclipselink-users mailing list > eclipselink-users@xxxxxxxxxxx > https://dev.eclipse.org/mailman/listinfo/eclipselink-users _______________________________________________ eclipselink-users mailing list eclipselink-users@xxxxxxxxxxx https://dev.eclipse.org/mailman/listinfo/eclipselink-users
Attachment:
DepartmentCloneErrorTestCase.zip
Description: DepartmentCloneErrorTestCase.zip