Wonseok, Markus,
I think the original proposal was closer to a better solution. Adding another method to ObjectBuilder adds a second merge path that will need to be maintained in the future.
I think that if we refactor MergeManager a bit we can have MergeManger not perform the 'short-circuit' when called from a RepeatableWriteUnitOfWork. This will isolate the cascade merging behaviour to the EJB30 implemetation.
--Gordon
-----Original Message-----
From: Wonseok Kim [mailto:guruwons_at_gmail.com]
Sent: Thursday, September 21, 2006 9:25 PM
To: persistence_at_glassfish.dev.java.net
Subject: Re: Fix for issue 1139 "cascade merge"
Thanks Markus for reviewing the fix.
I'll try to migrate the test into entity-persistence-tests framework.
I added a cascade-merge method into ObjectBuilder as Markus commented. There are other cascadeXXX methods in ObjectBuilder, so I named it "cascadeMergeIntoObject".
/**
* INTERNAL:
* Cascade merge of the contents of a registered object, this merge algorthim is dependent on the merge manager.
*/
public void cascadeMergeIntoObject(Object object, boolean isUnInitialized, MergeManager mergeManager) {
// PERF: Avoid synchronized enumerator as is concurrency bottleneck.
Vector mappings = getDescriptor().getMappings();
for (int index = 0; index < mappings.size(); index++) {
DatabaseMapping mapping = (DatabaseMapping)mappings.get(index);
if(mapping instanceof ForeignReferenceMapping
&& ((ForeignReferenceMapping)mapping).shouldMergeCascadeParts(mergeManager)){
mapping.mergeIntoObject(object, isUnInitialized, object, mergeManager);
}
}
}
cascade merge worked well for one-to-one, one-to-many and many-to-many relationships.
Thanks
- Wonseok
On 9/22/06, Markus Fuchs <Markus.Fuchs_at_sun.com> wrote:
Hi Wonseok, TopLink team,
I took a look at Wonseok's fix and it looks fine to me. I have the
same questions to the TopLink team as asked in the original mail. A
few remarks to the fix:
1) Wonseok's original email had a zip attachment. AFAIK, the TopLink
team can't receive emails with .zip attachment. Please always use
.jar archives. I'm attaching the content of the .zip file as a jar.
2) In MergeManager.java:
a) Consider removing the unused variable 'changeTracked'
b) Consider adding a new method ObjectBuilder.mergeIntoObjectCascade(...),
which basically implements the logic you've added to MergeManager in lines
409-418. MergeManager would simply call this method, as in:
...
if(registeredObject == rmiClone){
// GF#1139 Cascade merge operations even if it's already managed
builder.mergeIntoObjectCascade(registeredObject, false, this);
} else {
// Merge into the clone from the original, use clone as backup as backup as anything different should be merged.
builder.mergeIntoObject(registeredObject, false, rmiClone, this);
}
...
Note, that the new method has only a target argument, since
registeredObject == rmiClone
3) Please migrate CascadeTest to use the JUnit framework provided in
entity-persistence-tests.
4) The test class EntityA has an OneToOne and OneToMany
relationship. Please consider adding a ManyToMany relationship as well.
5) CascadeTest is setting the OneToOne relationship only on the
owning side. Please also set the relationship on the non-owning
side.
6) You might want to make sure that the objects associated to ma1 are
clones of the objects originally added to a1.
7) Please consider renaming the field EntityA.c to EntityA.cs, as it
is a collection field.
Thanks,
-- markus.
Wonseok Kim wrote:
Hi, Tom
I'm investigating the issue 1139 "Cascade doesn't work when merging managed entity"
https://glassfish.dev.java.net/issues/show_bug.cgi?id=1139
I don't know that there is anyone working on this. Now I'm trying to fix this.
MergeManager.mergeChangesOfCloneIntoWorkingCopy(Object rmiClone) just returns if the clone is already registred one.
So I modified it to traverse ForeignReferenceMapping mappings and trigger mapping.mergeIntoObject() if cascade merge is required. See below diffs.
Some questions:
* Is calling mapping.mergeIntoObject() with same source and target okay?
* ObjectBuilder.mergeIntoObject() also triggers PostMergeEvent. Is this required too for this type of cascade merge? (JPA doesn't require this event, but TopLink?)
I tested the fix with an attached testcase and it worked well. Also there was no entity-persistence-tests failure.
Please review and instruct me.
Index: src/java/oracle/toplink/essentials/internal/sessions/MergeManager.java
===================================================================
RCS file: /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/internal/sessions/MergeManager.java,v
retrieving revision 1.6
diff -c -r1.6 MergeManager.java
*** src/java/oracle/toplink/essentials/internal/sessions/MergeManager.java 20 Apr 2006 20:32:05 -0000 1.6
--- src/java/oracle/toplink/essentials/internal/sessions/MergeManager.java 21 Sep 2006 16:01:58 -0000
***************
*** 387,402 ****
ClassDescriptor descriptor = getSession().getDescriptor(rmiClone);
Object registeredObject = registerObjectForMergeCloneIntoWorkingCopy(rmiClone);
- if (registeredObject == rmiClone) {
- //need to find better better fix. prevents merging into itself.
- return rmiClone;
- }
-
boolean changeTracked = false;
try {
ObjectBuilder builder = descriptor.getObjectBuilder ();
! if (descriptor.usesVersionLocking()) {
VersionLockingPolicy policy = (VersionLockingPolicy) descriptor.getOptimisticLockingPolicy();
if (policy.isStoredInObject()) {
Object currentValue = builder.extractValueFromObjectForField(registeredObject, policy.getWriteLockField(), session);
--- 387,397 ----
ClassDescriptor descriptor = getSession().getDescriptor(rmiClone);
Object registeredObject = registerObjectForMergeCloneIntoWorkingCopy(rmiClone);
boolean changeTracked = false;
try {
ObjectBuilder builder = descriptor.getObjectBuilder ();
! if (registeredObject != rmiClone && descriptor.usesVersionLocking()) {
VersionLockingPolicy policy = (VersionLockingPolicy) descriptor.getOptimisticLockingPolicy();
if (policy.isStoredInObject()) {
Object currentValue = builder.extractValueFromObjectForField(registeredObject, policy.getWriteLockField(), session);
***************
*** 410,417 ****
// Toggle change tracking during the merge.
descriptor.getObjectChangePolicy().dissableEventProcessing(registeredObject);
! // Merge into the clone from the original, use clone as backup as anything different should be merged.
! builder.mergeIntoObject(registeredObject, false, rmiClone, this);
} finally {
descriptor.getObjectChangePolicy().enableEventProcessing(registeredObject);
}
--- 405,425 ----
// Toggle change tracking during the merge.
descriptor.getObjectChangePolicy().dissableEventProcessing(registeredObject);
! if(registeredObject == rmiClone){
! // GF#1139 Cascade merge operations even if it's already managed
! Vector mappings = descriptor.getMappings();
! for (int index = 0; index < mappings.size(); index++) {
! DatabaseMapping mapping = (DatabaseMapping)mappings.get(index);
! if(mapping instanceof ForeignReferenceMapping
! && ((ForeignReferenceMapping)mapping).shouldMergeCascadeParts(this)){
! mapping.mergeIntoObject(registeredObject, false, rmiClone, this);
! }
! }
!
! } else {
! // Merge into the clone from the original, use clone as backup as anything different should be merged.
! builder.mergeIntoObject(registeredObject, false, rmiClone, this);
! }
} finally {
descriptor.getObjectChangePolicy ().enableEventProcessing(registeredObject);
}
Thanks
--
Wonseok Kim
Senior Developer, TmaxSoft