diff options
author | Caspar De Groot | 2010-07-28 01:28:01 -0400 |
---|---|---|
committer | Caspar De Groot | 2010-07-28 01:28:01 -0400 |
commit | 39637a84031288c721223bf35e098eb523346902 (patch) | |
tree | 2fac0d441812557a373b4d19a0efc895367850d4 | |
parent | 5087a7a0de57243b4d40a4d85e383df85b6a16ab (diff) | |
download | cdo-39637a84031288c721223bf35e098eb523346902.zip cdo-39637a84031288c721223bf35e098eb523346902.tar.gz cdo-39637a84031288c721223bf35e098eb523346902.tar.xz |
[320369] Detach-reattach of dirty object discards pre-detach featureDeltas
https://bugs.eclipse.org/bugs/show_bug.cgi?id=320369
4 files changed, 447 insertions, 19 deletions
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllConfigs.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllConfigs.java index 178c4db..f35c9ca 100644 --- a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllConfigs.java +++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllConfigs.java @@ -79,6 +79,7 @@ import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_316145_Test; import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_316434_Test; import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_318844_Test; import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_318876_Test; +import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_319836_Test; import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_320837_Test; import org.eclipse.emf.cdo.tests.config.impl.ConfigTest; import org.eclipse.emf.cdo.tests.config.impl.ConfigTestSuite; @@ -222,6 +223,7 @@ public abstract class AllConfigs extends ConfigTestSuite testClasses.add(Bugzilla_316434_Test.class); testClasses.add(Bugzilla_318844_Test.class); testClasses.add(Bugzilla_318876_Test.class); + testClasses.add(Bugzilla_319836_Test.class); testClasses.add(Bugzilla_320837_Test.class); // TODO testClasses.add(NonCDOResourceTest.class); diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_319836_Test.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_319836_Test.java new file mode 100644 index 0000000..00264d9 --- /dev/null +++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_319836_Test.java @@ -0,0 +1,407 @@ +/** + * Copyright (c) 2004 - 2010 Eike Stepper (Berlin, Germany) and others. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Eike Stepper - initial API and implementation + */ +package org.eclipse.emf.cdo.tests.bugzilla; + +import org.eclipse.emf.cdo.common.id.CDOID; +import org.eclipse.emf.cdo.common.revision.delta.CDOFeatureDelta; +import org.eclipse.emf.cdo.common.revision.delta.CDOListFeatureDelta; +import org.eclipse.emf.cdo.common.revision.delta.CDORevisionDelta; +import org.eclipse.emf.cdo.eresource.CDOResource; +import org.eclipse.emf.cdo.server.IRepository.WriteAccessHandler; +import org.eclipse.emf.cdo.server.IStoreAccessor.CommitContext; +import org.eclipse.emf.cdo.server.ITransaction; +import org.eclipse.emf.cdo.session.CDOSession; +import org.eclipse.emf.cdo.spi.common.revision.InternalCDORevisionDelta; +import org.eclipse.emf.cdo.tests.AbstractCDOTest; +import org.eclipse.emf.cdo.tests.model3.NodeA; +import org.eclipse.emf.cdo.transaction.CDOTransaction; +import org.eclipse.emf.cdo.util.CDOUtil; +import org.eclipse.emf.cdo.view.CDOView; + +import org.eclipse.net4j.util.om.monitor.OMMonitor; + +import org.eclipse.emf.edit.command.DragAndDropCommand; + +import java.text.MessageFormat; + +/** + * Moving nodes in a tree structure (simulate a {@link DragAndDropCommand}) may result in an inconsistent tree. + * <p> + * See bug 319836 + * + * @author Cyril Jaquier + */ +public class Bugzilla_319836_Test extends AbstractCDOTest +{ + private static final boolean SHOW_BUG = true; + + public void testNodeMovesInTreeCreatesCycle() throws Exception + { + { + // Setup an initial session and a transaction. + CDOSession session = openSession(); + CDOTransaction tr1 = session.openTransaction(); + CDOResource resource = tr1.createResource("/test1"); + + NodeA n1 = getModel3Factory().createNodeA(); + NodeA n2 = getModel3Factory().createNodeA(); + NodeA n3 = getModel3Factory().createNodeA(); + + resource.getContents().add(n1); + + // Create a deep tree. + // + // n1 + // `- n2 + // ___`- n3 + n1.getChildren().add(n2); + n2.getChildren().add(n3); + + tr1.commit(); + setNames(n1, n2, n3); + tr1.commit(); + addCommitContextTracer(); + + // First move. + // + // n1 + // |- n2 + // `- n3 + if (SHOW_BUG) + { + n2.getChildren().remove(n3); + } + + n1.getChildren().add(n3); + + // Second move. + // + // n1 + // `- n3 + // ___`- n2 + + // Something bad will happen during the execution of the next line. Set a breakpoint in + // org.eclipse.emf.internal.cdo.transaction.CDOSavepointImpl.detachedObjects.new HashMap<CDOID,CDOObject>() + // {...}.put(CDOID, CDOObject) and see how the previous REMOVE is eaten. + if (SHOW_BUG) + { + n1.getChildren().remove(n2); + } + + n3.getChildren().add(n2); + + // Problem is with n2; the removal of its child has been lost + CDOID n2ID = CDOUtil.getCDOObject(n2).cdoID(); + CDORevisionDelta revDelta = tr1.getRevisionDeltas().get(n2ID); + assertEquals(2, revDelta.getFeatureDeltas().size()); + + tr1.commit(); + + // Checks the tree. + assertEquals(1, n1.getChildren().size()); + assertEquals(0, n2.getChildren().size()); + assertEquals(1, n3.getChildren().size()); + } + + { + // Setup a new session. The bug only does not seem to occur if we use the same session. + CDOSession session = openSession(); + CDOView view = session.openView(); + CDOResource resource = view.getResource("/test1"); + + NodeA n1 = (NodeA)resource.getContents().get(0); + + // Checks the tree. + assertEquals(1, n1.getChildren().size()); + NodeA n3 = n1.getChildren().get(0); + assertEquals(1, n3.getChildren().size()); + NodeA n2 = n3.getChildren().get(0); + // Oups... n2 has a child!? And even worst... it's n3 :'( Houston, we have a problem... + assertEquals(0, n2.getChildren().size()); + } + } + + public void testNodeMovesInTreeDuplicatesNode() throws Exception + { + { + // Setup an initial session and a transaction. + CDOSession session = openSession(); + CDOTransaction tr1 = session.openTransaction(); + CDOResource resource = tr1.createResource("/test1"); + + NodeA n1 = getModel3Factory().createNodeA(); + NodeA n2 = getModel3Factory().createNodeA(); + NodeA n3 = getModel3Factory().createNodeA(); + NodeA n4 = getModel3Factory().createNodeA(); + + resource.getContents().add(n1); + + // Create a deep tree. + // + // n1 + // `- n2 + // ___`- n3 + // ______`- n4 + n1.getChildren().add(n2); + n2.getChildren().add(n3); + n3.getChildren().add(n4); + + tr1.commit(); + setNames(n1, n2, n3); + tr1.commit(); + addCommitContextTracer(); + + // First move. + // + // n1 + // `- n2 + // ___|- n3 + // ___`- n4 + if (SHOW_BUG) + { + n3.getChildren().remove(n4); + } + + n2.getChildren().add(n4); + + // Second move. + // + // n1 + // |- n2 + // |__`- n4 + // `- n3 + + // Something bad will happen during the execution of the next line. Set a breakpoint in + // org.eclipse.emf.internal.cdo.transaction.CDOSavepointImpl.detachedObjects.new HashMap<CDOID,CDOObject>() + // {...}.put(CDOID, CDOObject) and see how the previous REMOVE is eaten. + if (SHOW_BUG) + { + n2.getChildren().remove(n3); + } + + n1.getChildren().add(n3); + + tr1.commit(); + + // Checks the tree. + assertEquals(2, n1.getChildren().size()); + assertEquals(1, n2.getChildren().size()); + assertEquals(0, n3.getChildren().size()); + assertEquals(0, n4.getChildren().size()); + } + + { + // Setup a new session. The bug only does not seem to occur if we use the same session. + CDOSession session = openSession(); + CDOView view = session.openView(); + CDOResource resource = view.getResource("/test1"); + + NodeA n1 = (NodeA)resource.getContents().get(0); + + // Checks the tree. + assertEquals(2, n1.getChildren().size()); + NodeA n2 = n1.getChildren().get(0); + assertEquals(1, n2.getChildren().size()); + NodeA n4 = n2.getChildren().get(0); + assertEquals(0, n4.getChildren().size()); + NodeA n3 = n1.getChildren().get(1); + // Oups... n3 has a child!? + assertEquals(0, n3.getChildren().size()); + } + } + + public void testNodeMovesInTreeEatsNode() throws Exception + { + { + // Setup an initial session and a transaction. + CDOSession session = openSession(); + CDOTransaction tr1 = session.openTransaction(); + CDOResource resource = tr1.createResource("/test1"); + + NodeA n1 = getModel3Factory().createNodeA(); + NodeA n2 = getModel3Factory().createNodeA(); + NodeA n3 = getModel3Factory().createNodeA(); + NodeA n4 = getModel3Factory().createNodeA(); + + resource.getContents().add(n1); + + // Create a flat tree. + // + // n1 + // |- n2 + // |- n3 + // `- n4 + n1.getChildren().add(n2); + n1.getChildren().add(n3); + n1.getChildren().add(n4); + + tr1.commit(); + setNames(n1, n2, n3); + tr1.commit(); + addCommitContextTracer(); + + // First move. + // + // n1 + // |- n2 + // `- n3 + // ___`- n4 + if (SHOW_BUG) + { + n1.getChildren().remove(n4); + } + + n3.getChildren().add(n4); + + // Second move. + // + // n1 + // `- n2 + // ___`- n3 + // ______`- n4 + + // Something bad will happen during the execution of the next line. Set a breakpoint in + // org.eclipse.emf.internal.cdo.transaction.CDOSavepointImpl.detachedObjects.new HashMap<CDOID,CDOObject>() + // {...}.put(CDOID, CDOObject) and see how the previous ADD is eaten. + if (SHOW_BUG) + { + n1.getChildren().remove(n3); + } + + n2.getChildren().add(n3); + + tr1.commit(); + + // Checks the tree. + assertEquals(1, n1.getChildren().size()); + assertEquals(n2, n1.getChildren().get(0)); + assertEquals(1, n2.getChildren().size()); + assertEquals(n3, n2.getChildren().get(0)); + assertEquals(1, n3.getChildren().size()); + assertEquals(n4, n3.getChildren().get(0)); + assertEquals(0, n4.getChildren().size()); + } + + { + // Setup a new session. The bug only does not seem to occur if we use the same session. + CDOSession session = openSession(); + CDOView view = session.openView(); + CDOResource resource = view.getResource("/test1"); + + NodeA n1 = (NodeA)resource.getContents().get(0); + + // Checks the tree. + assertEquals(1, n1.getChildren().size()); + NodeA n2 = n1.getChildren().get(0); + assertEquals(1, n2.getChildren().size()); + NodeA n3 = n2.getChildren().get(0); + // Oups... Where is n4!? + assertEquals(1, n3.getChildren().size()); + NodeA n4 = n3.getChildren().get(0); + assertEquals(0, n4.getChildren().size()); + } + } + + private void setNames(NodeA... nodes) + { + for (NodeA node : nodes) + { + String id = CDOUtil.getCDOObject(node).cdoID().toString(); + msg("Node: " + id); + node.setName(id); + } + } + + private void addCommitContextTracer() + { + getRepository().addHandler(new WriteAccessHandler() + { + public void handleTransactionBeforeCommitting(ITransaction transaction, CommitContext commitContext, + OMMonitor monitor) throws RuntimeException + { + msg(toPrettyString("\t", commitContext.getDirtyObjectDeltas())); + } + + public void handleTransactionAfterCommitted(ITransaction transaction, CommitContext commitContext, + OMMonitor monitor) + { + } + + /** + * Prints the {@link InternalCDORevisionDelta}s in a more friendly way than {@link #toString()}. + * + * @param spacer + * the spacer used to increment the output + * @return {@link String} + */ + private String toPrettyString(String spacer, InternalCDORevisionDelta[] dirtyObjects) + { + StringBuilder sb = new StringBuilder(); + + // Delta. + sb.append(spacer).append("Delta revision(s):\n"); + for (InternalCDORevisionDelta item : dirtyObjects) + { + String m = MessageFormat.format("{0}@{1}:{2}v{3}", item.getEClass().getName(), item.getID(), item.getBranch() + .getID(), item.getVersion()); + sb.append(spacer).append(spacer).append(m).append("\n"); + // Feature deltas. + for (CDOFeatureDelta delta : item.getFeatureDeltas()) + { + printFeatureDelta(sb, delta, spacer, 3); + } + } + + return sb.toString(); + } + + /** + * Pretty prints {@link CDOFeatureDelta}, recursing into {@link CDOListFeatureDelta}. + * + * @param sb + * {@link StringBuilder} where the output is written + * @param delta + * {@link CDOFeatureDelta} + * @param spacer + * the spacer used to increment the output + * @param numberOfSpacer + * the minimal number of spacer in front of a line + */ + private void printFeatureDelta(StringBuilder sb, CDOFeatureDelta delta, String spacer, int numberOfSpacer) + { + if (delta instanceof CDOListFeatureDelta) + { + CDOListFeatureDelta list = (CDOListFeatureDelta)delta; + for (int i = 0; i < numberOfSpacer; i++) + { + sb.append(spacer); + } + + sb.append("CDOListFeatureDelta").append("\n"); + for (CDOFeatureDelta c : list.getListChanges()) + { + sb.append(spacer); + printFeatureDelta(sb, c, spacer, numberOfSpacer); + } + } + else + { + for (int i = 0; i < numberOfSpacer; i++) + { + sb.append(spacer); + } + + sb.append(delta).append("\n"); + } + } + }); + } +} diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/CDOStateMachine.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/CDOStateMachine.java index 843db11..9a72b48 100644 --- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/CDOStateMachine.java +++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/CDOStateMachine.java @@ -31,6 +31,7 @@ import org.eclipse.emf.cdo.view.CDOInvalidationPolicy; import org.eclipse.emf.cdo.view.CDOView; import org.eclipse.emf.internal.cdo.bundle.OM; +import org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl; import org.eclipse.emf.internal.cdo.util.FSMUtil; import org.eclipse.net4j.util.collection.Pair; @@ -205,8 +206,13 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent private void attachOrReattach(InternalCDOObject object, InternalCDOTransaction transaction) { + if (!(transaction instanceof CDOTransactionImpl)) + { + throw new UnsupportedOperationException("Transaction does not support the getFormerRevisionKeys() method"); + } + // Bug 283985 (Re-attachment) - if (transaction.getFormerRevisions().containsKey(object)) + if (((CDOTransactionImpl)transaction).getFormerRevisionKeys().containsKey(object)) { reattachObject(object, transaction); } @@ -570,9 +576,12 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent InternalCDOTransaction transaction = transactionAndContents.getElement1(); List<InternalCDOObject> contents = transactionAndContents.getElement2(); - Map<InternalCDOObject, InternalCDORevision> formerRevisionMap = transaction.getFormerRevisions(); - boolean reattaching = formerRevisionMap.containsKey(object); + if (!(transaction instanceof CDOTransactionImpl)) + { + throw new UnsupportedOperationException("Transaction does not support the getFormerRevisionKeys() method"); + } + boolean reattaching = ((CDOTransactionImpl)transaction).getFormerRevisionKeys().containsKey(object); if (!reattaching) { // Prepare object @@ -682,9 +691,14 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent public void execute(InternalCDOObject object, CDOState state, CDOEvent event, InternalCDOTransaction transaction) { InternalCDORevisionManager revisionManager = transaction.getSession().getRevisionManager(); - InternalCDORevision formerRevision = transaction.getFormerRevisions().get(object); - CDOID id = formerRevision.getID(); + if (!(transaction instanceof CDOTransactionImpl)) + { + throw new UnsupportedOperationException("Transaction does not support the getFormerRevisionKeys() method"); + } + + CDORevisionKey revKey = ((CDOTransactionImpl)transaction).getFormerRevisionKeys().get(object); + CDOID id = revKey.getID(); object.cdoInternalSetID(id); object.cdoInternalSetView(transaction); @@ -693,16 +707,15 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent InternalCDORevision revision = (InternalCDORevision)factory.createRevision(object.eClass()); revision.setID(id); revision.setBranchPoint(transaction.getBranch().getHead()); - revision.setVersion(formerRevision.getVersion()); + revision.setVersion(revKey.getVersion()); // Populate the revision based on the values in the CDOObject object.cdoInternalSetRevision(revision); object.cdoInternalPostAttach(); // Compute a revision delta and register it with the tx - // CDOBranchVersion branchVersion = transaction.getBranch().getVersion(revision.getVersion()); - // CDORevision originalRevision = revisionManager.getRevisionByVersion(id, branchVersion, -1, true); - CDORevisionDelta revisionDelta = CDORevisionDeltaUtil.create(formerRevision, revision); + CDORevision cleanRevision = revisionManager.getRevisionByVersion(id, revKey, -1, true); + CDORevisionDelta revisionDelta = CDORevisionDeltaUtil.create(cleanRevision, revision); transaction.registerRevisionDelta(revisionDelta); transaction.registerDirty(object, null); changeState(object, CDOState.DIRTY); diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/transaction/CDOTransactionImpl.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/transaction/CDOTransactionImpl.java index 6b3ac9e..e2ac16c 100644 --- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/transaction/CDOTransactionImpl.java +++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/transaction/CDOTransactionImpl.java @@ -41,6 +41,7 @@ import org.eclipse.emf.cdo.common.revision.CDOListFactory; import org.eclipse.emf.cdo.common.revision.CDORevision; import org.eclipse.emf.cdo.common.revision.CDORevisionFactory; import org.eclipse.emf.cdo.common.revision.CDORevisionKey; +import org.eclipse.emf.cdo.common.revision.CDORevisionUtil; import org.eclipse.emf.cdo.common.revision.delta.CDOFeatureDelta; import org.eclipse.emf.cdo.common.revision.delta.CDORevisionDelta; import org.eclipse.emf.cdo.common.revision.delta.CDORevisionDeltaUtil; @@ -137,7 +138,6 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Set; -import java.util.WeakHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.locks.ReentrantLock; @@ -175,7 +175,7 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa private String commitComment; // Bug 283985 (Re-attachment) - private WeakHashMap<InternalCDOObject, InternalCDORevision> formerRevisions = new WeakHashMap<InternalCDOObject, InternalCDORevision>(); + private Map<InternalCDOObject, CDORevisionKey> formerRevisionKeys = new HashMap<InternalCDOObject, CDORevisionKey>(); // Bug 283985 (Re-attachment) private final ThreadLocal<Boolean> providingCDOID = new InheritableThreadLocal<Boolean>() @@ -1157,9 +1157,10 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa else { getLastSavepoint().getDetachedObjects().put(id, object); - if (!formerRevisions.containsKey(object)) + if (!formerRevisionKeys.containsKey(object)) { - formerRevisions.put(object, object.cdoRevision()); + CDORevisionKey revKey = CDORevisionUtil.createRevisionKey(object.cdoRevision()); + formerRevisionKeys.put(object, revKey); } // Object may have been reattached previously, in which case it must @@ -1518,7 +1519,7 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa firstSavepoint.getSharedDetachedObjects().clear(); // Bug 283985 (Re-attachment) - formerRevisions.clear(); + formerRevisionKeys.clear(); dirty = false; conflict = 0; @@ -1745,9 +1746,14 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa return lastSavepoint.getAllDetachedObjects(); } + public Map<InternalCDOObject, CDORevisionKey> getFormerRevisionKeys() + { + return formerRevisionKeys; + } + public Map<InternalCDOObject, InternalCDORevision> getFormerRevisions() { - return formerRevisions; + throw new UnsupportedOperationException("This method is no longer supported. Call getFormerRevisionKeys() instead."); } @Override @@ -1757,16 +1763,16 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa // The super implementation will return null for a transient (unattached) object; // but in a tx, an transient object may previously have been attached, so we consult - // the formerRevisions -- unless this is being called indirectly through provideCDOID. + // the formerIDs -- unless this is being called indirectly through provideCDOID. // The latter case occurs when deltas or revisions are being written out to a stream; in // which case null must be returned (for transients) so that the caller will detect a // dangling reference if (!providingCDOID.get().booleanValue() && id == null) { - CDORevision formerRevision = formerRevisions.get(object); - if (formerRevision != null) + CDORevisionKey revKey = formerRevisionKeys.get(object); + if (revKey != null) { - id = formerRevision.getID(); + id = revKey.getID(); } } |