Merge pull request #2962 from ebean-orm/feature/2961

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

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);