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 44c9777b4..f5fbc7d1c 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,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 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 collectIds(Collection collection, BeanDescriptor targetDescriptor, boolean isMap) { - List detailIds = new ArrayList<>(); - // stateless update with deleteMissingChildren so first - // collect the Id values to remove the 'missing children' + private List 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(); diff --git a/ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java b/ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java index 8dbedc9b3..8bacd0230 100644 --- a/ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java +++ b/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); 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 b747cb904..315ce1166 100644 --- a/ebean-test/src/test/java/org/tests/o2m/TestOneToManyStatelessUpdateResultsInSoftDelete.java +++ b/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);