#2961 - Followup for #2952 - @OneToMany + orphanRemoval + @SoftDelete + non-BeanCollection collection results in hard deletes

As noted in comments in #2952

In the internals of SaveManyBeans we have:

- BUG: the deleteByParentId is hard delete and does not care for soft delete
- YUK: internally we have 3 ways of performing the orphan removal when we really want 2

This change fixes the BUG and fixes the YUK. It does this by removing the special case at: https://github.com/ebean-orm/ebean/blob/ebean-parent-13.11.3/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBeans.java#L347-L350 ... and replacing it with the more common orphan removal code used when we do not have BeanCollection modifications.

The result of this change is that in SaveManyBeans internals we get back to have 2 ways to remove orphans.

- A BeanCollection with modifications: Orphans explicitly deleted using the known elements removed from the collection
- All other cases: Orphans as everything NOT in the collection that is going to be updated

ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBeans.java

@@ -35,8 +35,8 @@ final class SaveManyBeans extends SaveManyBase {
   private final boolean untouchedBeanCollection;
   private final Collection<?> collection;
   private final boolean hasOrderColumn;
+  private final boolean forcedUpdate;
   private int sortOrder;
-  private boolean insertAllChildren;
   private boolean forceOrphanRemoval;
 
   SaveManyBeans(DefaultPersister persister, boolean insertedParent, BeanPropertyAssocMany<?> many, EntityBean parentBean, PersistRequestBean<?> request) {
@@ -50,6 +50,7 @@ final class SaveManyBeans extends SaveManyBase {
     this.untouchedBeanCollection = untouchedBeanCollection();
     this.collection = cascade ? BeanCollectionUtil.getActualEntries(value) : null;
     this.hasOrderColumn = many.hasOrderColumn();
+    this.forcedUpdate = request.isForcedUpdate();
   }
 
   /**
@@ -128,10 +129,8 @@ final class SaveManyBeans extends SaveManyBase {
       targetDescriptor.preAllocateIds(collection.size());
     }
     if (forcedUpdateOrphanRemoval()) {
-      // collect the Id's (to exclude from deleteManyDetails)
-      List<Object> detailIds = collectIds(collection, targetDescriptor, isMap);
-      // deleting missing children - children not in our collected detailIds
-      persister.deleteManyDetails(transaction, many.descriptor(), parentBean, many, detailIds, deleteMode);
+      // deleting orphans, anything not in our detailsIds
+      persister.deleteManyDetails(transaction, many.descriptor(), parentBean, many, detailIds(), deleteMode);
     }
     transaction.depth(+1);
     saveAllBeans(orderColumn);
@@ -142,7 +141,7 @@ final class SaveManyBeans extends SaveManyBase {
   }
 
   private boolean forcedUpdateOrphanRemoval() {
-    return !insertedParent && many.isOrphanRemoval() && (forceOrphanRemoval || request.isForcedUpdate());
+    return !insertedParent && many.isOrphanRemoval() && (forceOrphanRemoval || forcedUpdate);
   }
 
   private void saveAllBeans(final BeanProperty orderColumn) {
@@ -178,10 +177,6 @@ final class SaveManyBeans extends SaveManyBase {
             skipSavingThisBean = false;
             // set the parent bean to detailBean
             many.setJoinValuesToChild(parentBean, detail, mapKeyValue);
-          } else if (insertAllChildren) {
-            ebi.setNew();
-            skipSavingThisBean = false;
-            many.setJoinValuesToChild(parentBean, detail, mapKeyValue);
           } else {
             skipSavingThisBean = saveRecurseSkippable;
           }
@@ -216,12 +211,10 @@ final class SaveManyBeans extends SaveManyBase {
   }
 
   /**
-   * Collect the Id values of the details to remove 'missing children' for stateless updates.
+   * Return the Id values of beans we know are being updated (any others are orphans)
    */
-  private List<Object> collectIds(Collection<?> collection, BeanDescriptor<?> targetDescriptor, boolean isMap) {
-    List<Object> detailIds = new ArrayList<>();
-    // stateless update with deleteMissingChildren so first
-    // collect the Id values to remove the 'missing children'
+  private List<Object> detailIds() {
+    final var detailIds = new ArrayList<>();
     for (Object detailBean : collection) {
       if (isMap) {
         detailBean = ((Map.Entry<?, ?>) detailBean).getValue();
@@ -229,8 +222,8 @@ final class SaveManyBeans extends SaveManyBase {
       if (detailBean instanceof EntityBean) {
         Object id = targetDescriptor.id(detailBean);
         if (!isNullOrZero(id)) {
-          if (!forceOrphanRemoval || !((EntityBean)detailBean)._ebean_getIntercept().isNew()) {
-            // remember the Id (other details not in the collection) will be removed
+          if (forcedUpdate || !((EntityBean) detailBean)._ebean_getIntercept().isNew()) {
+            // Id of bean that will be updated, exclude it from orphan removal
             detailIds.add(id);
           }
         }
@@ -264,8 +257,7 @@ final class SaveManyBeans extends SaveManyBase {
   }
 
   private void saveAssocManyIntersection(boolean queue) {
-    boolean forcedUpdate = request.isForcedUpdate();
-    boolean vanillaCollection = !(value instanceof BeanCollection<?>);
+    final boolean vanillaCollection = !(value instanceof BeanCollection<?>);
     if (vanillaCollection || forcedUpdate) {
       // delete all intersection rows and then treat all
       // beans in the collection as additions
@@ -324,7 +316,7 @@ final class SaveManyBeans extends SaveManyBase {
           CoreLog.log.log(System.Logger.Level.WARNING, m);
         } else {
           if (!many.hasImportedId(otherBean)) {
-            throw new PersistenceException("ManyToMany bean " + otherBean + " does not have an Id value.");
+            throw new PersistenceException("ManyToMany bean does not have an Id value? " + otherBean);
           } else {
             // build a intersection row for 'insert'
             IntersectionRow intRow = many.buildManyToManyMapBean(parentBean, otherBean, publish);
@@ -347,10 +339,7 @@ final class SaveManyBeans extends SaveManyBase {
       return;
     }
     if (!(value instanceof BeanCollection<?>)) {
-      if (!forcedUpdateOrphanRemoval() && (!insertedParent && cascade && isChangedProperty())) {
-        persister.addToFlushQueue(many.deleteByParentId(request.beanId(), null), transaction, 0);
-        insertAllChildren = true;
-      }
+      forceOrphanRemoval = !insertedParent && isChangedProperty();
     } else {
       BeanCollection<?> c = (BeanCollection<?>) value;
       Set<?> modifyRemovals = c.getModifyRemovals();

ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java

@@ -1,6 +1,7 @@
 package org.tests.cascade;
 
 import io.ebean.DB;
+import io.ebean.test.LoggedSql;
 import io.ebean.xtest.BaseTestCase;
 import org.junit.jupiter.api.Test;
 
@@ -9,6 +10,7 @@ import java.util.List;
 import java.util.stream.Collectors;
 
 import static java.util.Objects.requireNonNull;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 class TestOrphanCollectionReplacement extends BaseTestCase {
@@ -41,7 +43,16 @@ class TestOrphanCollectionReplacement extends BaseTestCase {
       updatedRoles.addAll(List.of(role));
       fetchedParent.setChildren(updatedRoles);
 
+      LoggedSql.start();
       DB.save(fetchedParent);
+      var sql = LoggedSql.stop();
+      if (isH2() || isPostgresCompatible()) { // using deleted=true vs deleted=1
+        assertThat(sql).hasSize(4);
+        assertThat(sql.get(0)).contains("update coone_many set deleted=true where coone_id = ? and not ( id in (?) )");
+        assertThat(sql.get(1)).contains(" -- bind(");
+        assertThat(sql.get(2)).contains("insert into coone_many (coone_id, name, deleted) values (?,?,?)");
+        assertThat(sql.get(3)).contains(" -- bind(");
+      }
     }
 
     COOne fetchedUser2 = DB.find(COOne.class, parentId);

ebean-test/src/test/java/org/tests/o2m/TestOneToManyStatelessUpdateResultsInSoftDelete.java

@@ -137,7 +137,14 @@ class TestOneToManyStatelessUpdateResultsInSoftDelete extends BaseTestCase {
     assertThat(goodsAfterInsert.getWorkflowEntity().getOperations()).hasSize(1);
     goodsAfterInsert.getWorkflowEntity().setOperations(List.of());
 
+    LoggedSql.start();
     DB.save(goodsAfterInsert);
+    var sql = LoggedSql.collect();
+    if (isH2() || isPostgresCompatible()) { // using deleted=true vs deleted=1
+      assertThat(sql).hasSize(2);
+      assertThat(sql.get(0)).contains("update workflow_operation_entity set deleted=true where workflow_id = ?");
+    }
+
     assertThat(goodsAfterInsert.getWorkflowEntity().getOperations()).isEmpty();
     assertThat(DB.find(GoodsEntity.class, goods.getId()).getWorkflowEntity().getOperations()).isEmpty();
 
@@ -154,12 +161,27 @@ class TestOneToManyStatelessUpdateResultsInSoftDelete extends BaseTestCase {
 
     // Using save() throws io.ebean.DuplicateKeyException: Error when batch flush on sql: insert into workflow_entity ...
     // Must be an update() and not save() for this to be a "stateless update"
+    LoggedSql.collect();
     DB.update(goodsStateless);
+    sql = LoggedSql.collect();
+    assertThat(sql).isNotEmpty();
+    if (isH2() || isPostgresCompatible()) { // using deleted=true vs deleted=1
+      assertThat(sql).hasSize(7);
+      assertThat(sql.get(0)).contains("update workflow_entity set when_modified=? where id=?");
+      assertThat(sql.get(1)).contains(" -- bind(");
+      assertThat(sql.get(2)).contains("update workflow_operation_entity set deleted=true where workflow_id = ?");
+      assertThat(sql.get(3)).contains(" -- bind(");
+      assertThat(sql.get(4)).contains("insert into workflow_operation_entity (name, version, when_created, when_modified");
+      assertThat(sql.get(5)).contains(" -- bind(");
+      assertThat(sql.get(6)).contains("update goods_entity set when_modified=?, workflow_entity_id=? where id=?");
+    }
 
     var ops = workflow.getOperations();
     // shouldn't contain deleted operations
     assertThat(ops).hasSize(1);
     assertThat(goodsStateless.getWorkflowEntity().getOperations().get(0).getId()).isNotEqualTo(operation1.getId());
+
+    LoggedSql.stop();
   }
 
   @Test
@@ -194,17 +216,23 @@ class TestOneToManyStatelessUpdateResultsInSoftDelete extends BaseTestCase {
     // uncommenting this lines makes the test pass
     //assertThat(goodsStateless.getWorkflowEntity().getOperations().size()).isEqualTo(0);
 
-    var sql = LoggedSql.stop();
-    sql.forEach(System.out::println);
-
-    System.out.println("BEFORE TRY");
-    LoggedSql.start();
+    var sql = LoggedSql.collect();
+    if (isH2() || isPostgresCompatible()) { // using deleted=true vs deleted=1
+      assertThat(sql).hasSize(5);
+      assertThat(sql.get(0)).contains("update workflow_entity set when_modified=? where id=?");
+      assertThat(sql.get(1)).contains(" -- bind(");
+      assertThat(sql.get(2)).contains("update workflow_operation_entity set deleted=true where workflow_id = ?");
+      assertThat(sql.get(3)).contains(" -- bind(");
+      assertThat(sql.get(4)).contains("update goods_entity set when_modified=?, workflow_entity_id=? where id=?");
+    }
 
     try (var writer = new StringWriter()) {
       var mapper = new ObjectMapper();
       mapper.writeValue(writer, goodsStateless);
       sql = LoggedSql.stop();
-      sql.forEach(System.out::println);
+      // queries fired to load the object graph for writing as json
+      assertThat(sql).hasSize(7);
+      sql.forEach(s -> assertThat(s).startsWith("select "));
       /*
       select t0.id, t0.name, t0.workflow_entity_id, t0.version, t0.when_created, t0.when_modified from goods_entity t0 where t0.id = ?; --bind(4, ) --micros(161)
       select t0.id, t0.name, t0.version, t0.when_created, t0.when_modified, t0.created_by, t0.updated_by, t0.workflow_entity_id from goods_entity t0 where t0.id = ?; --bind(4, ) --micros(525)
@@ -227,7 +255,8 @@ class TestOneToManyStatelessUpdateResultsInSoftDelete extends BaseTestCase {
 
       writer.flush();
       var serialized = writer.toString();
-      System.out.println(serialized);
+      // System.out.println(serialized);
+      assertThat(serialized).isNotEmpty();
       var readGoods = mapper.readValue(writer.toString(), GoodsEntity.class);
       assertThat(readGoods.getWorkflowEntity().getOperations()).hasSize(0);
     }
@@ -264,7 +293,16 @@ class TestOneToManyStatelessUpdateResultsInSoftDelete extends BaseTestCase {
       logger.error("Insert instead update", e);
     }
     var sql = LoggedSql.collect();
+    assertThat(sql).isNotEmpty();
     assertThat(sql.get(0)).contains("delete from attachment where goods_entity_id = ?");
+    assertThat(sql.get(1)).contains(" -- bind(");
+    assertThat(sql.get(2)).contains("insert into attachment (id, goods_entity_id, name, ");
+    assertThat(sql.get(3)).contains(" -- bind(");
+    assertThat(sql.get(4)).contains(" -- bind(");
+    if (isH2() || isPostgresCompatible() || isMySql()) { // i.e. using identity, not using sequence
+      assertThat(sql.get(5)).contains("insert into attachment (goods_entity_id, name,");
+      assertThat(sql.get(6)).contains(" -- bind(");
+    }
 
     persistedGoods = DB.find(GoodsEntity.class, goods.getId());
 
@@ -286,7 +324,18 @@ class TestOneToManyStatelessUpdateResultsInSoftDelete extends BaseTestCase {
       logger.error("Insert instead update", e);
     }
     sql = LoggedSql.stop();
+    assertThat(sql).hasSize(9);
     assertThat(sql.get(0)).contains("delete from attachment where id=?");
+    assertThat(sql.get(1)).contains(" -- bind(");
+    assertThat(sql.get(2)).contains(" -- bind(");
+    assertThat(sql.get(3)).contains(" -- bind(");
+    assertThat(sql.get(4)).contains("insert into attachment (id, goods_entity_id, name,");
+    assertThat(sql.get(5)).contains(" -- bind(");
+    assertThat(sql.get(6)).contains(" -- bind(");
+    if (isH2() || isPostgresCompatible()) { // using identity, not using sequence
+      assertThat(sql.get(7)).contains("insert into attachment (goods_entity_id, name, ");
+      assertThat(sql.get(8)).contains(" -- bind(");
+    }
 
     var persistedGoods2 = DB.find(GoodsEntity.class, goods.getId());
     assertThat(persistedGoods2.getAttachments()).hasSize(3);