From 2a24ec3d49c210927d236c2601fd3202eab7eda0 Mon Sep 17 00:00:00 2001 From: Roland Praml Date: Mon, 21 Aug 2023 16:51:19 +0200 Subject: [PATCH] Added some tests --- .../server/deploy/BeanPropertyAssocMany.java | 4 +- .../deploy/BeanPropertyAssocManySqlHelp.java | 12 +- .../server/deploy/IntersectionRow.java | 5 +- .../server/persist/DefaultPersister.java | 5 +- .../server/persist/SaveManyBeans.java | 12 +- .../TestOrphanCollectionReplacement.java | 151 ++++++++++++++---- 6 files changed, 145 insertions(+), 44 deletions(-) diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocMany.java b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocMany.java index fa00497d9..3990235fc 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocMany.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocMany.java @@ -318,7 +318,7 @@ public class BeanPropertyAssocMany extends BeanPropertyAssoc implements ST /** * Find the Id's of detail beans given a parent Id and optionally exclude detail IDs */ - public List findIdsByParentId(Object parentId, Transaction t, boolean hard, List excludeDetailIds) { + public List findIdsByParentId(Object parentId, Transaction t, boolean hard, Set excludeDetailIds) { return sqlHelp.findIdsByParentId(parentId, t, hard, excludeDetailIds); } @@ -755,7 +755,7 @@ public class BeanPropertyAssocMany extends BeanPropertyAssoc implements ST throw new PersistenceException(from + ": Could not find mapKey property " + mapKey + " on " + to); } - public IntersectionRow buildManyDeleteChildren(EntityBean parentBean, List excludeDetailIds) { + public IntersectionRow buildManyDeleteChildren(EntityBean parentBean, Set excludeDetailIds) { IntersectionRow row = new IntersectionRow(tableJoin.getTable(), targetDescriptor); if (excludeDetailIds != null && !excludeDetailIds.isEmpty()) { row.setExcludeIds(excludeDetailIds, targetDescriptor()); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocManySqlHelp.java b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocManySqlHelp.java index de50f63f1..b66250aac 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocManySqlHelp.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocManySqlHelp.java @@ -10,6 +10,7 @@ import io.ebeaninternal.server.deploy.visitor.VisitProperties; import io.ebeaninternal.server.util.Str; import java.util.List; +import java.util.Set; class BeanPropertyAssocManySqlHelp { @@ -123,14 +124,23 @@ class BeanPropertyAssocManySqlHelp { many.bindParentIdsIn(rawWhere, parentIds, query); } - List findIdsByParentId(Object parentId, Transaction t, boolean hard, List excludeDetailIds) { + List findIdsByParentId(Object parentId, Transaction t, boolean hard, Set excludeDetailIds) { final SpiEbeanServer server = descriptor.ebeanServer(); final SpiQuery query = many.newQuery(server); many.bindParentIdEq(rawParentIdEQ(""), parentId, query); if (hard) { query.setIncludeSoftDeletes(); } + if (excludeDetailIds != null && !excludeDetailIds.isEmpty()) { + if (excludeDetailIds.size() > 1000) { // TODO: Wait for #3176 + // if we hit the parameter limit, we must filter that on the java side. + // There is no easy way to batch "not in" queries. + // checkme: We could pass the first 1000-2000 params to the DB and filter the rest + List ret = server.findIds(query, t); + ret.removeIf(id -> excludeDetailIds.contains(id)); + return ret; + } query.where().not(query.getExpressionFactory().idIn(excludeDetailIds)); } return server.findIds(query, t); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/IntersectionRow.java b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/IntersectionRow.java index 76c732603..1475ef636 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/IntersectionRow.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/IntersectionRow.java @@ -11,13 +11,14 @@ import io.ebeaninternal.server.persist.DeleteMode; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; public final class IntersectionRow { private final String tableName; private final BeanDescriptor targetDescriptor; private final LinkedHashMap values = new LinkedHashMap<>(); - private List excludeIds; + private Set excludeIds; private BeanDescriptor excludeDescriptor; IntersectionRow(String tableName, BeanDescriptor targetDescriptor) { @@ -33,7 +34,7 @@ public final class IntersectionRow { /** * Set Id's to exclude. This is for deleting non-attached detail Id's. */ - void setExcludeIds(List excludeIds, BeanDescriptor excludeDescriptor) { + void setExcludeIds(Set excludeIds, BeanDescriptor excludeDescriptor) { this.excludeIds = excludeIds; this.excludeDescriptor = excludeDescriptor; } 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 ef897b02d..e813d8511 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 @@ -1082,12 +1082,13 @@ public final class DefaultPersister implements Persister { *

*/ void deleteManyDetails(SpiTransaction t, BeanDescriptor desc, EntityBean parentBean, - BeanPropertyAssocMany many, List excludeDetailIds, DeleteMode deleteMode) { + BeanPropertyAssocMany many, Set excludeDetailIds, DeleteMode deleteMode) { if (many.cascadeInfo().isDelete()) { // cascade delete the beans in the collection BeanDescriptor targetDesc = many.targetDescriptor(); if (deleteMode.isHard() || targetDesc.isSoftDelete()) { - if (targetDesc.isDeleteByStatement()) { + if (targetDesc.isDeleteByStatement() + && (excludeDetailIds == null || excludeDetailIds.size() <= 1000)) { // TODO wait for #3176 // Just delete all the children with one statement IntersectionRow intRow = many.buildManyDeleteChildren(parentBean, excludeDetailIds); SqlUpdate sqlDelete = intRow.createDelete(server, deleteMode); 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 68c446fed..00805a892 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 @@ -9,7 +9,10 @@ import io.ebeaninternal.server.core.PersistRequestBean; import io.ebeaninternal.server.deploy.*; import javax.persistence.PersistenceException; -import java.util.*; +import java.util.Collection; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; import static io.ebeaninternal.server.persist.DmlUtil.isNullOrZero; import static java.lang.System.Logger.Level.WARNING; @@ -205,9 +208,10 @@ final class SaveManyBeans extends SaveManyBase { /** * Return the Id values of beans we know are being updated (any others are orphans) + * If there are no IDs, null is returned. */ - private List detailIds() { - final var detailIds = new ArrayList<>(); + private Set detailIds() { + final var detailIds = new HashSet<>(); for (Object detailBean : collection) { if (isMap) { detailBean = ((Map.Entry) detailBean).getValue(); @@ -222,7 +226,7 @@ final class SaveManyBeans extends SaveManyBase { } } } - return detailIds; + return detailIds.isEmpty() ? null : detailIds; } /** 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 7b3f85bfb..9c9b552df 100644 --- a/ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java +++ b/ebean-test/src/test/java/org/tests/cascade/TestOrphanCollectionReplacement.java @@ -7,56 +7,141 @@ import org.junit.jupiter.api.Test; import java.util.ArrayList; import java.util.List; +import java.util.function.Predicate; 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 { @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(); + void replaceCollection_whenOrphan_expect_forcedInsertWithStatement() { + long parentId = setup(1000); // can be handled by statement for SqlServer + List sql = doUpdate(parentId, name -> !"c0".equals(name)); + 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 "); + 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("); } - { // act - COOne fetchedParent = DB.find(COOne.class, parentId); - assert fetchedParent != null; + if (isSqlServer()) { + // statement mode + assertThat(sql).hasSize(4); + assertThat(sql.get(0)).contains("update coone_many set deleted=1 where coone_id = ? and not ( id "); + assertThat(sql.get(1)).contains(" -- bind("); + assertThat(sql.get(2)).contains("insert into coone_many (id, coone_id, name, deleted) values (?,?,?,?)"); + assertThat(sql.get(3)).contains(" -- bind("); + } + COOne fetchedUser2 = DB.find(COOne.class, parentId); + requireNonNull(fetchedUser2); + assertThat(fetchedUser2.getChildren()) + .hasSize(1000) + .extracting(COOneMany::getName) + .doesNotContain("c0")// filtered + .contains("c1") + .contains("cTest"); // added + } - COOneMany role = new COOneMany("c2"); + @Test + void replaceCollection_whenOrphan_expect_forcedInsertWithFilter() { + long parentId = setup(2500); // we cannot make a "not in" query for so many params + List sql = doUpdate(parentId, name -> !"c0".equals(name)); + 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 "); + 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("); + } - List filtered = fetchedParent.getChildren().stream().filter(r -> "c0".equals(r.getName())).collect(Collectors.toList()); + if (isSqlServer()) { + // filter mode + assertThat(sql).hasSize(5); + assertThat(sql.get(0)).contains("select t0.id from coone_many t0 where coone_id=? and t0.deleted = 0 and t0.deleted = 0; --bind"); + assertThat(sql.get(1)).contains("update coone_many set deleted=1 where id in (?)"); + assertThat(sql.get(2)).contains(" -- bind("); + assertThat(sql.get(3)).contains("insert into coone_many (id, coone_id, name, deleted) values (?,?,?,?)"); + assertThat(sql.get(4)).contains(" -- bind("); + } + COOne fetchedUser2 = DB.find(COOne.class, parentId); + requireNonNull(fetchedUser2); + assertThat(fetchedUser2.getChildren()) + .hasSize(2500) + .extracting(COOneMany::getName) + .doesNotContain("c0")// filtered + .contains("c1") + .contains("cTest"); // added + } - List updatedRoles = new ArrayList<>(); - updatedRoles.addAll(filtered); - updatedRoles.addAll(List.of(role)); - fetchedParent.setChildren(updatedRoles); + @Test + void replaceCollection_whenOrphan_expect_forcedInsertWithManyReplacement() { + long parentId = setup(5000); // we will replace 2500 beans in this step + List sql = doUpdate(parentId, name -> Integer.parseInt(name.substring(1)) >= 2500); + 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 "); + 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("); + } - 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 "); - 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("); - } + if (isSqlServer()) { + // filter mode + assertThat(sql).hasSize(7); + assertThat(sql.get(0)).contains("select t0.id from coone_many t0 where coone_id=? and t0.deleted = 0 and t0.deleted = 0; --bind"); // find all Ids + assertThat(sql.get(1)).contains("update coone_many set deleted=1 where id in (?,?,?"); + assertThat(sql.get(2)).contains(" -- bind(Array[2000]="); // update first 2000 + assertThat(sql.get(3)).contains("update coone_many set deleted=1 where id in (?,?,?"); + assertThat(sql.get(4)).contains(" -- bind(Array[500]="); // update next 500 + assertThat(sql.get(5)).contains("insert into coone_many (id, coone_id, name, deleted) values (?,?,?,?)"); + assertThat(sql.get(6)).contains(" -- bind("); } COOne fetchedUser2 = DB.find(COOne.class, parentId); requireNonNull(fetchedUser2); - assertEquals(2, fetchedUser2.getChildren().size()); + assertThat(fetchedUser2.getChildren()) + .hasSize(2501) + .extracting(COOneMany::getName) + .doesNotContain("c0")// filtered + .contains("c2500") + .contains("cTest"); // added + } + + + private static List doUpdate(long parentId, Predicate filter) { + COOne fetchedParent = DB.find(COOne.class, parentId); + assert fetchedParent != null; + + + List filtered = fetchedParent.getChildren().stream().filter(r -> filter.test(r.getName())).collect(Collectors.toList()); + + List updatedRoles = new ArrayList<>(); + updatedRoles.addAll(filtered); + updatedRoles.add(new COOneMany("cTest")); + fetchedParent.setChildren(updatedRoles); + + LoggedSql.start(); + DB.save(fetchedParent); + return LoggedSql.stop(); + } + + private static long setup(int count) { + long parentId; + // setup + List children = new ArrayList<>(); + for (int i = 0; i < count; i++) { + children.add(new COOneMany("c" + i)); + + } + + COOne parent = new COOne("p0"); + parent.setChildren(children); + + DB.save(parent); + parentId = parent.getId(); + return parentId; } }