[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();
       }
     }