[353167] CDOSavePoint and Reattachment issue

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

4 files changed, 146 insertions, 89 deletions

diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/TransactionTest.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/TransactionTest.java
index 1056a47bec..c4daa1b6c9 100644
--- a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/TransactionTest.java
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/TransactionTest.java
@@ -527,6 +527,9 @@ public class TransactionTest extends AbstractCDOTest
     }
   }
 
+  /**
+   * Bug 353167.
+   */
   public void testReattachCommit() throws Exception
   {
     Company company = getModel1Factory().createCompany();
@@ -542,6 +545,9 @@ public class TransactionTest extends AbstractCDOTest
     transaction.commit();
   }
 
+  /**
+   * Bug 353167.
+   */
   public void testReattachModifyCommit() throws Exception
   {
     {
@@ -565,4 +571,57 @@ public class TransactionTest extends AbstractCDOTest
     Company company2 = (Company)resource2.getContents().get(0);
     assertEquals("ESC", company2.getName());
   }
+
+  /**
+   * Bug 353167.
+   */
+  public void testReattachCommitWithSavepoints() throws Exception
+  {
+    Company company = getModel1Factory().createCompany();
+
+    CDOSession session = openSession();
+    CDOTransaction transaction = session.openTransaction();
+    CDOResource resource = transaction.getOrCreateResource(getResourcePath("/test1"));
+    resource.getContents().add(company);
+    transaction.commit();
+
+    resource.getContents().remove(company);
+    transaction.setSavepoint();
+
+    resource.getContents().add(company);
+    transaction.setSavepoint();
+    transaction.commit();
+  }
+
+  /**
+   * Bug 353167.
+   */
+  public void testReattachModifyCommitWithSavepoints() throws Exception
+  {
+    {
+      Company company = getModel1Factory().createCompany();
+
+      CDOSession session = openSession();
+      CDOTransaction transaction = session.openTransaction();
+      CDOResource resource = transaction.getOrCreateResource(getResourcePath("/test1"));
+      resource.getContents().add(company);
+      transaction.commit();
+
+      resource.getContents().remove(company);
+      // transaction.setSavepoint();
+
+      resource.getContents().add(company);
+      transaction.setSavepoint();
+
+      company.setName("ESC");
+
+      transaction.commit();
+    }
+
+    CDOSession session2 = openSession();
+    CDOTransaction transaction2 = session2.openTransaction();
+    CDOResource resource2 = transaction2.getOrCreateResource(getResourcePath("/test1"));
+    Company company2 = (Company)resource2.getContents().get(0);
+    assertEquals("ESC", company2.getName());
+  }
 }
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 db69033adc..ab878152ee 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
@@ -35,7 +35,6 @@ import org.eclipse.emf.spi.cdo.InternalCDOTransaction;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -55,9 +54,8 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
 
   private Map<CDOID, CDOObject> newObjects = new HashMap<CDOID, CDOObject>();
 
-  private Map<CDOID, CDOObject> dirtyObjects = new HashMap<CDOID, CDOObject>();
-
-  private ConcurrentMap<CDOID, CDORevisionDelta> revisionDeltas = new ConcurrentHashMap<CDOID, CDORevisionDelta>();
+  // Bug 283985 (Re-attachment)
+  private Map<CDOID, CDOObject> reattachedObjects = new HashMap<CDOID, CDOObject>();
 
   private Map<CDOID, CDOObject> detachedObjects = new HashMap<CDOID, CDOObject>()
   {
@@ -68,24 +66,19 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
     {
       synchronized (transaction)
       {
-        sharedDetachedObjects.add(key);
-        dirtyObjects.remove(key);
         baseNewObjects.remove(key);
         newObjects.remove(key);
+        reattachedObjects.remove(key);
+        dirtyObjects.remove(key);
         revisionDeltas.remove(key);
         return super.put(key, object);
       }
     }
   };
 
-  // Bug 283985 (Re-attachment)
-  private Map<CDOID, CDOObject> reattachedObjects = new HashMap<CDOID, CDOObject>();
+  private Map<CDOID, CDOObject> dirtyObjects = new HashMap<CDOID, CDOObject>();
 
-  /**
-   * Contains all persistent CDOIDs that were removed. The same instance is shared between all save points that belong
-   * to the same transaction.
-   */
-  private Set<CDOID> sharedDetachedObjects;
+  private ConcurrentMap<CDOID, CDORevisionDelta> revisionDeltas = new ConcurrentHashMap<CDOID, CDORevisionDelta>();
 
   private boolean wasDirty;
 
@@ -93,16 +86,7 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
   {
     super(transaction, lastSavepoint);
     this.transaction = transaction;
-
     wasDirty = transaction.isDirty();
-    if (lastSavepoint == null)
-    {
-      sharedDetachedObjects = new HashSet<CDOID>();
-    }
-    else
-    {
-      sharedDetachedObjects = lastSavepoint.getSharedDetachedObjects();
-    }
   }
 
   @Override
@@ -177,9 +161,16 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
     return dirtyObjects;
   }
 
+  @Deprecated
   public Set<CDOID> getSharedDetachedObjects()
   {
-    return sharedDetachedObjects;
+    throw new UnsupportedOperationException();
+  }
+
+  @Deprecated
+  public void recalculateSharedDetachedObjects()
+  {
+    throw new UnsupportedOperationException();
   }
 
   public ConcurrentMap<CDOID, CDORevisionDelta> getRevisionDeltas()
@@ -266,27 +257,14 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
         return Collections.unmodifiableMap(getNewObjects());
       }
 
-      if (getSharedDetachedObjects().size() == 0)
-      {
-        MultiMap.ListBased<CDOID, CDOObject> newObjects = new MultiMap.ListBased<CDOID, CDOObject>();
-        for (InternalCDOSavepoint savepoint = this; savepoint != null; savepoint = savepoint.getPreviousSavepoint())
-        {
-          newObjects.getDelegates().add(savepoint.getNewObjects());
-        }
-
-        return newObjects;
-      }
-
       Map<CDOID, CDOObject> newObjects = new HashMap<CDOID, CDOObject>();
-      for (InternalCDOSavepoint savepoint = this; savepoint != null; savepoint = savepoint.getPreviousSavepoint())
+      for (InternalCDOSavepoint savepoint = getFirstSavePoint(); savepoint != null; savepoint = savepoint
+          .getNextSavepoint())
       {
-        for (Entry<CDOID, CDOObject> entry : savepoint.getNewObjects().entrySet())
+        newObjects.putAll(savepoint.getNewObjects());
+        for (CDOID removedID : savepoint.getDetachedObjects().keySet())
         {
-          CDOID id = entry.getKey();
-          if (!getSharedDetachedObjects().contains(id))
-          {
-            newObjects.put(id, entry.getValue());
-          }
+          newObjects.remove(removedID);
         }
       }
 
@@ -329,35 +307,37 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
       }
 
       // We need to combined the result for all delta in different Savepoint
-      Map<CDOID, CDORevisionDelta> revisionDeltas = new HashMap<CDOID, CDORevisionDelta>();
+      Map<CDOID, CDORevisionDelta> allRevisionDeltas = new HashMap<CDOID, CDORevisionDelta>();
       for (InternalCDOSavepoint savepoint = getFirstSavePoint(); savepoint != null; savepoint = savepoint
           .getNextSavepoint())
       {
-        for (Entry<CDOID, CDORevisionDelta> entry : savepoint.getRevisionDeltas().entrySet())
+        for (CDORevisionDelta revisionDelta : savepoint.getRevisionDeltas().values())
         {
-          // Skipping new or detached objects
-          CDOID id = entry.getKey();
-          if (isNewObject(id) || getSharedDetachedObjects().contains(id))
-          {
-            continue;
-          }
-
-          CDORevisionDeltaImpl revisionDelta = (CDORevisionDeltaImpl)revisionDeltas.get(id);
-          if (revisionDelta == null)
-          {
-            revisionDeltas.put(id, entry.getValue().copy());
-          }
-          else
+          CDOID id = revisionDelta.getID();
+          if (!isNewObject(id))
           {
-            for (CDOFeatureDelta delta : entry.getValue().getFeatureDeltas())
+            CDORevisionDeltaImpl oldRevisionDelta = (CDORevisionDeltaImpl)allRevisionDeltas.get(id);
+            if (oldRevisionDelta == null)
+            {
+              allRevisionDeltas.put(id, revisionDelta.copy());
+            }
+            else
             {
-              revisionDelta.addFeatureDelta(((InternalCDOFeatureDelta)delta).copy());
+              for (CDOFeatureDelta delta : revisionDelta.getFeatureDeltas())
+              {
+                oldRevisionDelta.addFeatureDelta(((InternalCDOFeatureDelta)delta).copy());
+              }
             }
           }
         }
+
+        for (CDOID detachedID : savepoint.getDetachedObjects().keySet())
+        {
+          allRevisionDeltas.remove(detachedID);
+        }
       }
 
-      return Collections.unmodifiableMap(revisionDeltas);
+      return Collections.unmodifiableMap(allRevisionDeltas);
     }
   }
 
@@ -371,42 +351,26 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
       }
 
       Map<CDOID, CDOObject> detachedObjects = new HashMap<CDOID, CDOObject>();
-      Set<CDOID> reattachedObjectIDs = new HashSet<CDOID>(); // Bug 283985 (Re-attachment)
-      for (InternalCDOSavepoint savepoint = this; savepoint != null; savepoint = savepoint.getPreviousSavepoint())
+      for (InternalCDOSavepoint savepoint = getFirstSavePoint(); savepoint != null; savepoint = savepoint
+          .getNextSavepoint())
       {
-        reattachedObjectIDs.addAll(savepoint.getReattachedObjects().keySet());
-
         for (Entry<CDOID, CDOObject> entry : savepoint.getDetachedObjects().entrySet())
         {
-          CDOID id = entry.getKey();
-          if (!isNewObject(id))
+          CDOID detachedID = entry.getKey();
+          if (!isNewObject(detachedID))
           {
-            // Bug 283985 (Re-attachment):
-            // Object is only included if it was not reattached in a later savepoint
-            if (!reattachedObjectIDs.contains(id))
-            {
-              detachedObjects.put(id, entry.getValue());
-            }
+            CDOObject detachedObject = entry.getValue();
+            detachedObjects.put(detachedID, detachedObject);
           }
         }
-      }
-
-      return detachedObjects;
-    }
-  }
 
-  public void recalculateSharedDetachedObjects()
-  {
-    synchronized (transaction)
-    {
-      sharedDetachedObjects.clear();
-      for (InternalCDOSavepoint savepoint = this; savepoint != null; savepoint = savepoint.getPreviousSavepoint())
-      {
-        for (CDOID id : savepoint.getDetachedObjects().keySet())
+        for (CDOID reattachedID : savepoint.getReattachedObjects().keySet())
         {
-          sharedDetachedObjects.add(id);
+          detachedObjects.remove(reattachedID);
         }
       }
+
+      return detachedObjects;
     }
   }
 
@@ -431,6 +395,41 @@ public class CDOSavepointImpl extends CDOUserSavepointImpl implements InternalCD
     return false;
   }
 
+  // TODO Not sure if this new implementation is needed. The existing one passes all tests.
+  // public boolean isNewObject(CDOID id)
+  // {
+  // if (id.isTemporary())
+  // {
+  // return true;
+  // }
+  //
+  // boolean isNew = false;
+  // boolean wasNew = false;
+  // synchronized (transaction)
+  // {
+  // for (InternalCDOSavepoint savepoint = this; savepoint != null; savepoint = savepoint.getPreviousSavepoint())
+  // {
+  // if (savepoint.getNewObjects().containsKey(id))
+  // {
+  // isNew = true;
+  // wasNew = true;
+  // }
+  //
+  // if (isNew && savepoint.getDetachedObjects().containsKey(id))
+  // {
+  // isNew = false;
+  // }
+  //
+  // if (!isNew && wasNew && savepoint.getReattachedObjects().containsKey(id))
+  // {
+  // isNew = true;
+  // }
+  // }
+  // }
+  //
+  // return isNew;
+  // }
+
   public void rollback()
   {
     synchronized (transaction)
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 5893f2daca..5f1608b8c5 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
@@ -1053,7 +1053,7 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa
 
   private boolean isObjectDetached(CDOID id)
   {
-    return lastSavepoint.getSharedDetachedObjects().contains(id);
+    return lastSavepoint.getAllDetachedObjects().containsKey(id);
   }
 
   /**
@@ -1225,8 +1225,6 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa
 
   private void loadSavepoint(CDOSavepoint savepoint, Set<CDOID> idsOfNewObjectWithDeltas)
   {
-    lastSavepoint.recalculateSharedDetachedObjects();
-
     Map<CDOID, CDOObject> dirtyObjects = getDirtyObjects();
     Map<CDOID, CDOObject> newObjMaps = getNewObjects();
     Map<CDOID, CDORevision> newBaseRevision = getBaseNewObjects();
@@ -1732,7 +1730,6 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa
       lastSavepoint = firstSavepoint;
       firstSavepoint.clear();
       firstSavepoint.setNextSavepoint(null);
-      firstSavepoint.getSharedDetachedObjects().clear();
 
       cleanRevisions.clear();
       dirty = false;
diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/spi/cdo/InternalCDOSavepoint.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/spi/cdo/InternalCDOSavepoint.java
index 7f42eb937d..eb708fcb8e 100644
--- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/spi/cdo/InternalCDOSavepoint.java
+++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/spi/cdo/InternalCDOSavepoint.java
@@ -33,8 +33,10 @@ public interface InternalCDOSavepoint extends CDOSavepoint, InternalCDOUserSavep
 
   public void clear();
 
+  @Deprecated
   public Set<CDOID> getSharedDetachedObjects();
 
+  @Deprecated
   public void recalculateSharedDetachedObjects();
 
   /**