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
> > >
> >
>