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