diff --git a/ebean-core/src/main/java/io/ebeaninternal/api/TransactionEvent.java b/ebean-core/src/main/java/io/ebeaninternal/api/TransactionEvent.java index c1fc45f304..a1b88fc959 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/api/TransactionEvent.java +++ b/ebean-core/src/main/java/io/ebeaninternal/api/TransactionEvent.java @@ -150,4 +150,45 @@ public CacheChangeSet obtainCacheChangeSet() { return changeSet; } + public void merge(TransactionEvent other) { + if (other == null) { + return; + } + + // Merge table events + if (other.eventTables != null) { + if (this.eventTables == null) { + this.eventTables = other.eventTables; + } else { + this.eventTables.add(other.eventTables); + } + } + + // Merge listeners + if (other.listenerNotify != null) { + if (this.listenerNotify == null) { + this.listenerNotify = other.listenerNotify; + } else { + this.listenerNotify.addAll(other.listenerNotify); + } + } + + // Merge delete-by-id + if (other.deleteByIdMap != null) { + if (this.deleteByIdMap == null) { + this.deleteByIdMap = other.deleteByIdMap; + } else { + this.deleteByIdMap.merge(other.deleteByIdMap); + } + } + + // Merge cache changes + if (other.changeSet != null) { + if (this.changeSet == null) { + this.changeSet = other.changeSet; + } else { + this.changeSet.merge(other.changeSet); + } + } + } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/cache/CacheChangeBeanRemove.java b/ebean-core/src/main/java/io/ebeaninternal/server/cache/CacheChangeBeanRemove.java index 7c2086c550..feae0f8a42 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/cache/CacheChangeBeanRemove.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/cache/CacheChangeBeanRemove.java @@ -42,4 +42,8 @@ public void addIds(Collection moreIds) { public void addId(Object id) { ids.add(id); } + + void merge(CacheChangeBeanRemove other) { + this.ids.addAll(other.ids); + } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/cache/CacheChangeSet.java b/ebean-core/src/main/java/io/ebeaninternal/server/cache/CacheChangeSet.java index dc598539f4..61f4ac6a2e 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/cache/CacheChangeSet.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/cache/CacheChangeSet.java @@ -184,6 +184,32 @@ private ManyChange many(BeanDescriptor desc, String manyProperty) { return manyChangeMap.computeIfAbsent(key, ManyChange::new); } + public void merge(CacheChangeSet other) { + if (other == null) { + return; + } + + this.entries.addAll(other.entries); + this.touchedTables.addAll(other.touchedTables); + this.queryCaches.addAll(other.queryCaches); + this.beanCaches.addAll(other.beanCaches); + + other.beanRemoveMap.forEach((desc, remove) -> + this.beanRemoveMap.merge(desc, remove, (a, b) -> { + a.merge(b); + return a; + }) + ); + + other.manyChangeMap.forEach((key, change) -> + this.manyChangeMap.merge(key, change, (a, b) -> { + a.merge(b); + return a; + }) + ); + } + + /** * Changes for a specific many property. */ @@ -215,6 +241,32 @@ void addRemove(String parentKey) { } } + void merge(ManyChange other) { + // clear dominates everything + if (other.clear) { + this.clear = true; + this.removes.clear(); + this.puts.clear(); + return; + } + + if (this.clear) { + // already clearing, ignore finer changes + return; + } + + // merge puts (put overrides remove) + this.puts.putAll(other.puts); + + // merge removes, but do not remove something we just put + for (String key : other.removes) { + if (!this.puts.containsKey(key)) { + this.removes.add(key); + } + } + } + + /** * Put entry for the given parentId. */ diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/transaction/DeleteByIdMap.java b/ebean-core/src/main/java/io/ebeaninternal/server/transaction/DeleteByIdMap.java index 06b78861e6..201dde344f 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/transaction/DeleteByIdMap.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/transaction/DeleteByIdMap.java @@ -91,4 +91,16 @@ void addDocStoreUpdates(DocStoreUpdates docStoreUpdates, DocStoreMode txnIndexMo } } } + + public void merge(DeleteByIdMap other) { + if (other == null || other.beanMap.isEmpty()) { + return; + } + other.beanMap.forEach((key, value) -> + beanMap.merge(key, value, (a, b) -> { + b.getIds().forEach(id -> a.addId(PersistRequest.Type.UPDATE, id)); + return a; + }) + ); + } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/transaction/SavepointTransaction.java b/ebean-core/src/main/java/io/ebeaninternal/server/transaction/SavepointTransaction.java index cb0ff45b47..78f480a821 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/transaction/SavepointTransaction.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/transaction/SavepointTransaction.java @@ -91,13 +91,25 @@ private void commitSavepoint() { try { connection.releaseSavepoint(savepoint); state = STATE_COMMITTED; - manager.notifyOfCommit(this); + mergeIntoParent(); transaction.logTxn(spPrefix + "commit"); } catch (SQLException e) { throw new PersistenceException("Error trying to commit/release Savepoint", e); } } + private void mergeIntoParent() { + if (event == null) { + return; + } + + TransactionEvent parentEvent = transaction.event(); + parentEvent.merge(event); + // Prevent accidental reuse + event = null; + } + + private void rollbackSavepoint(Throwable cause) throws PersistenceException { try { connection.rollback(savepoint); diff --git a/ebean-test/src/test/java/org/tests/cache/TestSavepointCacheConsistency.java b/ebean-test/src/test/java/org/tests/cache/TestSavepointCacheConsistency.java new file mode 100644 index 0000000000..d0b97c69f6 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/cache/TestSavepointCacheConsistency.java @@ -0,0 +1,190 @@ +package org.tests.cache; + +import io.ebean.DB; +import io.ebean.Transaction; +import io.ebean.cache.ServerCache; +import io.ebean.cache.ServerCacheStatistics; +import io.ebean.test.LoggedSql; +import io.ebean.xtest.BaseTestCase; +import io.ebean.xtest.IgnorePlatform; +import io.ebean.annotation.Platform; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.tests.model.basic.OCachedBean; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests that savepoint (nested) transaction commits do not prematurely apply + * cache changes before the parent transaction commits. + *

+ * Before the fix, SavepointTransaction.commit() called manager.notifyOfCommit() + * immediately, which would invalidate/update the L2 cache even if the parent + * transaction later rolled back — leaving the cache in a stale/polluted state. + */ +public class TestSavepointCacheConsistency extends BaseTestCase { + + private final ServerCache beanCache = DB.getDefault().cacheManager().beanCache(OCachedBean.class); + + @BeforeEach + void clearCache() { + beanCache.clear(); + } + + /** + * The bug scenario: savepoint commits, parent rolls back. + * Cache must remain valid (not evicted) because the DB change was rolled back. + * Before the fix, the premature notifyOfCommit on savepoint commit would + * evict the cache entry, causing an unnecessary (and potentially wrong) cache miss. + */ + @IgnorePlatform({Platform.MYSQL, Platform.SQLSERVER, Platform.HANA, Platform.ORACLE}) + @Test + void savepointCommit_parentRollback_cacheNotPolluted() { + OCachedBean bean = new OCachedBean(); + bean.setName("original"); + DB.save(bean); + + // Warm the cache with the original value + DB.find(OCachedBean.class, bean.getId()); + + beanCache.statistics(true); // reset hit/miss counters + + try (Transaction outer = DB.beginTransaction()) { + outer.setNestedUseSavepoint(); + + try (Transaction nested = DB.beginTransaction()) { + bean.setName("rolled-back-change"); + DB.save(bean); + nested.commit(); // savepoint released — must NOT flush cache yet + } + + outer.rollback(); // DB change undone; cache must stay untouched + } + + // Cache must still hold the original entry — no SQL should be needed + LoggedSql.start(); + OCachedBean found = DB.find(OCachedBean.class, bean.getId()); + List sql = LoggedSql.stop(); + + assertThat(found.getName()).isEqualTo("original"); + assertThat(sql).as("cache should still be valid after parent rollback — no DB query expected").isEmpty(); + + ServerCacheStatistics stats = beanCache.statistics(true); + assertThat(stats.getHitCount()).as("bean should be served from cache").isEqualTo(1); + assertThat(stats.getMissCount()).isZero(); + + DB.delete(OCachedBean.class, bean.getId()); + } + + /** + * Happy path: savepoint commits, parent commits. + * Cache must reflect the committed change (invalidated or updated). + * The fix must not suppress cache updates for the successful case. + */ + @IgnorePlatform({Platform.MYSQL, Platform.SQLSERVER, Platform.HANA, Platform.ORACLE}) + @Test + void savepointCommit_parentCommit_cacheProperlyInvalidated() { + OCachedBean bean = new OCachedBean(); + bean.setName("before"); + DB.save(bean); + + // Warm the cache + DB.find(OCachedBean.class, bean.getId()); + + try (Transaction outer = DB.beginTransaction()) { + outer.setNestedUseSavepoint(); + + try (Transaction nested = DB.beginTransaction()) { + bean.setName("after"); + DB.save(bean); + nested.commit(); + } + + outer.commit(); // parent commits — cache changes should now be applied + } + + // The bean must reflect the committed update + OCachedBean found = DB.find(OCachedBean.class, bean.getId()); + assertThat(found.getName()).isEqualTo("after"); + + DB.delete(OCachedBean.class, bean.getId()); + } + + /** + * Multiple savepoints in the same parent: only the changes whose savepoints + * committed are reflected when the parent commits. + */ + @IgnorePlatform({Platform.MYSQL, Platform.SQLSERVER, Platform.HANA, Platform.ORACLE}) + @Test + void multipleSavepoints_mixedCommitRollback_parentCommit_onlyCommittedChangesVisible() { + OCachedBean bean = new OCachedBean(); + bean.setName("start"); + DB.save(bean); + + DB.find(OCachedBean.class, bean.getId()); // warm cache + + try (Transaction outer = DB.beginTransaction()) { + outer.setNestedUseSavepoint(); + + // First nested: commits + try (Transaction nested1 = DB.beginTransaction()) { + bean.setName("after-nested1"); + DB.save(bean); + nested1.commit(); + } + + // Second nested: rolls back — its name change must not persist + try (Transaction nested2 = DB.beginTransaction()) { + bean.setName("after-nested2-rolled-back"); + DB.save(bean); + nested2.rollback(); + } + + outer.commit(); + } + + OCachedBean found = DB.find(OCachedBean.class, bean.getId()); + assertThat(found.getName()).isEqualTo("after-nested1"); + + DB.delete(OCachedBean.class, bean.getId()); + } + + /** + * Savepoint commits then parent also rolls back — same cache-pollution guard, + * but verified via LoggedSql rather than statistics, to confirm the cache + * entry was never evicted. + */ + @IgnorePlatform({Platform.MYSQL, Platform.SQLSERVER, Platform.HANA, Platform.ORACLE}) + @Test + void savepointCommit_parentRollback_cacheServedWithoutDbRoundtrip() { + OCachedBean bean = new OCachedBean(); + bean.setName("pristine"); + DB.save(bean); + + // Warm the cache + DB.find(OCachedBean.class, bean.getId()); + + try (Transaction outer = DB.beginTransaction()) { + outer.setNestedUseSavepoint(); + + try (Transaction nested = DB.beginTransaction()) { + bean.setName("discarded"); + DB.save(bean); + nested.commit(); + } + + outer.rollback(); + } + + LoggedSql.start(); + OCachedBean result = DB.find(OCachedBean.class, bean.getId()); + List sql = LoggedSql.stop(); + + assertThat(result.getName()).isEqualTo("pristine"); + assertThat(sql).as("no SQL expected — cache entry must survive parent rollback").isEmpty(); + + DB.delete(OCachedBean.class, bean.getId()); + } +}