Removing elements with @OrderColumn with a tree depth > 1 doesn't work (#1988)

* ADD: failing testcase for orderColumn when tree depth > 1

* FIX: no update with @OrderColumn on treeDepth > 1 and avoid needless updates

* ADD: failing testcase for removing models in oneToMany relation with @OrderColumn

Signed-off-by: Jonas Pöhler (JPo) <jonas.poehler@foconis.de>

* ADD: failing test for faulty element reordering in a entityBean tree with version

Signed-off-by: Jonas Pöhler (JPo) <jonas.poehler@foconis.de>

* FIX: incorrect behaviour when reordering or removing elements from collection with @OrderColumn.

Signed-off-by: Jonas Pöhler (JPo) <jonas.poehler@foconis.de>

src/main/java/io/ebean/bean/EntityBeanIntercept.java

@@ -40,6 +40,8 @@ public final class EntityBeanIntercept implements Serializable {
 
   private String ebeanServerName;
 
+  private boolean deletedFromCollection;
+
   /**
    * The actual entity bean that 'owns' this intercept.
    */
@@ -1169,4 +1171,18 @@ public final class EntityBeanIntercept implements Serializable {
     }
     return ret;
   }
+
+  /**
+   * Returns true if the entity was removed from a BeanCollection. This can be used to track movement from one collection to another.
+   */
+  public boolean isDeletedFromCollection() {
+    return deletedFromCollection;
+  }
+
+  /**
+   * Set if the entity was deleted from a BeanCollection.
+   */
+  public void setDeletedFromCollection(final boolean deletedFromCollection) {
+    this.deletedFromCollection = deletedFromCollection;
+  }
 }

src/main/java/io/ebean/common/ModifyHolder.java

@@ -1,5 +1,7 @@
 package io.ebean.common;
 
+import io.ebean.bean.EntityBean;
+
 import java.io.Serializable;
 import java.util.Collection;
 import java.util.LinkedHashSet;
@@ -54,6 +56,11 @@ class ModifyHolder<E> implements Serializable {
   void modifyAddition(E bean) {
     if (bean != null) {
       touched = true;
+
+      if (bean instanceof EntityBean) {
+        ((EntityBean) bean)._ebean_getIntercept().setDeletedFromCollection(false);
+      }
+
       // If it is to delete then just remove the deletion
       if (!undoDeletion(bean)) {
         // Insert
@@ -70,6 +77,11 @@ class ModifyHolder<E> implements Serializable {
   void modifyRemoval(Object bean) {
     if (bean != null) {
       touched = true;
+
+      if (bean instanceof EntityBean) {
+        ((EntityBean) bean)._ebean_getIntercept().setDeletedFromCollection(true);
+      }
+
       // If it is to be added then just remove the addition
       if (!undoAddition(bean)) {
         modifyDeletions.add((E) bean);

src/main/java/io/ebeaninternal/server/persist/SaveManyBeans.java

@@ -69,7 +69,7 @@ public class SaveManyBeans extends SaveManyBase {
         resetModifyState();
       }
     } else {
-      if (isModifyListenMode()) {
+      if (isModifyListenMode() || many.hasOrderColumn()) {
         // delete any removed beans via private owned. Needs to occur before
         // a 'deleteMissingChildren' statement occurs
         removeAssocManyPrivateOwned();
@@ -348,14 +348,25 @@ public class SaveManyBeans extends SaveManyBase {
 
       BeanCollection<?> c = (BeanCollection<?>) value;
       Set<?> modifyRemovals = c.getModifyRemovals();
-      modifyListenReset(c);
+
+      if (insertedParent) {
+        // after insert set the modify listening mode for private owned etc
+        c.setModifyListening(many.getModifyListenMode());
+      }
+
+      // We must not reset when we still have to update other entities in the collection and set their new orderColumn value
+      if (!many.hasOrderColumn()) {
+        c.modifyReset();
+      }
       if (modifyRemovals != null && !modifyRemovals.isEmpty()) {
         for (Object removedBean : modifyRemovals) {
           if (removedBean instanceof EntityBean) {
             EntityBean eb = (EntityBean) removedBean;
             if (!eb._ebean_getIntercept().isNew()) {
-              // only delete if the bean was loaded meaning that it is known to exist in the DB
-              persister.deleteRequest(persister.createDeleteRemoved(removedBean, transaction, request.getFlags()));
+              if (eb._ebean_intercept().isDeletedFromCollection()) {
+                // only delete if the bean was loaded meaning that it is known to exist in the DB
+                persister.deleteRequest(persister.createDeleteRemoved(removedBean, transaction, request.getFlags()));
+              }
             }
           }
         }

src/test/java/org/tests/model/version/TestVersionHierarchyModification.java

@@ -0,0 +1,118 @@
+package org.tests.model.version;
+
+import io.ebean.BaseTestCase;
+import io.ebean.Ebean;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class TestVersionHierarchyModification extends BaseTestCase {
+
+  @Before
+  public void setup() {
+    final VersionParent parent = new VersionParent();
+    parent.setName("vParent");
+
+    final VersionChild child1 = new VersionChild();
+    child1.setName("vChild1");
+    parent.getChildren().add(child1);
+
+    final VersionToy toy11 = new VersionToy();
+    toy11.setName("vToy1.1");
+    child1.getToys().add(toy11);
+
+    final VersionToy toy12 = new VersionToy();
+    toy12.setName("vToy1.2");
+    child1.getToys().add(toy12);
+
+    final VersionChild child2 = new VersionChild();
+    child2.setName("vChild2");
+    parent.getChildren().add(child2);
+
+    final VersionToy toy21 = new VersionToy();
+    toy21.setName("vToy2.1");
+    child2.getToys().add(toy21);
+
+    final VersionToy toy22 = new VersionToy();
+    toy22.setName("vToy2.2");
+    child2.getToys().add(toy22);
+
+    Ebean.save(parent);
+  }
+
+  @After
+  public void cleanUp() {
+    Ebean.find(VersionParent.class).delete();
+  }
+
+  @Test
+  public void testMoveDown() {
+    VersionParent parent = Ebean.find(VersionParent.class).findOne();
+    assertThat(parent).isNotNull();
+    assertThat(parent.getChildren()).hasSize(2);
+
+    VersionChild firstChild = parent.getChildren().get(0);
+    VersionChild secondChild = parent.getChildren().get(1);
+
+    assertThat(firstChild.getToys()).hasSize(2);
+    assertThat(secondChild.getToys()).hasSize(2);
+    assertThat(firstChild.getToys()).extracting(VersionToy::getName).containsExactly("vToy1.1", "vToy1.2");
+    assertThat(secondChild.getToys()).extracting(VersionToy::getName).containsExactly("vToy2.1", "vToy2.2");
+
+    final VersionToy toyToMove = firstChild.getToys().get(0);
+    firstChild.getToys().remove(toyToMove);
+    toyToMove.setChild(secondChild);
+    secondChild.getToys().add(1, toyToMove);
+
+    Ebean.save(parent);
+
+    parent = Ebean.find(VersionParent.class).findOne();
+    assertThat(parent).isNotNull();
+    assertThat(parent.getChildren()).hasSize(2);
+
+    firstChild = parent.getChildren().get(0);
+    secondChild = parent.getChildren().get(1);
+
+    assertThat(firstChild.getToys()).hasSize(1);
+    assertThat(secondChild.getToys()).hasSize(3);
+    assertThat(firstChild.getToys()).extracting(VersionToy::getName).containsExactly("vToy1.2");
+    assertThat(secondChild.getToys()).extracting(VersionToy::getName).containsExactly("vToy2.1", "vToy1.1", "vToy2.2");
+  }
+
+  @Test
+  public void testMoveUp() {
+    VersionParent parent = Ebean.find(VersionParent.class).findOne();
+    assertThat(parent).isNotNull();
+    assertThat(parent.getChildren()).hasSize(2);
+
+    VersionChild firstChild = parent.getChildren().get(0);
+    VersionChild secondChild = parent.getChildren().get(1);
+
+    assertThat(firstChild.getToys()).hasSize(2);
+    assertThat(secondChild.getToys()).hasSize(2);
+    assertThat(firstChild.getToys()).extracting(VersionToy::getName).containsExactly("vToy1.1", "vToy1.2");
+    assertThat(secondChild.getToys()).extracting(VersionToy::getName).containsExactly("vToy2.1", "vToy2.2");
+
+    final VersionToy toyToMove = secondChild.getToys().get(0);
+    secondChild.getToys().remove(toyToMove);
+    toyToMove.setChild(firstChild);
+    firstChild.getToys().add(1, toyToMove);
+
+    Ebean.save(parent);
+
+    parent = Ebean.find(VersionParent.class).findOne();
+    assertThat(parent).isNotNull();
+    assertThat(parent.getChildren()).hasSize(2);
+
+    firstChild = parent.getChildren().get(0);
+    secondChild = parent.getChildren().get(1);
+
+    assertThat(secondChild.getToys()).hasSize(1);
+    assertThat(firstChild.getToys()).hasSize(3);
+    assertThat(secondChild.getToys()).extracting(VersionToy::getName).containsExactly("vToy2.2");
+    assertThat(firstChild.getToys()).extracting(VersionToy::getName).containsExactly("vToy1.1", "vToy2.1", "vToy1.2");
+  }
+
+}

src/test/java/org/tests/model/version/VersionChild.java

@@ -0,0 +1,70 @@
+package org.tests.model.version;
+
+import javax.persistence.CascadeType;
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import javax.persistence.ManyToOne;
+import javax.persistence.OneToMany;
+import javax.persistence.OrderColumn;
+import javax.persistence.Version;
+import java.util.ArrayList;
+import java.util.List;
+
+@Entity
+public class VersionChild {
+
+  @Id
+  Integer id;
+
+  String name;
+
+  @Version
+  Integer version;
+
+  @OneToMany(mappedBy = "child", cascade = CascadeType.ALL, orphanRemoval = true)
+  @OrderColumn(name = "position")
+  List<VersionToy> toys = new ArrayList<>();
+
+  @ManyToOne
+  VersionParent parent;
+
+  public Integer getId() {
+    return id;
+  }
+
+  public void setId(final Integer id) {
+    this.id = id;
+  }
+
+  public String getName() {
+    return name;
+  }
+
+  public void setName(final String name) {
+    this.name = name;
+  }
+
+  public Integer getVersion() {
+    return version;
+  }
+
+  public void setVersion(final Integer version) {
+    this.version = version;
+  }
+
+  public List<VersionToy> getToys() {
+    return toys;
+  }
+
+  public void setToys(final List<VersionToy> toys) {
+    this.toys = toys;
+  }
+
+  public VersionParent getParent() {
+    return parent;
+  }
+
+  public void setParent(final VersionParent parent) {
+    this.parent = parent;
+  }
+}

src/test/java/org/tests/model/version/VersionParent.java

@@ -0,0 +1,58 @@
+package org.tests.model.version;
+
+import javax.persistence.CascadeType;
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import javax.persistence.OneToMany;
+import javax.persistence.OrderColumn;
+import javax.persistence.Version;
+import java.util.ArrayList;
+import java.util.List;
+
+@Entity
+public class VersionParent {
+
+  @Id
+  Integer id;
+
+  @Version
+  Integer version;
+
+  String name;
+
+  @OneToMany(mappedBy = "parent", cascade = CascadeType.ALL, orphanRemoval = true)
+  @OrderColumn(name = "position")
+  List<VersionChild> children = new ArrayList<>();
+
+  public Integer getId() {
+    return id;
+  }
+
+  public void setId(final Integer id) {
+    this.id = id;
+  }
+
+  public Integer getVersion() {
+    return version;
+  }
+
+  public void setVersion(final Integer version) {
+    this.version = version;
+  }
+
+  public String getName() {
+    return name;
+  }
+
+  public void setName(final String name) {
+    this.name = name;
+  }
+
+  public List<VersionChild> getChildren() {
+    return children;
+  }
+
+  public void setChildren(final List<VersionChild> children) {
+    this.children = children;
+  }
+}

src/test/java/org/tests/model/version/VersionToy.java

@@ -0,0 +1,53 @@
+package org.tests.model.version;
+
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import javax.persistence.ManyToOne;
+import javax.persistence.Version;
+
+@Entity
+public class VersionToy {
+
+  @Id
+  Integer id;
+
+  String name;
+
+  @Version
+  Integer version;
+
+  @ManyToOne
+  VersionChild child;
+
+  public Integer getId() {
+    return id;
+  }
+
+  public void setId(final Integer id) {
+    this.id = id;
+  }
+
+  public String getName() {
+    return name;
+  }
+
+  public void setName(final String name) {
+    this.name = name;
+  }
+
+  public Integer getVersion() {
+    return version;
+  }
+
+  public void setVersion(final Integer version) {
+    this.version = version;
+  }
+
+  public VersionChild getChild() {
+    return child;
+  }
+
+  public void setChild(final VersionChild child) {
+    this.child = child;
+  }
+}

src/test/java/org/tests/order/TestOrderColumn.java

@@ -107,4 +107,41 @@ public class TestOrderColumn extends TransactionalTestCase {
     assertThat(sql.get(2)).contains("bind(tt32");
   }
 
+  @Test
+  public void testRemoveElement() {
+    final OrderMaster master = new OrderMaster();
+
+    for (int i = 0; i < 5; i++) {
+      final OrderReferencedChild child = new OrderReferencedChild("p" + i);
+      child.setChildName("c" + i);
+
+      for (int j = 0; j < 3; j++) {
+        final OrderToy toy = new OrderToy("t" + i + j);
+        child.getToys().add(toy);
+      }
+
+      master.getChildren().add(child);
+    }
+
+    Ebean.save(master);
+
+    final OrderMaster result = Ebean.find(OrderMaster.class).findOne();
+
+    final List<OrderReferencedChild> children = result.getChildren();
+    assertThat(children).hasSize(5);
+
+    final OrderReferencedChild child = children.get(0);
+
+    child.getToys().remove(1);
+
+    LoggedSqlCollector.start();
+    Ebean.save(result);
+    final List<String> sql = LoggedSqlCollector.stop();
+
+    assertThat(sql).hasSize(4);
+    assertSql(sql.get(0)).contains("delete from order_toy where id=?");
+    assertSql(sql.get(2)).contains("update order_toy set sort_order=? where id=?");
+    assertSql(sql.get(3)).contains("bind(2,");
+  }
+
 }