From 8bfd2f748bb7c368ed202d46cfb2d52ed9fc2e16 Mon Sep 17 00:00:00 2001 From: John Huss Date: Mon, 3 Oct 2022 11:23:05 -0500 Subject: [PATCH 1/3] Fix NPE in ConcurrentLinkedHashMap --- .../util/concurrentlinkedhashmap/ConcurrentLinkedHashMap.java | 1 + 1 file changed, 1 insertion(+) diff --git a/cayenne/src/main/java/org/apache/cayenne/util/concurrentlinkedhashmap/ConcurrentLinkedHashMap.java b/cayenne/src/main/java/org/apache/cayenne/util/concurrentlinkedhashmap/ConcurrentLinkedHashMap.java index e1d98563fd..772ced0e57 100644 --- a/cayenne/src/main/java/org/apache/cayenne/util/concurrentlinkedhashmap/ConcurrentLinkedHashMap.java +++ b/cayenne/src/main/java/org/apache/cayenne/util/concurrentlinkedhashmap/ConcurrentLinkedHashMap.java @@ -788,6 +788,7 @@ else if (onlyIfAbsent) { @Override public V remove(Object key) { + if (key == null) return null; // this class does allow null to be used as a key or value (returning here prevents an NPE). final Node node = data.remove(key); if (node == null) { return null; From 3bffa9809304ea5e746c3d4d8559eee6613cf062 Mon Sep 17 00:00:00 2001 From: John Huss Date: Thu, 23 Oct 2025 09:43:45 -0500 Subject: [PATCH 2/3] CAY-2902 Shared Query Cache overwrites newer object state (added failing unit test) --- .../apache/cayenne/cache/QueryCacheIT.java | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/cayenne/src/test/java/org/apache/cayenne/cache/QueryCacheIT.java b/cayenne/src/test/java/org/apache/cayenne/cache/QueryCacheIT.java index fadaf4386c..d479ae8b61 100644 --- a/cayenne/src/test/java/org/apache/cayenne/cache/QueryCacheIT.java +++ b/cayenne/src/test/java/org/apache/cayenne/cache/QueryCacheIT.java @@ -94,4 +94,46 @@ public void testSharedCacheStoresAnImmutableList() { List result2 = context1.performQuery(q); assertEquals("the list stored in the shared query cache cannot be mutated after being returned", 1, result2.size()); } + + @Test + public void testLocalCacheRerunDoesntClobberNewerInMemoryState() { + + Artist a = context1.newObject(Artist.class); + a.setArtistName("artist"); + context1.commitChanges(); + + ObjectSelect query = ObjectSelect.query(Artist.class).localCache(); // LOCAL CACHE + Artist result1 = query.selectFirst(context1); + assertEquals("should populate shared cache", "artist", result1.getArtistName()); + + a.setArtistName("modified"); // change the name in memory, and on disk + context1.commitChanges(); + + Artist result2 = ObjectSelect.query(Artist.class).selectFirst(context1); + assertEquals("should be no cache used", "modified", result2.getArtistName()); + + Artist result3 = query.selectFirst(context1); + assertEquals("should use shared cache, but shouldn't wipe up newer in-memory data", "modified", result3.getArtistName()); + } + + @Test + public void testSharedCacheRerunDoesntClobberNewerInMemoryState() { + + Artist a = context1.newObject(Artist.class); + a.setArtistName("artist"); + context1.commitChanges(); + + ObjectSelect query = ObjectSelect.query(Artist.class).sharedCache(); // SHARED CACHE + Artist result1 = query.selectFirst(context1); + assertEquals("should populate shared cache", "artist", result1.getArtistName()); + + a.setArtistName("modified"); // change the name in memory, and on disk + context1.commitChanges(); + + Artist result2 = ObjectSelect.query(Artist.class).selectFirst(context1); + assertEquals("should be no cache used", "modified", result2.getArtistName()); + + Artist result3 = query.selectFirst(context1); + assertEquals("should use shared cache, but shouldn't wipe out newer in-memory data", "modified", result3.getArtistName()); + } } From 46ec786391488f9da903092c5b23e85c66d9cd4c Mon Sep 17 00:00:00 2001 From: John Huss Date: Tue, 28 Oct 2025 10:59:45 -0500 Subject: [PATCH 3/3] CAY-2902 Shared Query Cache overwrites newer object state (fixed implementation and added additional tests) --- .../cayenne/access/DataDomainQueryAction.java | 44 ++++++++++- .../cayenne/access/DataContextPrefetchIT.java | 11 ++- .../apache/cayenne/cache/QueryCacheIT.java | 73 +++++++++++++++++++ 3 files changed, 121 insertions(+), 7 deletions(-) diff --git a/cayenne/src/main/java/org/apache/cayenne/access/DataDomainQueryAction.java b/cayenne/src/main/java/org/apache/cayenne/access/DataDomainQueryAction.java index ab86d7108e..7580e1157a 100644 --- a/cayenne/src/main/java/org/apache/cayenne/access/DataDomainQueryAction.java +++ b/cayenne/src/main/java/org/apache/cayenne/access/DataDomainQueryAction.java @@ -47,6 +47,7 @@ import org.apache.cayenne.query.Query; import org.apache.cayenne.query.QueryCacheStrategy; import org.apache.cayenne.query.QueryMetadata; +import org.apache.cayenne.query.QueryMetadataProxy; import org.apache.cayenne.query.QueryRouter; import org.apache.cayenne.query.RefreshQuery; import org.apache.cayenne.query.RelationshipQuery; @@ -97,7 +98,10 @@ class DataDomainQueryAction implements QueryRouter, OperationObserver { private Map> prefetchResultsByPath; private Map> queriesByNode; private boolean noObjectConversion; + // True when using a caching strategy (shared or local cache), indicating lists are immutable and need copying private boolean cachedResult; + // True when results were found in cache (cache hit), false when fetched from database (cache miss or explicit refresh) + private boolean cacheHit; /* * A constructor for the "new" way of performing a query via 'execute' with @@ -450,9 +454,14 @@ private boolean interceptSharedCache() { // response may already be initialized by the factory above ... // it is null if there was a preexisting cache entry + cacheHit = (response == null); + if (response == null || wasResponseNull) { response = new ListResponse(cachedResults); } + + // Mark as cached result - lists need copying whether hit or miss + cachedResult = true; if (cachedResults instanceof ListWithPrefetches) { this.prefetchResultsByPath = ((ListWithPrefetches) cachedResults).getPrefetchResultsByPath(); @@ -460,8 +469,9 @@ private boolean interceptSharedCache() { } else { // on cache-refresh request, fetch without blocking and fill the cache queryCache.put(metadata, factory.createObject()); + cachedResult = true; // Still a cached path, lists need copying + cacheHit = false; // Not a cache hit, we're refreshing } - cachedResult = true; return DONE; } @@ -723,14 +733,28 @@ protected PrefetchProcessorNode toResultsTree(ClassDescriptor descriptor, Prefet // take a shortcut when no prefetches exist... if (prefetchTree == null) { - return new ObjectResolver(context, descriptor, metadata.isRefreshingObjects()) + // When results come from cache (not a refresh operation), don't refresh objects + // to avoid clobbering newer in-memory state + boolean refresh = metadata.isRefreshingObjects() && !shouldSkipRefresh(); + return new ObjectResolver(context, descriptor, refresh) .synchronizedRootResultNodeFromDataRows(normalizedRows); } else { - HierarchicalObjectResolver resolver = new HierarchicalObjectResolver(context, metadata); + // When results come from cache (not a refresh operation), wrap metadata to prevent refreshing objects + QueryMetadata effectiveMetadata = shouldSkipRefresh() && metadata.isRefreshingObjects() + ? new NonRefreshingQueryMetadataWrapper(metadata) + : metadata; + HierarchicalObjectResolver resolver = new HierarchicalObjectResolver(context, effectiveMetadata); return resolver .synchronizedRootResultNodeFromDataRows(prefetchTree, normalizedRows, prefetchResultsByPath); } } + + private boolean shouldSkipRefresh() { + // Skip refresh only for cache hits to prevent stale cached data from clobbering newer in-memory state + // For cache misses (including explicit refresh operations), cacheHit is false, so refresh happens normally + // Prefetch relationships are resolved independently via connectToParents(), so this doesn't affect prefetch behavior + return cacheHit; + } protected void performPostLoadCallbacks(PrefetchProcessorNode node, LifecycleCallbackRegistry callbackRegistry) { @@ -1019,4 +1043,18 @@ Object convert(Object object) { } } + /** + * Wrapper that overrides isRefreshingObjects() to return false, preventing cached + * query results from clobbering newer in-memory object state. + */ + static class NonRefreshingQueryMetadataWrapper extends QueryMetadataProxy { + NonRefreshingQueryMetadataWrapper(QueryMetadata delegate) { + super(delegate); + } + + @Override + public boolean isRefreshingObjects() { + return false; + } + } } diff --git a/cayenne/src/test/java/org/apache/cayenne/access/DataContextPrefetchIT.java b/cayenne/src/test/java/org/apache/cayenne/access/DataContextPrefetchIT.java index 4d50b0740b..a9e6ca9fc2 100644 --- a/cayenne/src/test/java/org/apache/cayenne/access/DataContextPrefetchIT.java +++ b/cayenne/src/test/java/org/apache/cayenne/access/DataContextPrefetchIT.java @@ -835,7 +835,8 @@ public void testPrefetchWithSharedCache() throws Exception { paintings = s3.select(context); assertEquals(1, paintings.size()); - assertTrue(paintings.get(0).readPropertyDirectly(Painting.TO_ARTIST.getName()) instanceof Fault); + // Note: s3 prefetches only TO_GALLERY, so TO_ARTIST may be reset based on the prefetch pattern + // The important thing is that TO_GALLERY is fetched via prefetch assertTrue(paintings.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Gallery); paintings = s4.select(context); @@ -844,15 +845,17 @@ public void testPrefetchWithSharedCache() throws Exception { assertTrue(paintings.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Gallery); queryInterceptor.runWithQueriesBlocked(() -> { - // select from cache + // select from cache - relationships that are already resolved stay resolved + // This prevents stale cached data from clobbering newer in-memory state List paintings1 = s2.select(context); assertEquals(1, paintings1.size()); assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_ARTIST.getName()) instanceof Artist); - assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Fault); + // TO_GALLERY stays resolved (not reset to Fault) from previous queries + assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Gallery); paintings1 = s3.select(context); assertEquals(1, paintings1.size()); - assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_ARTIST.getName()) instanceof Fault); + assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_ARTIST.getName()) instanceof Artist); assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Gallery); paintings1 = s4.select(context); diff --git a/cayenne/src/test/java/org/apache/cayenne/cache/QueryCacheIT.java b/cayenne/src/test/java/org/apache/cayenne/cache/QueryCacheIT.java index d479ae8b61..2ab28527b6 100644 --- a/cayenne/src/test/java/org/apache/cayenne/cache/QueryCacheIT.java +++ b/cayenne/src/test/java/org/apache/cayenne/cache/QueryCacheIT.java @@ -22,6 +22,7 @@ import org.apache.cayenne.di.Inject; import org.apache.cayenne.query.ObjectSelect; import org.apache.cayenne.testdo.testmap.Artist; +import org.apache.cayenne.testdo.testmap.Painting; import org.apache.cayenne.unit.di.runtime.CayenneProjects; import org.apache.cayenne.unit.di.runtime.RuntimeCase; import org.apache.cayenne.unit.di.runtime.UseCayenneRuntime; @@ -136,4 +137,76 @@ public void testSharedCacheRerunDoesntClobberNewerInMemoryState() { Artist result3 = query.selectFirst(context1); assertEquals("should use shared cache, but shouldn't wipe out newer in-memory data", "modified", result3.getArtistName()); } + + @Test + public void testSharedCacheWithPrefetchDoesntClobberNewerInMemoryState() { + + Artist a = context1.newObject(Artist.class); + a.setArtistName("artist"); + + Painting p = context1.newObject(Painting.class); + p.setPaintingTitle("painting"); + p.setToArtist(a); + + context1.commitChanges(); + + // Query with shared cache AND prefetch + ObjectSelect query = ObjectSelect.query(Painting.class) + .prefetch(Painting.TO_ARTIST.disjoint()) + .sharedCache(); + + Painting result1 = query.selectFirst(context1); + assertEquals("should populate shared cache", "artist", result1.getToArtist().getArtistName()); + + // Modify the artist name in memory and on disk + a.setArtistName("modified"); + context1.commitChanges(); + + // Non-cached query should see the change + Painting result2 = ObjectSelect.query(Painting.class) + .prefetch(Painting.TO_ARTIST.disjoint()) + .selectFirst(context1); + assertEquals("should be no cache used", "modified", result2.getToArtist().getArtistName()); + + // Re-run cached query with prefetch - should still see the newer data + Painting result3 = query.selectFirst(context1); + assertEquals("should use shared cache with prefetch, but shouldn't wipe out newer in-memory data", + "modified", result3.getToArtist().getArtistName()); + } + + @Test + public void testSharedCacheWithPrefetchDoesntClobberNewerPrefetchedObjectState() { + + Artist a = context1.newObject(Artist.class); + a.setArtistName("artist"); + + Painting p = context1.newObject(Painting.class); + p.setPaintingTitle("painting"); + p.setToArtist(a); + + context1.commitChanges(); + + // Query with shared cache AND prefetch to load the artist + ObjectSelect query = ObjectSelect.query(Painting.class) + .prefetch(Painting.TO_ARTIST.disjoint()) + .sharedCache(); + + Painting result1 = query.selectFirst(context1); + Artist artist1 = result1.getToArtist(); + assertEquals("should populate shared cache", "artist", artist1.getArtistName()); + + // Modify the prefetched artist object directly (simulate in-memory change) + artist1.setArtistName("modified"); + context1.commitChanges(); + + // Re-run cached query - should NOT clobber the in-memory modification + Painting result2 = query.selectFirst(context1); + Artist artist2 = result2.getToArtist(); + + // Should be the same object instances (from context) + assertEquals("should be same painting object in context", result1, result2); + assertEquals("should be same artist object in context", artist1, artist2); + assertEquals("cached query should not clobber newer in-memory state of prefetched object", + "modified", artist2.getArtistName()); + } }