From 7ea4311662abf15eb774ae48cf3848e0dc0881cd Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Thu, 4 Apr 2024 20:13:20 +1300 Subject: [PATCH] Fix for #3381 - Empty OneToMany when overflowing LazyLoadBatchSize and mixed queries 14.0.0 via #3295 introduced this bug. #3295 introduced a behaviour where bulk updates clear the persistence context. Now the lazy loading of a BeanCollection works with an assumption that the "parent bean" is in the persistence context (and this assumption is broken with that change for this test case). That is, the bulk update is clearing the persistence context - removing the parent bean BEFORE the lazy loading is invoked ... and that doesn't then work because the parent bean isn't in the persistence context. This fix means that when lazy loading many's, the parent beans are putIfAbsent into the persistence context. --- .../io/ebeaninternal/api/LoadManyRequest.java | 29 ++++++---- .../server/core/DefaultBeanLoader.java | 2 +- .../org/tests/cascade/TestLazyLoadMany.java | 53 +++++++++++++++++++ .../TestElementCollectionCascade.java | 2 +- .../org/tests/query/TestQueryFindIterate.java | 6 +-- .../tests/query/TestRefToLazyLoadMany.java | 25 ++++++--- .../src/test/resources/logback-test.xml | 6 +-- 7 files changed, 96 insertions(+), 27 deletions(-) create mode 100644 ebean-test/src/test/java/org/tests/cascade/TestLazyLoadMany.java diff --git a/ebean-core/src/main/java/io/ebeaninternal/api/LoadManyRequest.java b/ebean-core/src/main/java/io/ebeaninternal/api/LoadManyRequest.java index dbf1c1bab..14580bc08 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/api/LoadManyRequest.java +++ b/ebean-core/src/main/java/io/ebeaninternal/api/LoadManyRequest.java @@ -3,6 +3,7 @@ package io.ebeaninternal.api; import io.ebean.CacheMode; import io.ebean.bean.BeanCollection; import io.ebean.bean.EntityBean; +import io.ebean.bean.PersistenceContext; import io.ebeaninternal.server.core.BindPadding; import io.ebeaninternal.server.core.OrmQueryRequest; import io.ebeaninternal.server.deploy.BeanDescriptor; @@ -57,17 +58,22 @@ public final class LoadManyRequest extends LoadRequest { return loadContext.fullPath(); } - private List parentIdList(SpiEbeanServer server) { - List idList = new ArrayList<>(); - BeanPropertyAssocMany many = many(); + private List parentIdList(SpiEbeanServer server, BeanPropertyAssocMany many, PersistenceContext pc) { + final var idList = new ArrayList<>(loadContext.size()); + final var descriptor = many.descriptor(); for (int i = 0; i < loadContext.size(); i++) { - BeanCollection bc = loadContext.get(i); + final BeanCollection bc = loadContext.get(i); if (bc != null) { - if (lazy && !originIncluded && bc == originCollection) { - originIncluded = true; - } - idList.add(many.parentId(bc.owner())); + final var parent = bc.owner(); + final var parentId = descriptor.getId(parent); + idList.add(parentId); bc.setLoader(server); // don't use the load buffer again + if (lazy) { + descriptor.contextPutIfAbsent(pc, parentId, parent); + if (!originIncluded && bc == originCollection) { + originIncluded = true; + } + } } } if (originCollection != null && !originIncluded) { @@ -75,7 +81,7 @@ public final class LoadManyRequest extends LoadRequest { idList.add(many.parentId(originCollection.owner())); originCollection.setLoader(server); // don't use the load buffer again } - if (many.targetDescriptor().isPadInExpression()) { + if (descriptor.isPadInExpression()) { BindPadding.padIds(idList); } return idList; @@ -99,8 +105,9 @@ public final class LoadManyRequest extends LoadRequest { query.where().raw(extraWhere.replace("${ta}", "t0").replace("${mta}", "int_")); } query.setLazyLoadForParents(many); - many.addWhereParentIdIn(query, parentIdList(server), loadContext.isUseDocStore()); - query.setPersistenceContext(loadContext.persistenceContext()); + final var pc = loadContext.persistenceContext(); + many.addWhereParentIdIn(query, parentIdList(server, many, pc), loadContext.isUseDocStore()); + query.setPersistenceContext(pc); query.setLoadDescription(lazy ? "lazy" : "query", description()); if (lazy) { query.setLazyLoadBatchSize(loadContext.batchSize()); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/core/DefaultBeanLoader.java b/ebean-core/src/main/java/io/ebeaninternal/server/core/DefaultBeanLoader.java index cc3966455..eb24215b5 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/core/DefaultBeanLoader.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/core/DefaultBeanLoader.java @@ -63,8 +63,8 @@ final class DefaultBeanLoader { Object parentId = parentDesc.getId(parentBean); if (pc == null) { pc = new DefaultPersistenceContext(); - parentDesc.contextPut(pc, parentId, parentBean); } + parentDesc.contextPutIfAbsent(pc, parentId, parentBean); boolean useManyIdCache = beanCollection != null && parentDesc.isManyPropCaching() && many.isUseCache(); if (useManyIdCache) { Boolean readOnly = null; diff --git a/ebean-test/src/test/java/org/tests/cascade/TestLazyLoadMany.java b/ebean-test/src/test/java/org/tests/cascade/TestLazyLoadMany.java new file mode 100644 index 000000000..74565feee --- /dev/null +++ b/ebean-test/src/test/java/org/tests/cascade/TestLazyLoadMany.java @@ -0,0 +1,53 @@ +package org.tests.cascade; + +import io.ebean.DB; +import io.ebean.Transaction; +import io.ebean.xtest.BaseTestCase; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +class TestLazyLoadMany extends BaseTestCase { + + COOne create(String parentName, int childCount) { + COOne m0 = new COOne(parentName); + for (int i = 0; i < childCount; i++) { + m0.getChildren().add(new COOneMany(parentName+"_"+i)); + } + return m0; + } + @Test + void loadAfterPCCleared() { + + var m0 = create("tll-m0", 2); + var m1 = create("tll-m1", 4); + var m2 = create("tll-m2", 5); + + DB.saveAll(m0, m1, m2); + + try (Transaction transaction = DB.beginTransaction()) { + List children = DB.find(COOne.class) + .setLazyLoadBatchSize(2) + .where().startsWith("name", "tll-m") + .findList(); + + assertThat(children).hasSize(3); + assertThat(children.get(0).getChildren()).describedAs("invoke lazy loading on children").hasSize(2); + + DB.sqlUpdate("update coone set name = ? where id = ?") + .setParameters("new name", children.get(0).getId()) + .executeNow(); // clears the persistence context in 14.0.0+ due to #3295 #3301 + + // the children here were already lazy loaded + assertThat(children.get(1).getChildren()).hasSize(4); + // this invokes the lazy loading of children AFTER the persistence context was cleared + assertThat(children.get(2).getChildren()).hasSize(5); + + transaction.rollback(); + } finally { + DB.deleteAll(List.of(m0, m1, m2)); + } + } +} diff --git a/ebean-test/src/test/java/org/tests/model/elementcollection/TestElementCollectionCascade.java b/ebean-test/src/test/java/org/tests/model/elementcollection/TestElementCollectionCascade.java index 97c31d530..283ad6b4d 100644 --- a/ebean-test/src/test/java/org/tests/model/elementcollection/TestElementCollectionCascade.java +++ b/ebean-test/src/test/java/org/tests/model/elementcollection/TestElementCollectionCascade.java @@ -46,7 +46,7 @@ public class TestElementCollectionCascade extends BaseTestCase { assertThat(sql.get(0)).contains("select t0.id, t0.name, t0.version from ecsm_parent t0 where t0.id = ?"); if (!isPostgresCompatible()) { assertThat(sql.get(1)).contains("select t0.ecsm_parent_id, t0.one_id, t0.name, t0.version from ecsm_child t0 where (t0.ecsm_parent_id) in (?)"); - assertThat(sql.get(2)).contains("select t0.host_id, t0.value from ecsm_values t0 where (t0.host_id) in (?,?)"); + assertThat(sql.get(2)).contains("select t0.host_id, t0.value from ecsm_values t0 where (t0.host_id) in (?,?,?,?,?)"); } Set vals0 = childVals.get("c0"); diff --git a/ebean-test/src/test/java/org/tests/query/TestQueryFindIterate.java b/ebean-test/src/test/java/org/tests/query/TestQueryFindIterate.java index cd6bce065..d31b8d896 100644 --- a/ebean-test/src/test/java/org/tests/query/TestQueryFindIterate.java +++ b/ebean-test/src/test/java/org/tests/query/TestQueryFindIterate.java @@ -172,9 +172,9 @@ public class TestQueryFindIterate extends BaseTestCase { List loggedSql = LoggedSql.stop(); - assertEquals(2, loggedSql.size()); - assertThat(trimSql(loggedSql.get(0), 7).contains("select t0.id, t0.status, t0.order_date, t1.id, t1.name, t2.id, t2.order_qty, t2.ship_qty")); - assertThat(trimSql(loggedSql.get(1), 7).contains("select t0.order_id, t0.id, t0.ship_time, t0.cretime, t0.updtime, t0.version, t0.order_id from or_order_ship")); + assertThat(loggedSql).hasSize(2); + assertThat(trimSql(loggedSql.get(0), 7)).contains("select t0.id, t0.status, t0.order_date, t1.id, t1.name, t2.id, t2.order_qty, t2.ship_qty"); + assertThat(trimSql(loggedSql.get(1), 7)).contains("select t0.order_id, t0.id, t0.ship_time, t0.cretime, t0.updtime, t0.version, t0.order_id from or_order_ship"); } @ForPlatform(Platform.H2) diff --git a/ebean-test/src/test/java/org/tests/query/TestRefToLazyLoadMany.java b/ebean-test/src/test/java/org/tests/query/TestRefToLazyLoadMany.java index 0739c2801..f249e6145 100644 --- a/ebean-test/src/test/java/org/tests/query/TestRefToLazyLoadMany.java +++ b/ebean-test/src/test/java/org/tests/query/TestRefToLazyLoadMany.java @@ -1,7 +1,9 @@ package org.tests.query; +import io.ebean.Transaction; import io.ebean.xtest.BaseTestCase; import io.ebean.DB; +import io.ebeaninternal.api.SpiTransaction; import org.junit.jupiter.api.Test; import org.tests.model.basic.Contact; import org.tests.model.basic.Customer; @@ -9,6 +11,7 @@ import org.tests.model.basic.ResetBasicData; import java.util.List; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; public class TestRefToLazyLoadMany extends BaseTestCase { @@ -26,17 +29,23 @@ public class TestRefToLazyLoadMany extends BaseTestCase { assertEquals(3, DB.beanState(c).loadedProps().size()); // now lazy load the contacts - contacts2.size(); + int expectedSize = contacts2.size(); - Customer c2 = DB.reference(Customer.class, c.getId()); + try (Transaction transaction = DB.beginTransaction()) { + Customer c2 = DB.reference(Customer.class, c.getId()); - // we only "loaded" the contacts BeanList and not all of c2 - List contacts = c2.getContacts(); - // Set loadedProps = DB.beanState(c2).getLoadedProps(); - // assertEquals(1, loadedProps.size()); + SpiTransaction spiTxn = (SpiTransaction)transaction; + spiTxn.persistenceContext().clear(); - // now lazy load the contacts - contacts.size(); + // we only "loaded" the contacts BeanList and not all of c2 + List contacts = c2.getContacts(); + // Set loadedProps = DB.beanState(c2).getLoadedProps(); + // assertEquals(1, loadedProps.size()); + // now lazy load the contacts + int lazyLoadedSize = contacts.size(); + + assertThat(lazyLoadedSize).isEqualTo(expectedSize); + } } } diff --git a/ebean-test/src/test/resources/logback-test.xml b/ebean-test/src/test/resources/logback-test.xml index 36c235e0c..1612755c9 100644 --- a/ebean-test/src/test/resources/logback-test.xml +++ b/ebean-test/src/test/resources/logback-test.xml @@ -82,9 +82,9 @@ - - - + + +