From d57657c2091be6e70af9154a4adc9734b90bcc34 Mon Sep 17 00:00:00 2001 From: Robin Bygrave Date: Thu, 4 Feb 2021 15:45:19 +1300 Subject: [PATCH] #2127 - Fix for - Adding a new list of children to @OneToMany with orphanRemoval true does not delete existing orphans Modifies EntityBeanIntercept preSetterMany to set the changed state on the many property and use this in SaveManyBeans to detect when the existing beans need orphan removal --- .../io/ebean/bean/EntityBeanIntercept.java | 10 ++- .../server/persist/SaveManyBeans.java | 22 ++----- .../org/tests/batchload/TestBeanState.java | 64 +++++++++++++++++++ .../tests/cascade/TestDeleteO2MOrphans.java | 3 +- 4 files changed, 79 insertions(+), 20 deletions(-) diff --git a/ebean-api/src/main/java/io/ebean/bean/EntityBeanIntercept.java b/ebean-api/src/main/java/io/ebean/bean/EntityBeanIntercept.java index b49c6f8ae..12f3fda8f 100644 --- a/ebean-api/src/main/java/io/ebean/bean/EntityBeanIntercept.java +++ b/ebean-api/src/main/java/io/ebean/bean/EntityBeanIntercept.java @@ -931,10 +931,14 @@ public final class EntityBeanIntercept implements Serializable { * OneToMany and ManyToMany only set loaded state. */ public void preSetterMany(boolean interceptField, int propertyIndex, Object oldValue, Object newValue) { - if (readOnly) { - throw new IllegalStateException("This bean is readOnly"); + if (state == STATE_NEW) { + setLoadedProperty(propertyIndex); + } else { + if (readOnly) { + throw new IllegalStateException("This bean is readOnly"); + } + setChangedProperty(propertyIndex); } - setLoadedProperty(propertyIndex); } private void setChangedPropertyValue(int propertyIndex, boolean setDirtyState, Object origValue) { 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 183956ef8..f273b9dbc 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 @@ -210,18 +210,6 @@ public class SaveManyBeans extends SaveManyBase { return true; } - private boolean hasNewOrDirtyBeans() { - if (collection == null) { - return false; - } - for (Object bean : collection) { - if (bean instanceof EntityBean && ((EntityBean) bean)._ebean_getIntercept().isNewOrDirty()) { - return true; - } - } - return false; - } - /** * Collect the Id values of the details to remove 'missing children' for stateless updates. */ @@ -344,14 +332,18 @@ public class SaveManyBeans extends SaveManyBase { transaction.depth(-1); } + private boolean isChangedProperty() { + return parentBean._ebean_getIntercept().isChangedProperty(many.getPropertyIndex()); + } + private void removeAssocManyOrphans() { if (value == null) { return; } if (!(value instanceof BeanCollection)) { -// if (!insertedParent && cascade && hasNewOrDirtyBeans()) { -// persister.addToFlushQueue(many.deleteByParentId(request.getBeanId(), null), transaction, 0); -// } + if (!insertedParent && cascade && isChangedProperty()) { + persister.addToFlushQueue(many.deleteByParentId(request.getBeanId(), null), transaction, 0); + } } else { BeanCollection c = (BeanCollection) value; Set modifyRemovals = c.getModifyRemovals(); diff --git a/ebean-core/src/test/java/org/tests/batchload/TestBeanState.java b/ebean-core/src/test/java/org/tests/batchload/TestBeanState.java index 0d2c2b60f..28161dc3d 100644 --- a/ebean-core/src/test/java/org/tests/batchload/TestBeanState.java +++ b/ebean-core/src/test/java/org/tests/batchload/TestBeanState.java @@ -9,6 +9,7 @@ import org.junit.Test; import org.tests.model.basic.Customer; import org.tests.model.basic.ResetBasicData; +import java.util.ArrayList; import java.util.List; import static org.assertj.core.api.Assertions.assertThat; @@ -70,4 +71,67 @@ public class TestBeanState extends BaseTestCase { beanState.setDisableLazyLoad(true); assertNull(customer.getName()); } + + @Test + public void getChangedProps_when_setManyProperty() { + + ResetBasicData.reset(); + + Customer customer = DB.find(Customer.class).order("id").setMaxRows(1).findOne(); + + BeanState beanState = DB.getBeanState(customer); + assertThat(beanState.getChangedProps()).isEmpty(); + + customer.setContacts(new ArrayList<>()); + assertThat(beanState.getChangedProps()).containsOnly("contacts"); + } + + @Test + public void getChangedProps_when_setManyProperty_onNewBean() { + + Customer customer = new Customer(); + + BeanState beanState = DB.getBeanState(customer); + assertThat(beanState.getChangedProps()).isEmpty(); + + // when new state, then loaded + customer.setContacts(new ArrayList<>()); + assertThat(beanState.getChangedProps()).isEmpty(); + assertThat(beanState.getLoadedProps()).containsOnly("contacts"); + + // set loaded state, then marked as changed + beanState.setLoaded(); + customer.setContacts(new ArrayList<>()); + assertThat(beanState.getLoadedProps()).containsOnly("contacts"); + assertThat(beanState.getChangedProps()).containsOnly("contacts"); + } + + @Test(expected = IllegalStateException.class) + public void readOnly_when_setManyProperty() { + + Customer customer = new Customer(); + customer.setContacts(new ArrayList<>()); + + BeanState beanState = DB.getBeanState(customer); + beanState.setLoaded(); + beanState.setReadOnly(true); + + // act, try to mutate read only bean + customer.setContacts(new ArrayList<>()); + } + + + @Test(expected = IllegalStateException.class) + public void readOnly_when_setProperty() { + + Customer customer = new Customer(); + customer.setName("a"); + + BeanState beanState = DB.getBeanState(customer); + beanState.setLoaded(); + beanState.setReadOnly(true); + + // act, try to mutate read only bean + customer.setName("b"); + } } diff --git a/ebean-core/src/test/java/org/tests/cascade/TestDeleteO2MOrphans.java b/ebean-core/src/test/java/org/tests/cascade/TestDeleteO2MOrphans.java index 468187657..f4356d2bf 100644 --- a/ebean-core/src/test/java/org/tests/cascade/TestDeleteO2MOrphans.java +++ b/ebean-core/src/test/java/org/tests/cascade/TestDeleteO2MOrphans.java @@ -19,8 +19,7 @@ public class TestDeleteO2MOrphans extends BaseTestCase { // assert COOne check = findById(id); - //FIXME #2127 #2141: assertThat(check.getChildren()).hasSize(2); - assertThat(check.getChildren()).hasSize(4); + assertThat(check.getChildren()).hasSize(2); DB.delete(check); }