[293283] Failed writes on CDOObjects leave bad featureDeltas in transaction

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

3 files changed, 162 insertions, 12 deletions

diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTestsAllConfigs.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTestsAllConfigs.java
index 210ec84887..778101360f 100644
--- a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTestsAllConfigs.java
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTestsAllConfigs.java
@@ -57,6 +57,7 @@ import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_283985_CDOTest2;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_283985_SavePointTest;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_285008_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_289932_Test;
+import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_293283_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_294850_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_294859_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_299190_Test;
@@ -166,6 +167,7 @@ public abstract class AllTestsAllConfigs extends ConfigTestSuite
     testClasses.add(Bugzilla_283985_CDOTest2.class);
     testClasses.add(Bugzilla_283985_SavePointTest.class);
     testClasses.add(Bugzilla_285008_Test.class);
+    testClasses.add(Bugzilla_293283_Test.class);
     testClasses.add(Bugzilla_294850_Test.class);
     testClasses.add(Bugzilla_294859_Test.class);
     testClasses.add(Bugzilla_299190_Test.class);
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_293283_Test.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_293283_Test.java
new file mode 100644
index 0000000000..8e1c005753
--- /dev/null
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_293283_Test.java
@@ -0,0 +1,140 @@
+/**
+ * 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.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.Order;
+import org.eclipse.emf.cdo.tests.model1.OrderDetail;
+import org.eclipse.emf.cdo.transaction.CDOTransaction;
+
+/**
+ * @author Caspar De Groot
+ */
+public class Bugzilla_293283_Test extends AbstractCDOTest
+{
+  private CDOSession session;
+
+  private CDOTransaction tx;
+
+  private Order order1;
+
+  @Override
+  public void setUp() throws Exception
+  {
+    super.setUp();
+
+    session = openSession();
+    tx = session.openTransaction();
+    CDOResource r1 = tx.getOrCreateResource("/r1"); //$NON-NLS-1$
+    r1.getContents().clear();
+
+    Model1Factory factory = Model1Factory.eINSTANCE;
+    order1 = factory.createOrder();
+    OrderDetail detail1 = factory.createOrderDetail();
+    OrderDetail detail2 = factory.createOrderDetail();
+    order1.getOrderDetails().add(detail1);
+    order1.getOrderDetails().add(detail2);
+    r1.getContents().add(order1);
+    tx.commit();
+  }
+
+  @Override
+  public void tearDown() throws Exception
+  {
+    tx.close();
+    session.close();
+
+    super.tearDown();
+  }
+
+  public void test1()
+  {
+    test(Action.ADD);
+  }
+
+  public void test2()
+  {
+    test(Action.SET);
+  }
+
+  public void test3()
+  {
+    test(Action.MOVE1);
+  }
+
+  public void test4()
+  {
+    test(Action.MOVE2);
+  }
+
+  public void test5()
+  {
+    test(Action.REMOVE);
+  }
+
+  private void test(Action action)
+  {
+    try
+    {
+      switch (action)
+      {
+      case ADD:
+      {
+        OrderDetail newDetail = Model1Factory.eINSTANCE.createOrderDetail();
+        order1.getOrderDetails().add(3, newDetail);
+        break;
+      }
+
+      case MOVE1:
+        order1.getOrderDetails().move(0, 3);
+        break;
+
+      case MOVE2:
+        order1.getOrderDetails().move(3, 0);
+        break;
+
+      case REMOVE:
+        order1.getOrderDetails().remove(3);
+        break;
+
+      case SET:
+      {
+        OrderDetail newDetail = Model1Factory.eINSTANCE.createOrderDetail();
+        order1.getOrderDetails().set(3, newDetail);
+        break;
+      }
+      }
+
+      fail("Should have thrown " + IndexOutOfBoundsException.class.getSimpleName()); //$NON-NLS-1$
+    }
+    catch (IndexOutOfBoundsException ex)
+    {
+      // Good
+    }
+
+    try
+    {
+      tx.commit();
+    }
+    catch (Exception e)
+    {
+      fail("Should have committed cleanly, but failed with " + e.getClass().getName() + "\n" + e.getMessage());
+    }
+  }
+
+  private enum Action
+  {
+    ADD, MOVE1, MOVE2, REMOVE, SET
+  }
+}
diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/CDOStore.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/CDOStore.java
index db4d16d3d4..0202bca452 100644
--- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/CDOStore.java
+++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/CDOStore.java
@@ -38,6 +38,7 @@ import org.eclipse.emf.internal.cdo.util.FSMUtil;
 import org.eclipse.net4j.util.ImplementationError;
 import org.eclipse.net4j.util.om.trace.ContextTracer;
 
+import org.eclipse.emf.common.util.EList;
 import org.eclipse.emf.ecore.EClass;
 import org.eclipse.emf.ecore.EObject;
 import org.eclipse.emf.ecore.EReference;
@@ -71,13 +72,6 @@ public final class CDOStore implements EStore
 
   private InternalCDOView view;
 
-  // // Used for optimization. Multiple call to CDStore will be sent like size and than add.
-  // private EStructuralFeature lastLookupEFeature;
-  //
-  // private EStructuralFeature lastLookupEStructuralFeature;
-  //
-  // private Object lock = new Object();
-
   /**
    * @since 2.0
    */
@@ -480,6 +474,8 @@ public final class CDOStore implements EStore
 
   public Object remove(InternalEObject eObject, EStructuralFeature feature, int index)
   {
+    checkIndexOutOfBounds(eObject, feature, index); // Bugzilla 293283
+
     InternalCDOObject cdoObject = getCDOObject(eObject);
     if (TRACER.isEnabled())
     {
@@ -488,11 +484,9 @@ public final class CDOStore implements EStore
 
     CDOFeatureDelta delta = new CDORemoveFeatureDeltaImpl(feature, index);
     InternalCDORevision revision = getRevisionForWriting(cdoObject, delta);
-    Object result = revision.remove(feature, index);
 
-    result = convertToEMF(eObject, revision, feature, index, result);
-
-    return result;
+    Object oldValue = revision.remove(feature, index);
+    return convertToEMF(eObject, revision, feature, index, oldValue);
   }
 
   public void clear(InternalEObject eObject, EStructuralFeature feature)
@@ -598,6 +592,20 @@ public final class CDOStore implements EStore
     }
   }
 
+  private void checkIndexOutOfBounds(InternalEObject eObject, EStructuralFeature feature, int index)
+  {
+    // Bugzilla 293283
+    if (feature.isMany())
+    {
+      Object o = eObject.eGet(feature);
+      int size = ((EList<?>)o).size();
+      if (index >= size)
+      {
+        throw new IndexOutOfBoundsException("Index: " + index + ", Size: " + size);
+      }
+    }
+  }
+
   private static InternalCDORevision getRevisionForWriting(InternalCDOObject cdoObject, CDOFeatureDelta delta)
   {
     CDOStateMachine.INSTANCE.write(cdoObject, delta);
@@ -609,7 +617,7 @@ public final class CDOStore implements EStore
     InternalCDORevision revision = cdoObject.cdoRevision();
     if (revision == null)
     {
-      throw new IllegalStateException("revision == null"); //$NON-NLS-1$
+      throw new IllegalStateException("revision == null");
     }
 
     return revision;