From c5eb6fa034978dd8aa582d062d3f92208c9c2cbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20P=C3=B6hler=20=28JPo=29?= Date: Wed, 23 Sep 2020 15:19:59 +0200 Subject: [PATCH 1/2] ADD: failing testcase for wrong bean caching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jonas Pöhler (JPo) --- .../cache/TestBeanCacheContactLazyLoad.java | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 src/test/java/org/tests/cache/TestBeanCacheContactLazyLoad.java diff --git a/src/test/java/org/tests/cache/TestBeanCacheContactLazyLoad.java b/src/test/java/org/tests/cache/TestBeanCacheContactLazyLoad.java new file mode 100644 index 000000000..9eb438032 --- /dev/null +++ b/src/test/java/org/tests/cache/TestBeanCacheContactLazyLoad.java @@ -0,0 +1,68 @@ +package org.tests.cache; + +import io.ebean.BaseTestCase; +import io.ebean.DB; +import io.ebeantest.LoggedSql; +import org.junit.Test; +import org.tests.model.basic.Contact; +import org.tests.model.basic.Customer; + +import java.sql.Date; +import java.time.LocalDate; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Test class testing a wrong behaviour of the bean cache. + */ +public class TestBeanCacheContactLazyLoad extends BaseTestCase { + + /** + * This test shows a wrong behaviour of the bean cache up to at least Ebean 12.4.*: + * + */ + @Test + public void testBeanCacheWithLazyLoading() { + final Customer customer = new Customer(); + customer.setName("Customer"); + customer.setAnniversary(Date.valueOf(LocalDate.of(2010, 1, 1))); + DB.save(customer); + + final Contact contact = new Contact(); + contact.setFirstName("Tim"); + contact.setLastName("Button"); + contact.setPhone("1234567890"); + contact.setMobile("4567890123"); + contact.setEmail("tim@button.com"); + contact.setCustomer(customer); + + DB.save(contact); + + // Only get two properties, so we have to lazy-load later + final Contact contactDb = DB.find(Contact.class).where().eq("email", "tim@button.com").select("email,lastName").findOne(); + assertThat(contactDb).isNotNull(); + LoggedSql.start(); + contactDb.setLastName("Buttonnnn"); + List sql = LoggedSql.collect(); + assertThat(sql).isEmpty(); // setter did not trigger lazy load + + // trigger lazy load + assertThat(contactDb.getPhone()).isEqualTo("1234567890"); + sql = LoggedSql.collect(); + assertThat(sql).isNotEmpty(); // Lazy-load took place + + final Contact contactDb2 = DB.find(Contact.class).where().eq("email", "tim@button.com").select("email,lastName").findOne(); + sql = LoggedSql.stop(); + assertThat(sql).isEmpty(); // We expect that the bean was loaded from cache + assertThat(contactDb2).isNotNull(); + assertThat(contactDb2.getLastName()).isEqualTo("Button"); + } + +} From de734665cd4e979050b6776d7241792b7367c8fe Mon Sep 17 00:00:00 2001 From: rob bygrave Date: Thu, 24 Sep 2020 22:14:48 +1200 Subject: [PATCH 2/2] #2061 - Fix for L2 bean cache loaded using lazy loading on mutated partially loaded bean The fix is when the property is dirty (mutated) put the original value into the cache entry. --- .../server/cache/CachedBeanDataFromBean.java | 11 ++-- .../server/deploy/BeanProperty.java | 14 +++- .../server/deploy/BeanPropertyAssocOne.java | 13 +++- .../server/cache/CacheBeanDataTest.java | 2 +- .../cache/CachedBeanDataFromBeanTest.java | 66 ++++++++++++++++++- 5 files changed, 93 insertions(+), 13 deletions(-) diff --git a/src/main/java/io/ebeaninternal/server/cache/CachedBeanDataFromBean.java b/src/main/java/io/ebeaninternal/server/cache/CachedBeanDataFromBean.java index c91fee62d..32ac2ce9a 100644 --- a/src/main/java/io/ebeaninternal/server/cache/CachedBeanDataFromBean.java +++ b/src/main/java/io/ebeaninternal/server/cache/CachedBeanDataFromBean.java @@ -11,11 +11,9 @@ import java.util.Map; public class CachedBeanDataFromBean { - public static CachedBeanData extract(BeanDescriptor desc, EntityBean bean) { EntityBeanIntercept ebi = bean._ebean_getIntercept(); - Map data = new LinkedHashMap<>(); BeanProperty idProperty = desc.getIdProperty(); @@ -25,11 +23,13 @@ public class CachedBeanDataFromBean { data.put(idProperty.getName(), idProperty.getCacheDataValue(bean)); } } - BeanProperty[] props = desc.propertiesNonMany(); // extract all the non-many properties - for (BeanProperty prop : props) { - if (ebi.isLoadedProperty(prop.getPropertyIndex())) { + final boolean dirty = ebi.isDirty(); + for (BeanProperty prop : desc.propertiesNonMany()) { + if (dirty && ebi.isDirtyProperty(prop.getPropertyIndex())) { + data.put(prop.getName(), prop.getCacheDataValueOrig(ebi)); + } else if (ebi.isLoadedProperty(prop.getPropertyIndex())) { data.put(prop.getName(), prop.getCacheDataValue(bean)); } } @@ -72,5 +72,4 @@ public class CachedBeanDataFromBean { return sharableBean; } - } diff --git a/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java b/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java index 11d11d67e..31b3bc810 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java +++ b/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java @@ -3,6 +3,7 @@ package io.ebeaninternal.server.deploy; import com.fasterxml.jackson.core.JsonToken; import io.ebean.ValuePair; import io.ebean.bean.EntityBean; +import io.ebean.bean.EntityBeanIntercept; import io.ebean.bean.PersistenceContext; import io.ebean.config.EncryptKey; import io.ebean.config.dbplatform.DbEncryptFunction; @@ -10,7 +11,6 @@ import io.ebean.config.dbplatform.DbPlatformType; import io.ebean.plugin.Property; import io.ebean.text.StringParser; import io.ebean.util.SplitName; -import io.ebean.util.StringHelper; import io.ebeaninternal.api.SpiExpressionRequest; import io.ebeaninternal.api.SpiQuery; import io.ebeaninternal.api.json.SpiJsonReader; @@ -794,7 +794,17 @@ public class BeanProperty implements ElPropertyValue, Property, STreeProperty { *

*/ public Object getCacheDataValue(EntityBean bean) { - Object value = getValue(bean); + return cacheDataConvert(getValue(bean)); + } + + /** + * Return the bean cache value for this property using original values. + */ + public Object getCacheDataValueOrig(EntityBeanIntercept ebi) { + return cacheDataConvert(ebi.getOrigValue(propertyIndex)); + } + + private Object cacheDataConvert(Object value) { if (value == null || scalarType.isBinaryType()) { return value; } else { diff --git a/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocOne.java b/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocOne.java index d33dd95c2..59404d6b1 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocOne.java +++ b/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocOne.java @@ -5,6 +5,7 @@ import io.ebean.SqlUpdate; import io.ebean.Transaction; import io.ebean.ValuePair; import io.ebean.bean.EntityBean; +import io.ebean.bean.EntityBeanIntercept; import io.ebean.bean.PersistenceContext; import io.ebean.util.SplitName; import io.ebeaninternal.api.SpiEbeanServer; @@ -426,9 +427,19 @@ public class BeanPropertyAssocOne extends BeanPropertyAssoc implements STr return getPropertyType(); } + /** + * Return the bean cache value for this property using original values. + */ + public Object getCacheDataValueOrig(EntityBeanIntercept ebi) { + return cacheDataConvert(ebi.getOrigValue(propertyIndex)); + } + @Override public Object getCacheDataValue(EntityBean bean) { - Object ap = getValue(bean); + return cacheDataConvert(getValue(bean)); + } + + private Object cacheDataConvert(Object ap) { if (ap == null) { return null; } diff --git a/src/test/java/io/ebeaninternal/server/cache/CacheBeanDataTest.java b/src/test/java/io/ebeaninternal/server/cache/CacheBeanDataTest.java index 2ba1b9747..957515f80 100644 --- a/src/test/java/io/ebeaninternal/server/cache/CacheBeanDataTest.java +++ b/src/test/java/io/ebeaninternal/server/cache/CacheBeanDataTest.java @@ -43,7 +43,7 @@ public class CacheBeanDataTest extends BaseTestCase { billingAddress.setLine1("92 Someplace Else"); c.setBillingAddress(billingAddress); - ((EntityBean) c)._ebean_getIntercept().setNewBeanForUpdate(); + ((EntityBean) c)._ebean_getIntercept().setLoaded(); CachedBeanData cacheData = CachedBeanDataFromBean.extract(desc, (EntityBean) c); diff --git a/src/test/java/io/ebeaninternal/server/cache/CachedBeanDataFromBeanTest.java b/src/test/java/io/ebeaninternal/server/cache/CachedBeanDataFromBeanTest.java index 1490bead6..323fcc2e0 100644 --- a/src/test/java/io/ebeaninternal/server/cache/CachedBeanDataFromBeanTest.java +++ b/src/test/java/io/ebeaninternal/server/cache/CachedBeanDataFromBeanTest.java @@ -5,21 +5,24 @@ import io.ebean.bean.EntityBean; import io.ebeaninternal.api.SpiEbeanServer; import io.ebeaninternal.server.deploy.BeanDescriptor; import io.ebeaninternal.server.transaction.DefaultPersistenceContext; +import org.junit.Test; import org.tests.model.basic.Address; import org.tests.model.basic.Car; +import org.tests.model.basic.Contact; import org.tests.model.basic.Customer; -import org.junit.Test; import java.sql.Date; +import java.util.Map; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; public class CachedBeanDataFromBeanTest extends BaseTestCase { - SpiEbeanServer server = spiEbeanServer(); + private final SpiEbeanServer server = spiEbeanServer(); @Test - public void extract() throws Exception { + public void extract() { BeanDescriptor desc = server.getBeanDescriptor(Customer.class); @@ -64,4 +67,61 @@ public class CachedBeanDataFromBeanTest extends BaseTestCase { assertEquals(newCar.getDriver(), car.getDriver()); assertEquals(newCar.getNotes(), car.getNotes()); } + + @SuppressWarnings("unchecked") + @Test + public void dirtyScalar_expect_originalValueUsed() { + + Contact contact = new Contact(); + contact.setId(42); + contact.setLastName("Bygrave"); + contact.setFirstName("Foo"); + contact.setEmail("rob@email.com"); + + EntityBean entityBean = (EntityBean)contact; + entityBean._ebean_getIntercept().setLoaded(); + + // mutate, dirty + contact.setLastName("Banana"); + + final BeanDescriptor desc = getBeanDescriptor(Contact.class); + CachedBeanData cacheData = CachedBeanDataFromBean.extract(desc, entityBean); + + final Map data = cacheData.getData(); + assertThat(data.get("id")).isEqualTo("42"); + assertThat(data.get("lastName")).isEqualTo("Bygrave"); // ORIGINAL VALUE + assertThat(data.get("firstName")).isEqualTo(contact.getFirstName()); + assertThat(data.get("email")).isEqualTo(contact.getEmail()); + } + + @Test + public void dirtyManyToOne_expect_originalValueUsed() { + + Customer customer = new Customer(); + customer.setId(99); + + Contact contact = new Contact(); + contact.setFirstName("Foo"); + contact.setLastName("Bygrave"); + contact.setEmail("rob@email.com"); + contact.setCustomer(customer); + + EntityBean entityBean = (EntityBean)contact; + entityBean._ebean_getIntercept().setLoaded(); + + // mutate, dirty + Customer customer2 = new Customer(); + customer2.setId(108); + contact.setCustomer(customer2); + contact.setLastName("Banana"); + + final BeanDescriptor desc = getBeanDescriptor(Contact.class); + CachedBeanData cacheData = CachedBeanDataFromBean.extract(desc, entityBean); + + final Map data = cacheData.getData(); + assertThat(data.get("lastName")).isEqualTo("Bygrave"); // Original value + assertThat(data.get("customer")).isEqualTo("99"); // Original value + assertThat(data.get("firstName")).isEqualTo(contact.getFirstName()); + assertThat(data.get("email")).isEqualTo(contact.getEmail()); + } }