[294528] Preserve CDOID for re-attached CDOObjects

https://bugs.eclipse.org/bugs/show_bug.cgi?id=294528

4 files changed, 44 insertions, 24 deletions

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 4f74be92f3..b15d0c6960 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
@@ -142,7 +142,7 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent
     init(CDOState.PROXY, CDOEvent.RELOAD, new ReloadTransition());
     init(CDOState.PROXY, CDOEvent.COMMIT, FAIL);
     init(CDOState.PROXY, CDOEvent.ROLLBACK, FAIL);
-    init(CDOState.PROXY, CDOEvent.REATTACH, FAIL); // Bug 283985 (Re-attachment) - Not sure about this one
+    init(CDOState.PROXY, CDOEvent.REATTACH, FAIL);
 
     init(CDOState.CONFLICT, CDOEvent.PREPARE, FAIL);
     init(CDOState.CONFLICT, CDOEvent.ATTACH, IGNORE);
@@ -154,7 +154,7 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent
     init(CDOState.CONFLICT, CDOEvent.RELOAD, FAIL);
     init(CDOState.CONFLICT, CDOEvent.COMMIT, IGNORE);
     init(CDOState.CONFLICT, CDOEvent.ROLLBACK, new RollbackTransition());
-    init(CDOState.CONFLICT, CDOEvent.REATTACH, FAIL); // Bug 283985 (Re-attachment) Not sure about this one
+    init(CDOState.CONFLICT, CDOEvent.REATTACH, FAIL);
 
     init(CDOState.INVALID, CDOEvent.PREPARE, InvalidTransition.INSTANCE);
     init(CDOState.INVALID, CDOEvent.ATTACH, InvalidTransition.INSTANCE);
@@ -166,7 +166,7 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent
     init(CDOState.INVALID, CDOEvent.RELOAD, InvalidTransition.INSTANCE);
     init(CDOState.INVALID, CDOEvent.COMMIT, InvalidTransition.INSTANCE);
     init(CDOState.INVALID, CDOEvent.ROLLBACK, InvalidTransition.INSTANCE);
-    init(CDOState.INVALID, CDOEvent.REATTACH, FAIL); // Bug 283985 (Re-attachment) Not sure about this one
+    init(CDOState.INVALID, CDOEvent.REATTACH, FAIL);
 
     init(CDOState.INVALID_CONFLICT, CDOEvent.PREPARE, InvalidTransition.INSTANCE);
     init(CDOState.INVALID_CONFLICT, CDOEvent.ATTACH, InvalidTransition.INSTANCE);
@@ -178,7 +178,7 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent
     init(CDOState.INVALID_CONFLICT, CDOEvent.RELOAD, InvalidTransition.INSTANCE);
     init(CDOState.INVALID_CONFLICT, CDOEvent.COMMIT, InvalidTransition.INSTANCE);
     init(CDOState.INVALID_CONFLICT, CDOEvent.ROLLBACK, DetachRemoteTransition.INSTANCE);
-    init(CDOState.INVALID_CONFLICT, CDOEvent.REATTACH, FAIL); // Bug 283985 (Re-attachment) Not sure about this one
+    init(CDOState.INVALID_CONFLICT, CDOEvent.REATTACH, FAIL);
   }
 
   /**
@@ -210,7 +210,7 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent
 
   private void attachOrReattach(InternalCDOObject object, InternalCDOTransaction transaction)
   {
-    // Bug 283985 (Re-attachment): Special case: re-attachment
+    // Bug 283985 (Re-attachment)
     if (transaction.getFormerRevisions().containsKey(object))
     {
       reattachObject(object, transaction);
@@ -769,7 +769,8 @@ public final class CDOStateMachine extends FiniteStateMachine<CDOState, CDOEvent
       else
       {
         // This branch only gets taken if the object that is being re-attached,
-        // was already DIRTY when it was first detached.
+        // was already DIRTY when it was first detached. In this case, the revision
+        // is already transactional; we clear it before repopulating it.
         //
         revision = formerRevision;
         for (int i = 0; i < eClass.getFeatureCount(); i++)
diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/transaction/CDOSavepointImpl.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/transaction/CDOSavepointImpl.java
index 997eb803be..b6f355667e 100644
--- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/transaction/CDOSavepointImpl.java
+++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/transaction/CDOSavepointImpl.java
@@ -69,6 +69,7 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
     }
   };
 
+  // Bug 283985 (Re-attachment)
   private Map<CDOID, CDOObject> reattachedObjects = new HashMap<CDOID, CDOObject>();
 
   private ConcurrentMap<CDOID, CDORevisionDelta> revisionDeltas = new ConcurrentHashMap<CDOID, CDORevisionDelta>();
@@ -139,7 +140,7 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
     revisionDeltas.clear();
     baseNewObjects.clear();
     detachedObjects.clear();
-    // TODO reattachedObjects.clear(); ???
+    reattachedObjects.clear();
   }
 
   public boolean isDirty()
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 5ebfe9afdf..9be5494a3b 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
@@ -124,6 +124,16 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa
   // Bug 283985 (Re-attachment)
   private WeakHashMap<InternalCDOObject, InternalCDORevision> formerRevisions = new WeakHashMap<InternalCDOObject, InternalCDORevision>();
 
+  // Bug 283985 (Re-attachment)
+  private final ThreadLocal<Boolean> providingCDOID = new InheritableThreadLocal<Boolean>()
+  {
+    @Override
+    protected Boolean initialValue()
+    {
+      return false;
+    }
+  };
+
   /**
    * @since 2.0
    */
@@ -1321,24 +1331,38 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa
   @Override
   protected CDOID getID(InternalCDOObject object, boolean onlyPersistedID)
   {
-    // Bug 283985 (Re-attachment): We consult the formerRevision map before delegating
-    // to the super implementation. Note that we *abuse* the onlyPersistedID
-    // argument to determine if we should disregard the formerRevision map. We need
-    // to disregard it when this gets called during commit, and it just so happens
-    // that only during commit it is being called with onlyPersistedID = false. So
-    // this code exploits a regularity in the calling code, rather than interpreting
-    // the onlyPersistedID argument in accordance with its intended meaning. Indeed
-    // this is a very fragile hack...
-    if (onlyPersistedID)
+    CDOID id = super.getID(object, onlyPersistedID);
+
+    // 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 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)
       {
-        return formerRevision.getID();
+        id = formerRevision.getID();
       }
     }
 
-    return super.getID(object, onlyPersistedID);
+    return id;
+  }
+
+  @Override
+  public CDOID provideCDOID(Object idOrObject)
+  {
+    try
+    {
+      providingCDOID.set(true);
+      return super.provideCDOID(idOrObject);
+    }
+    finally
+    {
+      providingCDOID.set(false);
+    }
   }
 
   /**
diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/CDOViewImpl.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/CDOViewImpl.java
index beff13a087..75d393ef09 100644
--- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/CDOViewImpl.java
+++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/CDOViewImpl.java
@@ -1026,12 +1026,6 @@ public class CDOViewImpl extends Lifecycle implements InternalCDOView
     return potentialObject;
   }
 
-  // TTT public CDOIDDangling convertDanglingObjectToID(InternalCDOObject source, EStructuralFeature feature,
-  // InternalEObject target)
-  // {
-  // throw new IllegalStateException("Dangling objects not possible outside of a transaction");
-  // }
-
   protected CDOID getID(InternalCDOObject object, boolean onlyPersistedID)
   {
     if (onlyPersistedID)