From 74174444bbee8f2505af8ba08f9076d9fb2b0af2 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Thu, 12 May 2022 23:14:26 +1200 Subject: [PATCH 1/2] Bug - OneToMany orphanRemoval = true, replace collection adding back original collection entry - Adding back a bean from the original collection - Expect that bean to exist in the final result but, it's missing Fix here is in SaveManyBeans to add a insertAllChildren and use ebi.setNew(); for beans to force them to insert for this case. --- .../server/persist/SaveManyBeans.java | 7 ++- .../TestOrphanCollectionReplacement.java | 51 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java 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 f6f2f241c..57b18f312 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 @@ -35,6 +35,7 @@ public final class SaveManyBeans extends SaveManyBase { private final boolean untouchedBeanCollection; private final Collection collection; private int sortOrder; + private boolean insertAllChildren; SaveManyBeans(DefaultPersister persister, boolean insertedParent, BeanPropertyAssocMany many, EntityBean parentBean, PersistRequestBean request) { super(persister, insertedParent, many, parentBean, request); @@ -176,8 +177,11 @@ public 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 { - // unmodified so skip depending on prop.isSaveRecurseSkippable(); skipSavingThisBean = saveRecurseSkippable; } } @@ -342,6 +346,7 @@ public final class SaveManyBeans extends SaveManyBase { if (!(value instanceof BeanCollection)) { if (!insertedParent && cascade && isChangedProperty()) { persister.addToFlushQueue(many.deleteByParentId(request.beanId(), null), transaction, 0); + insertAllChildren = true; } } else { BeanCollection c = (BeanCollection) value; diff --git a/ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java b/ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java new file mode 100644 index 000000000..8dbedc9b3 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java @@ -0,0 +1,51 @@ +package org.tests.cascade; + +import io.ebean.DB; +import io.ebean.xtest.BaseTestCase; +import org.junit.jupiter.api.Test; + +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; + +import static java.util.Objects.requireNonNull; +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestOrphanCollectionReplacement extends BaseTestCase { + + @Test + void replaceCollection_whenOrphan_expect_forcedInsert() { + long parentId; + { // setup + List children = new ArrayList<>(); + children.add(new COOneMany("c0")); + children.add(new COOneMany("c1")); + + COOne parent = new COOne("p0"); + parent.setChildren(children); + + DB.save(parent); + parentId = parent.getId(); + } + + { // act + COOne fetchedParent = DB.find(COOne.class, parentId); + assert fetchedParent != null; + + COOneMany role = new COOneMany("c2"); + + List filtered = fetchedParent.getChildren().stream().filter(r -> "c0".equals(r.getName())).collect(Collectors.toList()); + + List updatedRoles = new ArrayList<>(); + updatedRoles.addAll(filtered); + updatedRoles.addAll(List.of(role)); + fetchedParent.setChildren(updatedRoles); + + DB.save(fetchedParent); + } + + COOne fetchedUser2 = DB.find(COOne.class, parentId); + requireNonNull(fetchedUser2); + assertEquals(2, fetchedUser2.getChildren().size()); + } +} From a4df643d82d2f5df3c79e970dddb88d0d89b76cf Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Fri, 13 May 2022 11:38:39 +1200 Subject: [PATCH 2/2] #2690 - Refactor internals, tidy SaveManyBeans - Introduce hasOrderColumn field - Add clearedParent flag to only clear the parent bean from persistence context once (not per detail bean) --- .../server/persist/SaveManyBeans.java | 30 ++++++++----------- .../java/org/tests/order/TestOrderColumn.java | 10 +++---- 2 files changed, 18 insertions(+), 22 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 57b18f312..1d1008765 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 @@ -24,7 +24,7 @@ import static io.ebeaninternal.server.persist.DmlUtil.isNullOrZero; /** * Saves the details for a OneToMany or ManyToMany relationship (entity beans). */ -public final class SaveManyBeans extends SaveManyBase { +final class SaveManyBeans extends SaveManyBase { private final boolean cascade; private final boolean publish; @@ -34,6 +34,7 @@ public final class SaveManyBeans extends SaveManyBase { private final DeleteMode deleteMode; private final boolean untouchedBeanCollection; private final Collection collection; + private final boolean hasOrderColumn; private int sortOrder; private boolean insertAllChildren; @@ -47,6 +48,7 @@ public final class SaveManyBeans extends SaveManyBase { this.deleteMode = targetDescriptor.isSoftDelete() ? DeleteMode.SOFT : DeleteMode.HARD; this.untouchedBeanCollection = untouchedBeanCollection(); this.collection = cascade ? BeanCollectionUtil.getActualEntries(value) : null; + this.hasOrderColumn = many.hasOrderColumn(); } /** @@ -74,7 +76,7 @@ public final class SaveManyBeans extends SaveManyBase { resetModifyState(); } } else { - if (isModifyListenMode() || many.hasOrderColumn()) { + if (isModifyListenMode() || hasOrderColumn) { // delete any removed beans / orphans removeAssocManyOrphans(); } @@ -91,6 +93,7 @@ public final class SaveManyBeans extends SaveManyBase { private boolean isSaveIntersection() { if (!many.isManyToMany()) { + // OneToMany JoinTable return true; } return transaction.isSaveAssocManyIntersection(many.intersectionTableJoin().getTable(), many.descriptor().rootName()); @@ -113,26 +116,22 @@ public final class SaveManyBeans extends SaveManyBase { private void processDetails() { BeanProperty orderColumn = null; - boolean hasOrderColumn = many.hasOrderColumn(); if (hasOrderColumn) { if (!insertedParent && canSkipForOrderColumn() && saveRecurseSkippable) { return; } orderColumn = targetDescriptor.orderColumn(); } - if (insertedParent) { // performance optimisation for large collections targetDescriptor.preAllocateIds(collection.size()); } - if (!insertedParent && many.isOrphanRemoval() && request.isForcedUpdate()) { // collect the Id's (to exclude from deleteManyDetails) List detailIds = collectIds(collection, targetDescriptor, isMap); // deleting missing children - children not in our collected detailIds persister.deleteManyDetails(transaction, many.descriptor(), parentBean, many, detailIds, deleteMode); } - transaction.depth(+1); saveAllBeans(orderColumn); if (hasOrderColumn) { @@ -141,17 +140,14 @@ public final class SaveManyBeans extends SaveManyBase { transaction.depth(-1); } - private void saveAllBeans(BeanProperty orderColumn) { - // if a map, then we get the key value and - // set it to the appropriate property on the - // detail bean before we save it + private void saveAllBeans(final BeanProperty orderColumn) { Object mapKeyValue = null; boolean skipSavingThisBean; - + boolean clearedParent = false; for (Object detailBean : collection) { sortOrder++; if (isMap) { - // its a map so need the key and value + // a map so need the key and value Map.Entry entry = (Map.Entry) detailBean; mapKeyValue = entry.getKey(); detailBean = entry.getValue(); @@ -170,7 +166,7 @@ public final class SaveManyBeans extends SaveManyBase { ebi.setDirty(true); } } - if (targetDescriptor.isReference(ebi) && originalOrder == 0) { + if (originalOrder == 0 && targetDescriptor.isReference(ebi)) { // we can skip this one skipSavingThisBean = true; } else if (ebi.isNewOrDirty()) { @@ -185,13 +181,13 @@ public final class SaveManyBeans extends SaveManyBase { skipSavingThisBean = saveRecurseSkippable; } } - if (!skipSavingThisBean) { persister.saveRecurse(detail, transaction, parentBean, request.flags()); - if (many.hasOrderColumn()) { - // Clear the bean from the PersistenceContext (L1 cache), because the order of referenced beans might have changed + if (hasOrderColumn && !clearedParent) { + // Clear the parent bean from the PersistenceContext (L1 cache), because the order of referenced beans might have changed final BeanDescriptor beanDescriptor = many.descriptor(); beanDescriptor.contextClear(transaction.getPersistenceContext(), beanDescriptor.getId(parentBean)); + clearedParent = true; } } } @@ -356,7 +352,7 @@ public final class SaveManyBeans extends SaveManyBase { c.setModifyListening(many.modifyListenMode()); } // We must not reset when we still have to update other entities in the collection and set their new orderColumn value - if (!many.hasOrderColumn()) { + if (!hasOrderColumn) { c.modifyReset(); } if (modifyRemovals != null && !modifyRemovals.isEmpty()) { diff --git a/ebean-test/src/test/java/org/tests/order/TestOrderColumn.java b/ebean-test/src/test/java/org/tests/order/TestOrderColumn.java index a6e198735..54587376d 100644 --- a/ebean-test/src/test/java/org/tests/order/TestOrderColumn.java +++ b/ebean-test/src/test/java/org/tests/order/TestOrderColumn.java @@ -10,10 +10,10 @@ import java.util.List; import static org.assertj.core.api.Assertions.assertThat; -public class TestOrderColumn extends TransactionalTestCase { +class TestOrderColumn extends TransactionalTestCase { @Test - public void testOrderColumnInheritance() { + void testOrderColumnInheritance() { final OrderMaster master = new OrderMaster(); for (int i = 0; i < 5; i++) { @@ -33,7 +33,7 @@ public class TestOrderColumn extends TransactionalTestCase { } @Test - public void testOrderColumnSortChange() { + void testOrderColumnSortChange() { final OrderMaster master = new OrderMaster(); for (int i = 0; i < 5; i++) { @@ -62,7 +62,7 @@ public class TestOrderColumn extends TransactionalTestCase { } @Test - public void testModifyTree() { + void testModifyTree() { final OrderMaster master = new OrderMaster(); for (int i = 0; i < 5; i++) { @@ -108,7 +108,7 @@ public class TestOrderColumn extends TransactionalTestCase { } @Test - public void testRemoveElement() { + void testRemoveElement() { final OrderMaster master = new OrderMaster(); for (int i = 0; i < 5; i++) {