Hello Wonseok,
Perhaps this...
public void mergeIntoObject(Object target, boolean isUnInitialized, Object source, MergeManager mergeManager, boolean cascadeOnly) {
// 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( !cascadeOnly || mapping.isForeignReferenceMapping()){
//cascadeOnly = F, frm = X, merge = true;
//cascadeOnly = T, frm = T, merge = true;
//cascadeOnly = T, frm = F, merge = false;
mapping.mergeIntoObject (target, isUnInitialized, source, mergeManager);
}
}
}
This is a much cleaner method that performs optimally without the need for duplication of code. Calls to mergeIntoObject that should not be cascaded are already checked in mergeIntoObject code before any major work is attempted by these methods.
--Gordon
-----Original Message-----
From: Wonseok Kim [mailto:guruwons_at_gmail.com]
Sent: Wednesday, September 27, 2006 6:39 PM
To: persistence_at_glassfish.dev.java.net
Subject: Re1: Fix for issue 1139 "cascade merge"
Gordon,
Please review below message so I could submit the final fix.
If you agree with adding a new method "cascadeMergeIntoObject()", I think there is no more remained issue.
Thanks
- Wonseok
On 9/27/06, Wonseok Kim <guruwons_at_gmail.com> wrote:
Hello Gordon,
Removing the code block will work well, but I thought calling mergeIntoObject() for non-cascade mappings is unnecessary overhead - it does some comparison and invokes reflection call. If an entity has many mappings but there are a few or no cascade mappings the unnecessary cost will increase.
How about making a new method "cascadeMergeIntoObject()" which I mentioned in the second fix. Because adding a new flag to mergeIntoObject() something like below does not look good. It looks like making the method ...dirty. :-) Do you have a good idea?
public void mergeIntoObject(Object target, boolean isUnInitialized, Object source, MergeManager mergeManager, boolean cascadeOnly) {
// PERF: Avoid synchronized enumerator as is concurrency bottleneck.
Vector mappings = getDescriptor().getMappings();
if(cascadeOnly){
for (int index = 0; index < mappings.size(); index++) {
DatabaseMapping mapping = (DatabaseMapping)mappings.get(index);
if(mapping instanceof ForeignReferenceMapping
&& ((ForeignReferenceMapping)mapping).shouldMergeCascadeParts(mergeManager)){
mapping.mergeIntoObject (target, isUnInitialized, source, mergeManager);
}
}
} else {
for (int index = 0; index < mappings.size(); index++) {
((DatabaseMapping)mappings.get(index)).mergeIntoObject(target, isUnInitialized, source, mergeManager);
}
}
Thanks
-Wonseok
On 9/27/06, Gordon Yorke <gordon.yorke_at_oracle.com > wrote:
Hello Wonseok,
Looks good, I would remove the code block :
if(registeredObject == rmiClone){
// GF#1139 Cascade merge operations even if it's already registered
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);
}
}
As this is almost identical to what mergeIntoObject does now. I do not think there is a lot of value in limiting the merge to only foreign reference mappings but if you prefer to do that then it would be better to add that behaviour to the ObjectBuilder (perhaps through another flag).
--Gordon
-----Original Message-----
From: Wonseok Kim [mailto:guruwons_at_gmail.com]
Sent: Tuesday, September 26, 2006 1:51 AM
To: persistence_at_glassfish.dev.java.net
Subject: Re: Fix for issue 1139 "cascade merge"
Hello Gordon,
Overriding UoW.mergeCloneWithReferences() causes a code duplication, and introducing a new merge policy CLONE_INTO_WORKING_COPY_FORCE_CASCADE makes another complexity to maintain this policy.
Instead of the approach I think introducing a new force-cascade flag is cleaner solution. Also when I reconsider moving the force-cascade logic into ObjectBuilder.mergeIntoObject(), it seems not a good idea because mergeIntoObject() is called by several different places. So I left the logic in MergeManager.
Please review below.
MergeManager:
/** Force cascade merge even if a clone is already registered */
// GF#1139 Cascade doesn't work when merging managed entity
protected boolean forceCascade;
...
protected Object mergeChangesOfCloneIntoWorkingCopy(Object rmiClone) {
ClassDescriptor descriptor = getSession().getDescriptor(rmiClone);
Object registeredObject = registerObjectForMergeCloneIntoWorkingCopy(rmiClone);
if (registeredObject == rmiClone && !shouldForceCascade()) {
//need to find better better fix. prevents merging into itself.
return rmiClone;
}
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);
if (policy.isNewerVersion(currentValue, rmiClone, session.keyFromObject(rmiClone), session)) {
throw OptimisticLockException.objectChangedSinceLastMerge (rmiClone);
}
}
}
// Toggle change tracking during the merge.
descriptor.getObjectChangePolicy().dissableEventProcessing(registeredObject);
if(registeredObject == rmiClone){
// GF#1139 Cascade merge operations even if it's already registered
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);
}
return registeredObject;
}
...
To set forceCascade flag, UnitOfWorkImpl and EntityManagerImpl are modified as follows.
UnitOfWorkImpl:
public Object mergeCloneWithReferences(Object rmiClone, int cascadePolicy) {
return mergeCloneWithReferences(rmiClone, cascadePolicy, false);
}
...
public Object mergeCloneWithReferences(Object rmiClone, int cascadePolicy, boolean forceCascade) {
if (rmiClone == null) {
return null;
}
ClassDescriptor descriptor = getDescriptor(rmiClone);
if ((descriptor == null) || descriptor.isAggregateDescriptor() || descriptor.isAggregateCollectionDescriptor()) {
if (cascadePolicy == MergeManager.CASCADE_BY_MAPPING){
throw new IllegalArgumentException(ExceptionLocalization.buildMessage("not_an_entity", new Object[]{rmiClone}));
}
return rmiClone;
}
//CR#2272
logDebugMessage(rmiClone, "merge_clone_with_references");
ObjectBuilder builder = descriptor.getObjectBuilder ();
Object implementation = builder.unwrapObject(rmiClone, this);
MergeManager manager = new MergeManager(this);
manager.mergeCloneWithReferencesIntoWorkingCopy();
manager.setCascadePolicy (cascadePolicy);
manager.setForceCascade(forceCascade);
Object mergedObject = manager.mergeChanges(implementation, null);
if (isSmartMerge()) {
return builder.wrapObject(mergedObject, this);
} else {
return mergedObject;
}
}
EntityManagerImpl:
protected Object mergeInternal(Object entity){
verifyOpen();
if (entity == null){
throw new IllegalArgumentException(ExceptionLocalization.buildMessage("not_an_entity", new Object[] {entity}));
}
//gf830 - merging a removed entity should throw exception
if (getActivePersistenceContext(checkForTransaction(!isExtended())).getDeletedObjects().contains(entity)){
throw new IllegalArgumentException(ExceptionLocalization.buildMessage("cannot_merge_removed_entity", new Object[]{entity}));
}
try {
return getActivePersistenceContext(checkForTransaction(!isExtended())).mergeCloneWithReferences(entity, MergeManager.CASCADE_BY_MAPPING, true) ;
} catch (oracle.toplink.essentials.exceptions.OptimisticLockException ole) {
throw new javax.persistence.OptimisticLockException(ole);
}
}
Thanks
-Wonseok
On 9/26/06, Wonseok Kim <guruwons_at_gmail.com> wrote:
Hello Gordon, thanks for review.
Currently EntityManagerImpl is using UoW.mergeCloneWithReferences() instead of mergeClone(). So mergeCloneWithReferences() should be overrided as follows. right?
public Object mergeCloneWithReferences(Object rmiClone, int cascadePolicy) {
if (rmiClone == null) {
return null;
}
ClassDescriptor descriptor = getDescriptor(rmiClone);
if ((descriptor == null) || descriptor.isAggregateDescriptor() || descriptor.isAggregateCollectionDescriptor()) {
if (cascadePolicy == MergeManager.CASCADE_BY_MAPPING){
throw new IllegalArgumentException(ExceptionLocalization.buildMessage("not_an_entity", new Object[]{rmiClone}));
}
return rmiClone;
}
//CR#2272
// logDebugMessage(rmiClone, "merge_clone_with_references");
ObjectBuilder builder = descriptor.getObjectBuilder();
Object implementation = builder.unwrapObject(rmiClone, this);
MergeManager manager = new MergeManager(this);
manager.mergeCloneIntoWorkingCopyForceCascade();
manager.setCascadePolicy(cascadePolicy);
Object mergedObject = manager.mergeChanges(implementation, null);
if (isSmartMerge()) {
return builder.wrapObject(mergedObject, this);
} else {
return mergedObject;
}
}
I'm curious, Why mergeCloneWithReferences() code is different from mergeClone()?
It doesn't use SessionProfiler and handleException() as you see.
Also, to override this private logDebugMessage() should have protected access.
And the force-cascade logic which was in MergeManager can go to ObjectBuilder for avoding a duplicate like below.
public void mergeIntoObject(Object target, boolean isUnInitialized, Object source, 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(target != source
|| (mergeManager.shouldMergeCloneIntoWorkingCopyForceCascade()
&& mapping instanceof ForeignReferenceMapping
&& ((ForeignReferenceMapping)mapping).shouldMergeCascadeParts(mergeManager))){
// GF#1139 Cascade merge operations even if it's already managed
mapping.mergeIntoObject(target, isUnInitialized, source, mergeManager);
}
}
...
}
What do you think?
-Wonseok
On 9/25/06, Gordon Yorke < gordon.yorke_at_oracle.com> wrote:
Hello Wonseok,
A slight change will have to be made to your code as it is not backward compatible with current TopLink behaviour. Instead of altering the MergeManger to always cascade the merge and duplicating code already found within the ObjectBuilder the RepeatableWriteUnitOfWork (which is specific to EJB3.0) should request the cascaded merge from the MergeManager while the UnitOfWork continues to request the abortive merge.
The following method should be added to the RepeatableWriteUnitOfWork :
/**
* INTERNAL:
* Merge the attributes of the clone into the unit of work copy.
*/
public Object mergeClone(Object rmiClone, int cascadeDepth) {
if (rmiClone == null) {
return null;
}
//CR#2272
logDebugMessage(rmiClone, "merge_clone");
startOperationProfile(SessionProfiler.Merge);
ObjectBuilder builder = getDescriptor(rmiClone).getObjectBuilder();
Object implementation = builder.unwrapObject(rmiClone, this);
MergeManager manager = new MergeManager(this);
manager.mergeCloneIntoWorkingCopyForceCascade();
manager.setCascadePolicy(cascadeDepth);
Object merged = null;
try {
merged = manager.mergeChanges(implementation, null);
} catch (RuntimeException exception) {
merged = handleException(exception);
}
endOperationProfile(SessionProfiler.Merge);
return merged;
}
Then the MergeManager should have the following method and corresponding property added:
/**
* This can be used by the user for merging clones from RMI into the unit of work.
*/
public void mergeCloneIntoWorkingCopyForceCascade() {
setMergePolicy(CLONE_INTO_WORKING_COPY_FORCE_CASCADE);
}
Then update the MergeManager to check for this setting in two places:
mergeClone():
} else if (shouldMergeCloneIntoWorkingCopy() || shouldMergeCloneWithReferencesIntoWorkingCopy() || shouldMergeCloneIntoWorkingCopyForceCascade()) {
mergedObject = mergeChangesOfCloneIntoWorkingCopy(object);
and mergeChangesOfCloneIntoWorkingCopy() :
if (registeredObject== rmiClone && ! shouldMergeCloneIntoWorkingCopyForceCascade() ){
return rmiClone;
}
These changes preserve existing TopLink behaviour while updating the functionality for EJB 3.0 support and the code continues to use the current TopLink patterns for method call sequence.
--Gordon