[318876] Mechanism for avoiding dangling refs can introduce spurious conflicts

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

4 files changed, 126 insertions, 27 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 e36c717ba8..88c4e650fa 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
@@ -80,6 +80,7 @@ import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_316434_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_316444_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_318518_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.config.impl.ConfigTest;
 import org.eclipse.emf.cdo.tests.config.impl.ConfigTestSuite;
 
@@ -224,6 +225,7 @@ public abstract class AllConfigs extends ConfigTestSuite
     testClasses.add(Bugzilla_316444_Test.class);
     testClasses.add(Bugzilla_318518_Test.class);
     testClasses.add(Bugzilla_318844_Test.class);
+    testClasses.add(Bugzilla_318876_Test.class);
 
     // TODO testClasses.add(NonCDOResourceTest.class);
     // TODO testClasses.add(GeneratedEcoreTest.class);
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_298561_Test.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_298561_Test.java
index 169d7baeda..4da25294e0 100644
--- a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_298561_Test.java
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_298561_Test.java
@@ -67,7 +67,7 @@ public class Bugzilla_298561_Test extends AbstractCDOTest
       boolean isSet = referencer.eIsSet(ref);
       if (isSet)
       {
-        referencer.getElement();
+        assertNull(referencer.getElement());
       }
     }
     catch (ObjectNotFoundException e)
@@ -88,17 +88,14 @@ public class Bugzilla_298561_Test extends AbstractCDOTest
     CDOTransaction tx = session.openTransaction();
     CDOResource r1 = tx.createResource(RESOURCENAME);
 
-    // Create referencee and store it
+    // Create referencee and referencer (but no reference yet), and store them
     ContainedElementNoOpposite referencee = getModel4Factory().createContainedElementNoOpposite();
     r1.getContents().add(referencee);
-    tx.commit();
-
-    // Create referencer, store it, then make it DIRTY
     RefSingleNonContainedNPL referencer = getModel4Factory().createRefSingleNonContainedNPL();
     r1.getContents().add(referencer);
-    referencer.setElement(referencee);
     tx.commit();
-    referencer.setElement(null);
+
+    // Create the reference, making the referencer dirty
     referencer.setElement(referencee);
     assertEquals(CDOState.DIRTY, CDOUtil.getCDOObject(referencer).cdoState());
 
@@ -113,7 +110,7 @@ public class Bugzilla_298561_Test extends AbstractCDOTest
       boolean isSet = referencer.eIsSet(getModel4Package().getRefSingleNonContainedNPL_Element());
       if (isSet)
       {
-        referencer.getElement();
+        assertNull(referencer.getElement());
       }
     }
     catch (ObjectNotFoundException e)
@@ -157,7 +154,7 @@ public class Bugzilla_298561_Test extends AbstractCDOTest
       boolean isSet = referencer.eIsSet(getModel4Package().getRefMultiNonContainedNPL_Elements());
       if (isSet && referencer.getElements().size() > 0)
       {
-        referencer.getElements().get(0);
+        assertNull(referencer.getElements().get(0));
       }
     }
     catch (ObjectNotFoundException e)
@@ -178,17 +175,14 @@ public class Bugzilla_298561_Test extends AbstractCDOTest
     CDOTransaction tx = session.openTransaction();
     CDOResource r1 = tx.createResource(RESOURCENAME);
 
-    // Create referencee and store it
+    // Create referencee and referencer (but no reference yet), and store them
     ContainedElementNoOpposite referencee = getModel4Factory().createContainedElementNoOpposite();
     r1.getContents().add(referencee);
-    tx.commit();
-
-    // Create referencer, store it, then make it DIRTY
     RefMultiNonContainedNPL referencer = getModel4Factory().createRefMultiNonContainedNPL();
     r1.getContents().add(referencer);
-    referencer.getElements().add(referencee);
     tx.commit();
-    referencer.getElements().remove(referencee);
+
+    // Create the reference, making the referencer dirty
     referencer.getElements().add(referencee);
     assertEquals(CDOState.DIRTY, CDOUtil.getCDOObject(referencer).cdoState());
 
@@ -204,7 +198,7 @@ public class Bugzilla_298561_Test extends AbstractCDOTest
       boolean isSet = referencer.eIsSet(getModel4Package().getRefMultiNonContainedNPL_Elements());
       if (isSet && referencer.getElements().size() > 0)
       {
-        referencer.getElements().get(0);
+        assertNull(referencer.getElements().get(0));
       }
     }
     catch (ObjectNotFoundException e)
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_318876_Test.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_318876_Test.java
new file mode 100644
index 0000000000..a8324dc1ec
--- /dev/null
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_318876_Test.java
@@ -0,0 +1,84 @@
+/**
+ * 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:
+ *    Caspar De Groot - initial API and implementation
+ */
+package org.eclipse.emf.cdo.tests.bugzilla;
+
+import org.eclipse.emf.cdo.CDOState;
+import org.eclipse.emf.cdo.eresource.CDOResource;
+import org.eclipse.emf.cdo.session.CDOSession;
+import org.eclipse.emf.cdo.tests.AbstractCDOTest;
+import org.eclipse.emf.cdo.tests.model1.Model1Factory;
+import org.eclipse.emf.cdo.tests.model1.PurchaseOrder;
+import org.eclipse.emf.cdo.tests.model1.Supplier;
+import org.eclipse.emf.cdo.transaction.CDOTransaction;
+import org.eclipse.emf.cdo.util.CDOUtil;
+import org.eclipse.emf.cdo.util.CommitException;
+
+import org.eclipse.emf.ecore.util.EcoreUtil;
+
+import java.util.Date;
+
+/**
+ * Bugzilla 318876 - Mechanism for avoiding dangling refs can introduce spurious conflicts</p>
+ * http://bugs.eclipse.org/318876</p>
+ * 
+ * @author Caspar De Groot
+ */
+public class Bugzilla_318876_Test extends AbstractCDOTest
+{
+  public void test() throws CommitException
+  {
+    final CDOSession session = openSession();
+    session.options().setPassiveUpdateEnabled(false);
+    CDOTransaction tx = session.openTransaction();
+    CDOResource r1 = tx.createResource("/r1"); //$NON-NLS-1$
+
+    PurchaseOrder po1 = Model1Factory.eINSTANCE.createPurchaseOrder();
+    po1.setDate(new Date());
+    r1.getContents().add(po1);
+
+    Supplier supplier = Model1Factory.eINSTANCE.createSupplier();
+    supplier.getPurchaseOrders().add(po1);
+    r1.getContents().add(supplier);
+
+    tx.commit();
+
+    // Remove po2 in another session
+    doSecondSession();
+
+    // Make the supplier dirty so that it will be scanned for dangling refs
+    // during processing of the refresh results
+    supplier.setName("Supplier");
+
+    session.refresh();
+
+    CDOState state = CDOUtil.getCDOObject(po1).cdoState();
+    System.out.println("--> purchaseOrder state (should be INVALID) = " + state);
+    assertEquals(CDOState.INVALID, state);
+
+    tx.close();
+    session.close();
+  }
+
+  private void doSecondSession() throws CommitException
+  {
+    CDOSession session = openSession();
+    CDOTransaction tx = session.openTransaction();
+    CDOResource r1 = tx.getResource("/r1"); //$NON-NLS-1$
+
+    // Detach the po
+    PurchaseOrder po = (PurchaseOrder)r1.getContents().get(0);
+    EcoreUtil.delete(po);
+
+    tx.commit();
+    tx.close();
+    session.close();
+  }
+}
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 38758e0604..0d96734d39 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
@@ -36,6 +36,7 @@ import org.eclipse.emf.cdo.common.model.CDOPackageUnit;
 import org.eclipse.emf.cdo.common.model.EMFUtil;
 import org.eclipse.emf.cdo.common.protocol.CDODataInput;
 import org.eclipse.emf.cdo.common.protocol.CDODataOutput;
+import org.eclipse.emf.cdo.common.revision.CDOList;
 import org.eclipse.emf.cdo.common.revision.CDOListFactory;
 import org.eclipse.emf.cdo.common.revision.CDORevision;
 import org.eclipse.emf.cdo.common.revision.CDORevisionFactory;
@@ -77,6 +78,7 @@ import org.eclipse.emf.cdo.transaction.CDOTransactionHandler;
 import org.eclipse.emf.cdo.transaction.CDOTransactionStartedEvent;
 import org.eclipse.emf.cdo.transaction.CDOUserSavepoint;
 import org.eclipse.emf.cdo.util.CDOURIUtil;
+import org.eclipse.emf.cdo.util.CDOUtil;
 import org.eclipse.emf.cdo.util.CommitException;
 import org.eclipse.emf.cdo.util.LegacyModeNotEnabledException;
 import org.eclipse.emf.cdo.util.ObjectNotFoundException;
@@ -108,8 +110,8 @@ import org.eclipse.emf.ecore.EPackage;
 import org.eclipse.emf.ecore.EReference;
 import org.eclipse.emf.ecore.EStructuralFeature.Setting;
 import org.eclipse.emf.ecore.InternalEObject;
+import org.eclipse.emf.ecore.InternalEObject.EStore;
 import org.eclipse.emf.ecore.util.EContentsEList;
-import org.eclipse.emf.ecore.util.EContentsEList.FeatureIterator;
 import org.eclipse.emf.ecore.util.EcoreUtil;
 import org.eclipse.emf.spi.cdo.CDOSessionProtocol;
 import org.eclipse.emf.spi.cdo.CDOSessionProtocol.CommitTransactionResult;
@@ -1830,23 +1832,40 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa
         detachedObjects);
   }
 
-  private void removeCrossReferences(Collection<CDOObject> objects, Set<CDOID> referencedOIDs)
+  private void removeCrossReferences(Collection<CDOObject> referencers, Set<CDOID> referencedOIDs)
   {
-    for (CDOObject object : objects)
+    for (CDOObject referencer : referencers)
     {
-      EContentsEList.FeatureIterator<EObject> it = (FeatureIterator<EObject>)object.eCrossReferences().iterator();
+      EContentsEList.FeatureIterator<EObject> it = (EContentsEList.FeatureIterator<EObject>)referencer
+          .eCrossReferences().iterator();
       while (it.hasNext())
       {
-        EObject crossReferencedObject = it.next();
-        if (crossReferencedObject instanceof CDOObject)
+        EObject referencedObject = it.next();
+        CDOID referencedOID = CDOUtil.getCDOObject(referencedObject).cdoID();
+
+        if (referencedOIDs.contains(referencedOID))
         {
-          CDOID id = ((CDOObject)crossReferencedObject).cdoID();
-          if (referencedOIDs.contains(id))
+          EReference reference = (EReference)it.feature();
+
+          // In the case of DIRTY, we must investigate further: Is the referencer dirty
+          // because a reference to the referencedObject was added? Only in this case
+          // should we remove it. If this is not the case (i.e. it is dirty in a different
+          // way, we must ignore it.)
+          if (referencer.cdoState() == CDOState.DIRTY)
           {
-            EReference eReference = (EReference)it.feature();
-            Setting setting = ((InternalEObject)object).eSetting(eReference);
-            EcoreUtil.remove(setting, crossReferencedObject);
+            InternalCDORevision cleanRevision = getSession().getRevisionManager().getRevisionByVersion(
+                referencer.cdoID(), referencer.cdoRevision(), CDORevision.UNCHUNKED, true);
+            Object value = cleanRevision.get(reference, EStore.NO_INDEX);
+            if (value instanceof CDOObject && value == referencedObject || //
+                value instanceof CDOID && value.equals(referencedOID) || //
+                value instanceof CDOList && ((CDOList)value).contains(referencedOID))
+            {
+              continue;
+            }
           }
+
+          Setting setting = ((InternalEObject)referencer).eSetting(reference);
+          EcoreUtil.remove(setting, referencedObject);
         }
       }
     }