[Bug 338884] CommitIntegrityCheck does not check inclusion of normal refTargets in NEW state

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

6 files changed, 179 insertions, 36 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 2df911f..bfb35dd 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
@@ -209,5 +209,6 @@ public abstract class AllConfigs extends ConfigTestSuite
     testClasses.add(Bugzilla_336314_Test.class);
     testClasses.add(Bugzilla_336382_Test.class);
     testClasses.add(Bugzilla_336590_Test.class);
+    testClasses.add(Bugzilla_338884_Test.class);
   }
 }
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_338884_Test.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_338884_Test.java
new file mode 100644
index 0000000..b3f268c
--- /dev/null
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_338884_Test.java
@@ -0,0 +1,124 @@
+/**
+ * Copyright (c) 2004 - 2011 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.eresource.CDOResource;
+import org.eclipse.emf.cdo.session.CDOSession;
+import org.eclipse.emf.cdo.tests.AbstractCDOTest;
+import org.eclipse.emf.cdo.tests.model4.ContainedElementNoOpposite;
+import org.eclipse.emf.cdo.tests.model4.RefMultiNonContainedNPL;
+import org.eclipse.emf.cdo.tests.model4.RefSingleNonContainedNPL;
+import org.eclipse.emf.cdo.tests.model4.model4Factory;
+import org.eclipse.emf.cdo.transaction.CDOTransaction;
+import org.eclipse.emf.cdo.util.CommitException;
+import org.eclipse.emf.cdo.util.CommitIntegrityException;
+
+import java.util.Collections;
+
+/**
+ * @author Caspar De Groot
+ */
+public class Bugzilla_338884_Test extends AbstractCDOTest
+{
+  public void test_single() throws CommitException
+  {
+    CDOSession session = openSession();
+    CDOTransaction tx = session.openTransaction();
+    CDOResource resource = tx.createResource(getResourcePath("test"));
+
+    // 1. Create object A;
+    model4Factory factory = getModel4Factory();
+    RefSingleNonContainedNPL referencer = factory.createRefSingleNonContainedNPL();
+    resource.getContents().add(referencer);
+
+    // 2. Commit;
+    tx.commit();
+
+    // 3. Create object B;
+    ContainedElementNoOpposite element = factory.createContainedElementNoOpposite();
+    resource.getContents().add(element);
+
+    // 4. Create reference from object A to object B;
+    referencer.setElement(element);
+
+    // 5. Add object A to committables, but not add B;
+    tx.setCommittables(Collections.singleton(referencer));
+
+    // 6. Commit.
+    try
+    {
+      tx.commit();
+      fail("Should have thrown " + CommitException.class.getName());
+    }
+    catch (CommitException e)
+    {
+      Throwable t = e.getCause().getCause();
+      if (t instanceof CommitIntegrityException)
+      {
+        // Good
+      }
+      else
+      {
+        fail("Unexpected exception type: " + t.getClass().getName());
+      }
+    }
+
+    tx.close();
+    session.close();
+  }
+
+  public void test_multi() throws CommitException
+  {
+    CDOSession session = openSession();
+    CDOTransaction tx = session.openTransaction();
+    CDOResource resource = tx.createResource(getResourcePath("test"));
+
+    // 1. Create object A;
+    model4Factory factory = getModel4Factory();
+    RefMultiNonContainedNPL referencer = factory.createRefMultiNonContainedNPL();
+    resource.getContents().add(referencer);
+
+    // 2. Commit;
+    tx.commit();
+
+    // 3. Create object B;
+    ContainedElementNoOpposite element = factory.createContainedElementNoOpposite();
+    resource.getContents().add(element);
+
+    // 4. Create reference from object A to object B;
+    referencer.getElements().add(element);
+
+    // 5. Add object A to committables, but not add B;
+    tx.setCommittables(Collections.singleton(referencer));
+
+    // 6. Commit.
+    try
+    {
+      tx.commit();
+      fail("Should have thrown " + CommitException.class.getName());
+    }
+    catch (CommitException e)
+    {
+      Throwable t = e.getCause().getCause();
+      if (t instanceof CommitIntegrityException)
+      {
+        // Good
+      }
+      else
+      {
+        fail("Unexpected exception type: " + t.getClass().getName());
+      }
+    }
+
+    tx.close();
+    session.close();
+  }
+}
diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/cdo/transaction/CDOPushTransaction.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/cdo/transaction/CDOPushTransaction.java
index 13162b0..4008096 100644
--- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/cdo/transaction/CDOPushTransaction.java
+++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/cdo/transaction/CDOPushTransaction.java
@@ -685,7 +685,7 @@ public class CDOPushTransaction extends Notifier implements CDOTransaction
   /**
    * @since 4.0
    */
-  public void setCommittables(Set<EObject> committables)
+  public void setCommittables(Set<? extends EObject> committables)
   {
     delegate.setCommittables(committables);
   }
@@ -693,7 +693,7 @@ public class CDOPushTransaction extends Notifier implements CDOTransaction
   /**
    * @since 4.0
    */
-  public Set<EObject> getCommittables()
+  public Set<? extends EObject> getCommittables()
   {
     return delegate.getCommittables();
   }
diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/cdo/transaction/CDOTransaction.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/cdo/transaction/CDOTransaction.java
index 9c4f5fb..23bcd6c 100644
--- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/cdo/transaction/CDOTransaction.java
+++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/cdo/transaction/CDOTransaction.java
@@ -168,12 +168,12 @@ public interface CDOTransaction extends CDOView, CDOCommonTransaction, CDOUserTr
   /**
    * @since 4.0
    */
-  public void setCommittables(Set<EObject> committables);
+  public void setCommittables(Set<? extends EObject> committables);
 
   /**
    * @since 4.0
    */
-  public Set<EObject> getCommittables();
+  public Set<? extends EObject> getCommittables();
 
   /**
    * @since 4.0
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 7ad0a99..d9f7759 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
@@ -213,7 +213,7 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa
   /**
    * An optional set to specify which objects in this TX are to be committed by {@link #commit()}
    */
-  private Set<EObject> committables;
+  private Set<? extends EObject> committables;
 
   /**
    * A map to hold a clean (i.e. unmodified) revision for objects that have been modified or detached.
@@ -2164,12 +2164,12 @@ public class CDOTransactionImpl extends CDOViewImpl implements InternalCDOTransa
     commitComment = comment;
   }
 
-  public synchronized void setCommittables(Set<EObject> committables)
+  public synchronized void setCommittables(Set<? extends EObject> committables)
   {
     this.committables = committables;
   }
 
-  public synchronized Set<EObject> getCommittables()
+  public synchronized Set<? extends EObject> getCommittables()
   {
     return committables;
   }
diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/util/CommitIntegrityCheck.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/util/CommitIntegrityCheck.java
index eae943d..1a9a651 100644
--- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/util/CommitIntegrityCheck.java
+++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/util/CommitIntegrityCheck.java
@@ -11,7 +11,9 @@
 package org.eclipse.emf.internal.cdo.util;
 
 import org.eclipse.emf.cdo.CDOObject;
+import org.eclipse.emf.cdo.CDOState;
 import org.eclipse.emf.cdo.common.id.CDOID;
+import org.eclipse.emf.cdo.common.id.CDOIDTemp;
 import org.eclipse.emf.cdo.common.id.CDOIDUtil;
 import org.eclipse.emf.cdo.common.model.EMFUtil;
 import org.eclipse.emf.cdo.common.revision.delta.CDOAddFeatureDelta;
@@ -162,24 +164,17 @@ public class CommitIntegrityCheck
       }
       else if (feat instanceof EReference)
       {
-        EReference ref = (EReference)feat;
-        if (ref.isContainment() || hasPersistentOpposite(ref))
+        if (featureDelta instanceof CDOListFeatureDelta)
         {
-          // Object is dirty with respect to a containment feature or a feature with an eOpposite
-          // We must ensure that any reference targets that were added/removed/set, are also
-          // included in the commit
-          if (featureDelta instanceof CDOListFeatureDelta)
+          for (CDOFeatureDelta innerFeatDelta : ((CDOListFeatureDelta)featureDelta).getListChanges())
           {
-            for (CDOFeatureDelta innerFeatDelta : ((CDOListFeatureDelta)featureDelta).getListChanges())
-            {
-              checkFeatureDelta(innerFeatDelta, dirtyObject);
-            }
-          }
-          else
-          {
-            checkFeatureDelta(featureDelta, dirtyObject);
+            checkFeatureDelta(innerFeatDelta, dirtyObject);
           }
         }
+        else
+        {
+          checkFeatureDelta(featureDelta, dirtyObject);
+        }
       }
     }
   }
@@ -201,16 +196,16 @@ public class CommitIntegrityCheck
 
   private void checkFeatureDelta(CDOFeatureDelta featureDelta, CDOObject dirtyObject) throws CommitIntegrityException
   {
-    if (featureDelta instanceof CDORemoveFeatureDelta)
-    {
-      Object idOrObject = ((CDORemoveFeatureDelta)featureDelta).getValue();
-      CDOID id = (CDOID)transaction.convertObjectToID(idOrObject);
-      checkIncluded(id, "removed child of", dirtyObject);
-    }
-    else if (featureDelta instanceof CDOAddFeatureDelta)
+    EReference ref = (EReference)featureDelta.getFeature();
+    boolean containmentOrWithOpposite = ref.isContainment() || hasPersistentOpposite(ref);
+
+    if (featureDelta instanceof CDOAddFeatureDelta)
     {
       Object idOrObject = ((CDOAddFeatureDelta)featureDelta).getValue();
-      checkIncluded(idOrObject, "added child of", dirtyObject);
+      if (containmentOrWithOpposite || isNew(idOrObject))
+      {
+        checkIncluded(idOrObject, "added child/refTarget of", dirtyObject);
+      }
     }
     else if (featureDelta instanceof CDOSetFeatureDelta)
     {
@@ -220,17 +215,26 @@ public class CommitIntegrityCheck
       if (oldIDOrObject != null)
       {
         // Old child must be included
-        checkIncluded(oldID, "removed/former child of", dirtyObject);
+        checkIncluded(oldID, "removed/former child/refTarget of", dirtyObject);
       }
 
       if (newIDOrObject != null)
       {
         // New child must be included
         newIDOrObject = transaction.convertObjectToID(newIDOrObject);
-        checkIncluded(newIDOrObject, "new child of", dirtyObject);
+        if (containmentOrWithOpposite || isNew(newIDOrObject))
+        {
+          checkIncluded(newIDOrObject, "new child/refTarget of", dirtyObject);
+        }
       }
     }
-    else if (featureDelta instanceof CDOClearFeatureDelta)
+    else if (containmentOrWithOpposite && featureDelta instanceof CDORemoveFeatureDelta)
+    {
+      Object idOrObject = ((CDORemoveFeatureDelta)featureDelta).getValue();
+      CDOID id = (CDOID)transaction.convertObjectToID(idOrObject);
+      checkIncluded(id, "removed child/refTarget of", dirtyObject);
+    }
+    else if (containmentOrWithOpposite && featureDelta instanceof CDOClearFeatureDelta)
     {
       EStructuralFeature feat = ((CDOClearFeatureDelta)featureDelta).getFeature();
       InternalCDORevision cleanRev = transaction.getCleanRevisions().get(dirtyObject);
@@ -239,18 +243,18 @@ public class CommitIntegrityCheck
       {
         Object idOrObject = cleanRev.get(feat, i);
         CDOID id = (CDOID)transaction.convertObjectToID(idOrObject);
-        checkIncluded(id, "removed child of", dirtyObject);
+        checkIncluded(id, "removed child/refTarget of", dirtyObject);
       }
     }
-    else if (featureDelta instanceof CDOUnsetFeatureDelta)
+    else if (containmentOrWithOpposite && featureDelta instanceof CDOUnsetFeatureDelta)
     {
       EStructuralFeature feat = ((CDOUnsetFeatureDelta)featureDelta).getFeature();
       InternalCDORevision cleanRev = transaction.getCleanRevisions().get(dirtyObject);
       Object idOrObject = cleanRev.getValue(feat);
       CDOID id = (CDOID)transaction.convertObjectToID(idOrObject);
-      checkIncluded(id, "removed child of", dirtyObject);
+      checkIncluded(id, "removed child/refTarget of", dirtyObject);
     }
-    else if (featureDelta instanceof CDOMoveFeatureDelta)
+    else if (containmentOrWithOpposite && featureDelta instanceof CDOMoveFeatureDelta)
     {
       // Nothing to do: a move doesn't affect the child being moved
       // so that child does not need to be included
@@ -261,6 +265,20 @@ public class CommitIntegrityCheck
     }
   }
 
+  private boolean isNew(Object idOrObject)
+  {
+    if (idOrObject instanceof CDOIDTemp)
+    {
+      return true;
+    }
+    if (idOrObject instanceof EObject)
+    {
+      CDOObject obj = CDOUtil.getCDOObject((EObject)idOrObject);
+      return obj.cdoState() == CDOState.NEW;
+    }
+    return false;
+  }
+
   private void checkIncluded(CDOID id, String msg, CDOObject o) throws CommitIntegrityException
   {
     if (id.isNull())