From dcd5eda91d2b16ea687830630611751d693e56c0 Mon Sep 17 00:00:00 2001 From: "robin.bygrave" Date: Tue, 23 Jun 2026 22:38:37 +1200 Subject: [PATCH] Two related tidy-ups to query-cache dependent-table handling. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a replacement for #3150 **1. Read `dependentTables` from the query plan (single source of truth)** `OrmQueryRequest` accumulated query-cache `dependentTables` into a field via `addDependentTables(...)`, fed from per-CQuery `dependentTables()` helpers. The `CQueryPlan` already owns this set, so the copying is redundant. - `putToQueryCache(...)` now reads `dependentTables` directly from the request's `CQueryPlan` at cache-put time. - Removed the `OrmQueryRequest.dependentTables` field and `addDependentTables(Set)`. - Removed the now-unused `dependentTables()` helpers from `CQuery`, `CQueryRowCount` and `CQueryFetchSingleAttribute` (and their `Set` imports). This is behaviour-preserving: each CQuery is built with exactly the plan stored under `request.queryPlanKey`, so `request.queryPlan().dependentTables()` is the same set the per-CQuery helpers returned. **2. Fix a query-plan trim race that could null the plan at put time** A freshly built `CQueryPlan` was stored in the plan cache with `lastQueryTime == 0`, making it immediately eligible for `trimQueryPlans` (runs every 60s; TTL default 300s) during its *first* execution. If the trim fired mid-execution, `request.queryPlan()` could return `null` at put time — and a cache entry with `null` dependentTables is never invalidated by table modifications (`TableModState.isValid`), i.e. latent stale data. - `CQueryPlanStats` now initialises `lastQueryTime` to construction time, so a new plan is only trim-eligible after a genuine TTL idle. `trimQueryPlans` is the sole consumer of `lastQueryTime()`, so this is safe. - `putToQueryCache(...)` skips the put when the plan is `null` (fail safe rather than caching an un-invalidatable entry) for the residual pathological case (a single query running past the TTL on first execution). ### Tests - Added `testFindSingleAttributeOnDependent` and `testFindListOnDependent` to `TestQueryCacheTableDependency`, asserting cache-hit then dependent-table invalidation for the single-attribute and findMany paths (the count path was already covered). - All cache/query tests pass: 69 in `ebean-test` `org.tests.cache.**`, 929 in `ebean-core` cache + query packages. --- .../server/core/OrmQueryRequest.java | 16 +--- .../io/ebeaninternal/server/query/CQuery.java | 4 - .../server/query/CQueryEngine.java | 5 - .../query/CQueryFetchSingleAttribute.java | 5 - .../server/query/CQueryPlanStats.java | 2 + .../server/query/CQueryRowCount.java | 5 - .../cache/TestQueryCacheTableDependency.java | 96 +++++++++++++++++++ 7 files changed, 103 insertions(+), 30 deletions(-) diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java b/ebean-core/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java index 55cd38f5f1..eaf01bb464 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java @@ -42,7 +42,6 @@ public final class OrmQueryRequest extends BeanRequest implements SpiOrmQuery private SpiQuerySecondary secondaryQueries; private List cacheBeans; private boolean inlineCountDistinct; - private Set dependentTables; private SpiQueryManyJoin manyJoin; public OrmQueryRequest(SpiEbeanServer server, OrmQueryEngine queryEngine, SpiQuery query, SpiTransaction t) { @@ -667,7 +666,11 @@ private boolean readAuditQueryType() { } public void putToQueryCache(Object result) { - beanDescriptor.queryCachePut(cacheKey, new QueryCacheEntry(result, dependentTables, transaction.startTime())); + CQueryPlan plan = queryPlan(); + if (plan != null) { + // only cache when we have the plan's dependent tables + beanDescriptor.queryCachePut(cacheKey, new QueryCacheEntry(result, plan.dependentTables(), transaction.startTime())); + } } /** @@ -737,15 +740,6 @@ public boolean isInlineCountDistinct() { return inlineCountDistinct; } - public void addDependentTables(Set tables) { - if (tables != null && !tables.isEmpty()) { - if (dependentTables == null) { - dependentTables = new LinkedHashSet<>(); - } - dependentTables.addAll(tables); - } - } - /** * Return true if no MaxRows or use LIMIT in SQL update. */ diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java index 9679b6465a..d64a0e2ba2 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java @@ -768,8 +768,4 @@ PreparedStatement pstmt() { public void handleLoadError(String fullName, Exception e) { query.handleLoadError(fullName, e); } - - public Set dependentTables() { - return queryPlan.dependentTables(); - } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java index 6ede229ea9..1a72466b65 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java @@ -105,7 +105,6 @@ private > A findAttributeCollection(OrmQueryRequest r request.transaction().logSummary(rcQuery.summary()); } if (request.isQueryCachePut()) { - request.addDependentTables(rcQuery.dependentTables()); if (collection instanceof List) { collection = (A) Collections.unmodifiableList((List) collection); request.putToQueryCache(collection); @@ -163,7 +162,6 @@ public int findCount(OrmQueryRequest request) { request.transaction().logSummary(rcQuery.summary()); } if (request.isQueryCachePut()) { - request.addDependentTables(rcQuery.dependentTables()); request.putToQueryCache(count); } return count; @@ -355,9 +353,6 @@ BeanCollection findMany(OrmQueryRequest request) { } request.executeSecondaryQueries(false); request.populateFromImmutableCache(); - if (request.isQueryCachePut()) { - request.addDependentTables(cquery.dependentTables()); - } request.unmodifiableFreeze(beanCollection); return beanCollection; diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryFetchSingleAttribute.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryFetchSingleAttribute.java index 34c946cea5..83b20f2cc1 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryFetchSingleAttribute.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryFetchSingleAttribute.java @@ -16,7 +16,6 @@ import java.sql.PreparedStatement; import java.sql.SQLException; import java.util.Collection; -import java.util.Set; import java.util.concurrent.locks.ReentrantLock; import static java.lang.System.Logger.Level.ERROR; @@ -168,10 +167,6 @@ public void profile() { .addQueryEvent(query.profileEventId(), profileOffset, desc.name(), rowCount, query.profileId(), queryPlan.hash(), query.getGeneratedSql()); } - Set dependentTables() { - return queryPlan.dependentTables(); - } - @Override public void cancel() { lock.lock(); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPlanStats.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPlanStats.java index 8c911aaac7..85c07f4e78 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPlanStats.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPlanStats.java @@ -22,6 +22,8 @@ public final class CQueryPlanStats { CQueryPlanStats(CQueryPlan queryPlan) { this.queryPlan = queryPlan; this.timedMetric = queryPlan.createTimedMetric(); + // treat newly built plan as recently used to avoid trim + this.lastQueryTime = System.currentTimeMillis(); } /** diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryRowCount.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryRowCount.java index f0bf608354..0d79f46c54 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryRowCount.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryRowCount.java @@ -13,7 +13,6 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; -import java.util.Set; import java.util.concurrent.locks.ReentrantLock; /** @@ -140,10 +139,6 @@ public void profile() { .addQueryEvent(query.profileEventId(), profileOffset, desc.name(), rowCount, query.profileId(), queryPlan.hash(), query.getGeneratedSql()); } - Set dependentTables() { - return queryPlan.dependentTables(); - } - @Override public void cancel() { lock.lock(); diff --git a/ebean-test/src/test/java/org/tests/cache/TestQueryCacheTableDependency.java b/ebean-test/src/test/java/org/tests/cache/TestQueryCacheTableDependency.java index 714274bbf7..a9cbdec091 100644 --- a/ebean-test/src/test/java/org/tests/cache/TestQueryCacheTableDependency.java +++ b/ebean-test/src/test/java/org/tests/cache/TestQueryCacheTableDependency.java @@ -1,9 +1,11 @@ package org.tests.cache; import io.ebean.xtest.BaseTestCase; +import io.ebean.CacheMode; import io.ebean.DB; import io.ebean.Query; import io.ebean.cache.ServerCache; +import io.ebean.test.LoggedSql; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.tests.model.basic.Address; @@ -191,4 +193,98 @@ public void testCache_withEnabeldQueryCache() throws InterruptedException { DB.delete(child); DB.delete(root); } + + /** + * findSingleAttributeList caches with dependent tables sourced from the query plan. + * A change to the joined (dependent) table must invalidate the query cache entry. + */ + @Test + public void testFindSingleAttributeOnDependent() throws InterruptedException { + Address addr = new Address(); + addr.setCity("qcache-sa-city"); + DB.save(addr); + + Customer cust = new Customer(); + cust.setName("qcache-sa-cust"); + cust.setBillingAddress(addr); + DB.save(cust); + + DB.cacheManager().queryCache(Customer.class).clear(); + Thread.sleep(10); + + LoggedSql.start(); + List first = namesByBillingCity("qcache-sa-city"); + assertThat(LoggedSql.stop()).as("first call executes SQL").hasSize(1); + assertThat(first).containsExactly("qcache-sa-cust"); + + LoggedSql.start(); + List second = namesByBillingCity("qcache-sa-city"); + assertThat(LoggedSql.stop()).as("second call is a query cache hit").isEmpty(); + assertThat(second).isEqualTo(first); + + // modify the joined (dependent) o_address table -> evict the Customer query cache + addr.setCity("qcache-sa-city2"); + DB.save(addr); + + LoggedSql.start(); + List third = namesByBillingCity("qcache-sa-city"); + assertThat(LoggedSql.stop()).as("cache invalidated after dependent table change").hasSize(1); + assertThat(third).isEmpty(); + + DB.delete(cust); + } + + /** + * findList (unmodifiable query cache) caches with dependent tables sourced from the + * query plan. A change to the joined (dependent) table must invalidate the entry. + */ + @Test + public void testFindListOnDependent() throws InterruptedException { + Address addr = new Address(); + addr.setCity("qcache-list-city"); + DB.save(addr); + + Customer cust = new Customer(); + cust.setName("qcache-list-cust"); + cust.setBillingAddress(addr); + DB.save(cust); + + DB.cacheManager().queryCache(Customer.class).clear(); + Thread.sleep(10); + + LoggedSql.start(); + List first = customersByBillingCity("qcache-list-city"); + assertThat(LoggedSql.stop()).as("first call executes SQL").hasSize(1); + assertThat(first).hasSize(1); + + LoggedSql.start(); + List second = customersByBillingCity("qcache-list-city"); + assertThat(LoggedSql.stop()).as("second call is a query cache hit").isEmpty(); + assertThat(second).isSameAs(first); + + // modify the joined (dependent) o_address table -> evict the Customer query cache + addr.setCity("qcache-list-city2"); + DB.save(addr); + + LoggedSql.start(); + List third = customersByBillingCity("qcache-list-city"); + assertThat(LoggedSql.stop()).as("cache invalidated after dependent table change").hasSize(1); + assertThat(third).isEmpty(); + + DB.delete(cust); + } + + private List namesByBillingCity(String city) { + return DB.find(Customer.class).setUseQueryCache(CacheMode.ON) + .select("name") + .where().eq("billingAddress.city", city) + .orderBy("name") + .findSingleAttributeList(); + } + + private List customersByBillingCity(String city) { + return DB.find(Customer.class).setUseQueryCache(CacheMode.ON) + .where().eq("billingAddress.city", city) + .findList(); + } }