From 244d45674a243fc578d4aa7f47689140ba8131c2 Mon Sep 17 00:00:00 2001
From: Rob Bygrave <robin.bygrave@gmail.com>
Date: Thu, 9 Feb 2023 23:29:13 +1300
Subject: [PATCH] #2952 #2953 - Fix for @OneToMany orphanRemoval not occurring

orphanRemoval was not occurring when a loaded bean had a collection
replaced by a new BeanCollection (as opposed to a vanilla collection
like java.util.ArrayList).

Json marshalling a collection puts beans into BeanCollection and this
is part of the test that reproduced this issue.
---
 .../server/persist/SaveManyBeans.java         | 11 ++++--
 ...anyStatelessUpdateResultsInSoftDelete.java | 39 +++++++++++--------
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBeans.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBeans.java
index 5b6858e78..7d0790d2d 100644
--- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBeans.java
+++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBeans.java
@@ -37,6 +37,7 @@ final class SaveManyBeans extends SaveManyBase {
   private final boolean hasOrderColumn;
   private int sortOrder;
   private boolean insertAllChildren;
+  private boolean forceOrphanRemoval;
 
   SaveManyBeans(DefaultPersister persister, boolean insertedParent, BeanPropertyAssocMany<?> many, EntityBean parentBean, PersistRequestBean<?> request) {
     super(persister, insertedParent, many, parentBean, request);
@@ -141,7 +142,7 @@ final class SaveManyBeans extends SaveManyBase {
   }
 
   private boolean forcedUpdateOrphanRemoval() {
-    return !insertedParent && many.isOrphanRemoval() && request.isForcedUpdate();
+    return !insertedParent && many.isOrphanRemoval() && (forceOrphanRemoval || request.isForcedUpdate());
   }
 
   private void saveAllBeans(final BeanProperty orderColumn) {
@@ -228,8 +229,10 @@ final class SaveManyBeans extends SaveManyBase {
       if (detailBean instanceof EntityBean) {
         Object id = targetDescriptor.id(detailBean);
         if (!isNullOrZero(id)) {
-          // remember the Id (other details not in the collection) will be removed
-          detailIds.add(id);
+          if (!forceOrphanRemoval || !((EntityBean)detailBean)._ebean_getIntercept().isNew()) {
+            // remember the Id (other details not in the collection) will be removed
+            detailIds.add(id);
+          }
         }
       }
     }
@@ -369,6 +372,8 @@ final class SaveManyBeans extends SaveManyBase {
             }
           }
         }
+      } else {
+        forceOrphanRemoval = !insertedParent && isChangedProperty();
       }
     }
   }
diff --git a/ebean-test/src/test/java/org/tests/o2m/TestOneToManyStatelessUpdateResultsInSoftDelete.java b/ebean-test/src/test/java/org/tests/o2m/TestOneToManyStatelessUpdateResultsInSoftDelete.java
index ad8036b1f..b747cb904 100644
--- a/ebean-test/src/test/java/org/tests/o2m/TestOneToManyStatelessUpdateResultsInSoftDelete.java
+++ b/ebean-test/src/test/java/org/tests/o2m/TestOneToManyStatelessUpdateResultsInSoftDelete.java
@@ -8,7 +8,6 @@ import org.junit.jupiter.api.Test;
 import org.tests.o2m.dm.*;
 
 import java.io.StringWriter;
-import java.sql.SQLException;
 import java.util.List;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -240,48 +239,56 @@ class TestOneToManyStatelessUpdateResultsInSoftDelete extends BaseTestCase {
     attachment1.setName("File1");
     var attachment2 = new Attachment();
     attachment2.setName("File2");
-    var attachment3 = new Attachment();
-    attachment3.setName("File3");
+
     var goods = new GoodsEntity();
     goods.setName("goods1");
     goods.setAttachments(List.of(attachment1, attachment2));
 
-    LoggedSql.start();
-
     DB.save(goods);
 
-    var marshaledGoods = DB.json().toBean(GoodsEntity.class, DB.json().toJson(goods));
-    marshaledGoods.getAttachments().add(attachment3);
+    String goodAsJson = DB.json().toJson(goods);
+    var marshaledGoods = DB.json().toBean(GoodsEntity.class, goodAsJson);
+    var attachment3a = new Attachment();
+    attachment3a.setName("File3");
+    marshaledGoods.getAttachments().add(attachment3a);
 
     var persistedGoods = DB.find(GoodsEntity.class, goods.getId());
 
-    // this forces insert and throws exception due primary key conflict
+    // this was not deleting orphans and so the inserts throw exception due primary key conflict
     persistedGoods.setAttachments(marshaledGoods.getAttachments());
+    LoggedSql.start();
     try {
       logger.info("Saving goods with set new attachments list");
       DB.save(persistedGoods);
     } catch (Exception e) {
       logger.error("Insert instead update", e);
     }
+    var sql = LoggedSql.collect();
+    assertThat(sql.get(0)).contains("delete from attachment where goods_entity_id = ?");
 
     persistedGoods = DB.find(GoodsEntity.class, goods.getId());
 
-    // this works good
+    // this works good using .clear() and .addAll()
+    // because here we are mutating the collection, it detects the
+    // orphan removals occurring via the .clear()
+    marshaledGoods = DB.json().toBean(GoodsEntity.class, goodAsJson);
+    var attachment3b = new Attachment();
+    attachment3b.setName("File3");
+    marshaledGoods.getAttachments().add(attachment3b);
+
     persistedGoods.getAttachments().clear();
     persistedGoods.getAttachments().addAll(marshaledGoods.getAttachments());
+    LoggedSql.collect();
     try {
       logger.info("Saving goods with clear/added attachments");
-        DB.save(persistedGoods);
+      DB.save(persistedGoods);
     } catch (Exception e) {
       logger.error("Insert instead update", e);
     }
+    sql = LoggedSql.stop();
+    assertThat(sql.get(0)).contains("delete from attachment where id=?");
+
     var persistedGoods2 = DB.find(GoodsEntity.class, goods.getId());
     assertThat(persistedGoods2.getAttachments()).hasSize(3);
-
-    var sql = LoggedSql.collect();
-
-    LoggedSql.stop();
-
-    System.out.println(String.join("\n",sql));
   }
 }