From 55324653a5843cb6bbf9a5ef0aaa1c856261dfb1 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Wed, 16 Dec 2020 15:36:44 +1300 Subject: [PATCH] #2127 - Adding a new list of children to @OneToMany with orphanRemoval true does not delete existing orphans (#2131) --- .../server/core/PersistRequestUpdateSql.java | 4 +- .../ebeaninternal/server/core/Persister.java | 12 ++--- .../server/persist/BatchControl.java | 25 +++------ .../server/persist/DefaultPersister.java | 24 +++------ .../server/persist/SaveManyBase.java | 2 +- .../server/persist/SaveManyBeans.java | 14 ++--- .../persist/SaveManyElementCollection.java | 2 +- .../persist/SaveManyElementCollectionMap.java | 2 +- .../test/java/org/tests/cascade/COOne.java | 3 ++ .../tests/cascade/TestDeleteO2MOrphans.java | 51 +++++++++++++++++++ 10 files changed, 86 insertions(+), 53 deletions(-) create mode 100644 ebean-core/src/test/java/org/tests/cascade/TestDeleteO2MOrphans.java diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestUpdateSql.java b/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestUpdateSql.java index fc2ae7419..80471f5aa 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestUpdateSql.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestUpdateSql.java @@ -73,13 +73,13 @@ public final class PersistRequestUpdateSql extends PersistRequest { /** * Add this request to BatchControl to flush later. */ - public void addToFlushQueue(boolean early) { + public void addToFlushQueue(int pos) { BatchControl control = transaction.getBatchControl(); if (control == null) { control = persistExecute.createBatchControl(transaction); } flushQueue = true; - control.addToFlushQueue(this, early); + control.addToFlushQueue(this, pos); } @Override diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/core/Persister.java b/ebean-core/src/main/java/io/ebeaninternal/server/core/Persister.java index b8bb132e1..587ce0625 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/core/Persister.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/core/Persister.java @@ -104,18 +104,14 @@ public interface Persister { void executeOrQueue(SpiSqlUpdate update, SpiTransaction t, boolean queue); /** - * Queue the SqlUpdate for early execution (with JDBC batch). + * Queue the SqlUpdate for execution with position 0, 1 or 2 defining + * when it executes relative to the flush of beans . */ - void addToFlushQueue(SpiSqlUpdate update, SpiTransaction t); + void addToFlushQueue(SpiSqlUpdate update, SpiTransaction t, int pos); /** - * Queue the SqlUpdate for late execution (with JDBC batch). + * Add the statement to JDBC batch for later execution via executeBatch. */ - void addToFlushQueueLast(SpiSqlUpdate update, SpiTransaction t); - - /** - * Add the statement to JDBC batch for later execution via executeBatch. - */ void addBatch(SpiSqlUpdate sqlUpdate, SpiTransaction transaction); /** diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchControl.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchControl.java index ba826af34..bc4b00c4e 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchControl.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchControl.java @@ -81,8 +81,7 @@ public final class BatchControl { */ private int bufferMax; - private Queue earlyQueue; - private Queue lateQueue; + private Queue[] queues = new Queue[3]; /** * Create for a given transaction, PersistExecute, default size and getGeneratedKeys. @@ -271,9 +270,10 @@ public final class BatchControl { } private void flushBuffer(boolean reset) throws BatchedSqlException { + flushQueue(queues[0]); flushInternal(reset); - flushQueue(earlyQueue); - flushQueue(lateQueue); + flushQueue(queues[1]); + flushQueue(queues[2]); } private void flushQueue(Queue queue) throws BatchedSqlException { @@ -368,20 +368,11 @@ public final class BatchControl { /** * Add a SqlUpdate request to execute after flush. */ - public void addToFlushQueue(PersistRequestUpdateSql request, boolean early) { - if (early) { - // add it to the early queue - if (earlyQueue == null) { - earlyQueue = new Queue(); - } - earlyQueue.add(request); - } else { - // add it to the late queue - if (lateQueue == null) { - lateQueue = new Queue(); - } - lateQueue.add(request); + public void addToFlushQueue(PersistRequestUpdateSql request, int pos) { + if (queues[pos] == null) { + queues[pos] = new Queue(); } + queues[pos].add(request); } private static class Queue { diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java index 467e823d4..a7b21f0c8 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java @@ -141,24 +141,17 @@ public final class DefaultPersister implements Persister { @Override public void executeOrQueue(SpiSqlUpdate update, SpiTransaction t, boolean queue) { if (queue) { - addToFlushQueue(update, t, false); + addToFlushQueue(update, t, 2); } else { executeSqlUpdate(update, t); } } - @Override - public void addToFlushQueue(SpiSqlUpdate update, SpiTransaction t) { - addToFlushQueue(update, t, true); - } - - @Override - public void addToFlushQueueLast(SpiSqlUpdate update, SpiTransaction t) { - addToFlushQueue(update, t, false); - } - - private void addToFlushQueue(SpiSqlUpdate update, SpiTransaction t, boolean early) { - new PersistRequestUpdateSql(server, update, t, persistExecute).addToFlushQueue(early); + /** + * Add to the flush queue in position 0, 1 or 2. + */ + public void addToFlushQueue(SpiSqlUpdate update, SpiTransaction t, int pos) { + new PersistRequestUpdateSql(server, update, t, persistExecute).addToFlushQueue(pos); } /** @@ -963,7 +956,7 @@ public final class DefaultPersister implements Persister { void deleteManyIntersection(EntityBean bean, BeanPropertyAssocMany many, SpiTransaction t, boolean publish, boolean queue) { SpiSqlUpdate sqlDelete = deleteAllIntersection(bean, many, publish); if (queue) { - addToFlushQueue(sqlDelete, t, true); + addToFlushQueue(sqlDelete, t, 1); } else { executeSqlUpdate(sqlDelete, t); } @@ -1237,7 +1230,6 @@ public final class DefaultPersister implements Persister { @SuppressWarnings({"unchecked"}) private PersistRequestBean createRequest(T bean, Transaction t, Object parentBean, BeanManager mgr, PersistRequest.Type type, int flags) { - // no delete requests come here return new PersistRequestBean(server, bean, parentBean, mgr, (SpiTransaction) t, persistExecute, type, flags); } @@ -1252,7 +1244,6 @@ public final class DefaultPersister implements Persister { @SuppressWarnings({"unchecked"}) private PersistRequestBean createDeleteRequest(Object bean, Transaction t, PersistRequest.Type type, int flags) { - BeanManager mgr = getBeanManager(bean); if (type == Type.DELETE_PERMANENT) { type = Type.DELETE; @@ -1281,7 +1272,6 @@ public final class DefaultPersister implements Persister { */ @SuppressWarnings("unchecked") private BeanManager getBeanManager(Object bean) { - BeanManager mgr = (BeanManager) beanDescriptorManager.getBeanManager(bean.getClass()); if (mgr == null) { throw new PersistenceException(errNotRegistered(bean.getClass())); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBase.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBase.java index c463c330b..f305e3244 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBase.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBase.java @@ -49,7 +49,7 @@ abstract class SaveManyBase implements SaveMany { void preElementCollectionUpdate() { if (!insertedParent) { request.preElementCollectionUpdate(); - persister.addToFlushQueue(many.deleteByParentId(request.getBeanId(), null), transaction); + persister.addToFlushQueue(many.deleteByParentId(request.getBeanId(), null), transaction, 1); } } 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 003a295c8..114aa232e 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 @@ -235,9 +235,6 @@ public class SaveManyBeans extends SaveManyBase { /** * Save the additions and removals from a ManyToMany collection as inserts * and deletes from the intersection table. - *

- * This is done via MapBeans. - *

*/ private void saveAssocManyIntersection() { if (value == null) { @@ -336,9 +333,14 @@ public class SaveManyBeans extends SaveManyBase { } private void removeAssocManyOrphans() { - // check that the list is not null and if it is a BeanCollection - // check that is has been populated (don't trigger lazy loading) - if (value instanceof BeanCollection) { + if (value == null) { + return; + } + if (!(value instanceof BeanCollection)) { + if (!insertedParent) { + persister.addToFlushQueue(many.deleteByParentId(request.getBeanId(), null), transaction, 0); + } + } else { BeanCollection c = (BeanCollection) value; Set modifyRemovals = c.getModifyRemovals(); if (insertedParent) { diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyElementCollection.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyElementCollection.java index e7622ce56..53f638114 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyElementCollection.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyElementCollection.java @@ -48,7 +48,7 @@ class SaveManyElementCollection extends SaveManyBase { final SpiSqlUpdate sqlInsert = proto.copy(); sqlInsert.setParameter(parentId); many.bindElementValue(sqlInsert, value); - persister.addToFlushQueueLast(sqlInsert, transaction); + persister.addToFlushQueue(sqlInsert, transaction, 2); } resetModifyState(); postElementCollectionUpdate(); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyElementCollectionMap.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyElementCollectionMap.java index 552f21424..60dd2e01d 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyElementCollectionMap.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyElementCollectionMap.java @@ -51,7 +51,7 @@ class SaveManyElementCollectionMap extends SaveManyBase { sqlInsert.setParameter(parentId); sqlInsert.setParameter(entry.getKey()); many.bindElementValue(sqlInsert, entry.getValue()); - persister.addToFlushQueueLast(sqlInsert, transaction); + persister.addToFlushQueue(sqlInsert, transaction, 2); } resetModifyState(); postElementCollectionUpdate(); diff --git a/ebean-core/src/test/java/org/tests/cascade/COOne.java b/ebean-core/src/test/java/org/tests/cascade/COOne.java index cc2430574..07c92ecac 100644 --- a/ebean-core/src/test/java/org/tests/cascade/COOne.java +++ b/ebean-core/src/test/java/org/tests/cascade/COOne.java @@ -34,4 +34,7 @@ public class COOne { return children; } + public void setChildren(List children) { + this.children = children; + } } diff --git a/ebean-core/src/test/java/org/tests/cascade/TestDeleteO2MOrphans.java b/ebean-core/src/test/java/org/tests/cascade/TestDeleteO2MOrphans.java new file mode 100644 index 000000000..f4356d2bf --- /dev/null +++ b/ebean-core/src/test/java/org/tests/cascade/TestDeleteO2MOrphans.java @@ -0,0 +1,51 @@ +package org.tests.cascade; + +import io.ebean.BaseTestCase; +import io.ebean.DB; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +public class TestDeleteO2MOrphans extends BaseTestCase { + + @Test + public void test() { + final long id = setup(); + // act + setNewChildren(id); + + // assert + COOne check = findById(id); + assertThat(check.getChildren()).hasSize(2); + DB.delete(check); + } + + private COOne findById(long id) { + return DB.find(COOne.class).where().idEq(id).findOne(); + } + + private void setNewChildren(long id) { + COOne found = findById(id); + found.setChildren(createManies("M3", "M4")); + DB.update(found); + } + + private long setup() { + COOne company = new COOne("P0"); + company.setChildren(createManies("M1", "M2")); + DB.insert(company); + return company.getId(); + } + + private List createManies(String name1, String name2) { + COOneMany employee1 = new COOneMany(name1); + COOneMany employee2 = new COOneMany(name2); + List employees = new ArrayList<>(); + employees.add(employee1); + employees.add(employee2); + return employees; + } +}