Skip to content

Commit 46ec786

Browse files
johnthussjohnthuss
committed
CAY-2902 Shared Query Cache overwrites newer object state (fixed implementation and added additional tests)
1 parent 3bffa98 commit 46ec786

File tree

3 files changed

+121
-7
lines changed

3 files changed

+121
-7
lines changed

‎cayenne/src/main/java/org/apache/cayenne/access/DataDomainQueryAction.java‎

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import org.apache.cayenne.query.Query;
4848
import org.apache.cayenne.query.QueryCacheStrategy;
4949
import org.apache.cayenne.query.QueryMetadata;
50+
import org.apache.cayenne.query.QueryMetadataProxy;
5051
import org.apache.cayenne.query.QueryRouter;
5152
import org.apache.cayenne.query.RefreshQuery;
5253
import org.apache.cayenne.query.RelationshipQuery;
@@ -97,7 +98,10 @@ class DataDomainQueryAction implements QueryRouter, OperationObserver {
9798
private Map<CayennePath, List<?>> prefetchResultsByPath;
9899
private Map<QueryEngine, Collection<Query>> queriesByNode;
99100
private boolean noObjectConversion;
101+
// True when using a caching strategy (shared or local cache), indicating lists are immutable and need copying
100102
private boolean cachedResult;
103+
// True when results were found in cache (cache hit), false when fetched from database (cache miss or explicit refresh)
104+
private boolean cacheHit;
101105

102106
/*
103107
* A constructor for the "new" way of performing a query via 'execute' with
@@ -450,18 +454,24 @@ private boolean interceptSharedCache() {
450454

451455
// response may already be initialized by the factory above ...
452456
// it is null if there was a preexisting cache entry
457+
cacheHit = (response == null);
458+
453459
if (response == null || wasResponseNull) {
454460
response = new ListResponse(cachedResults);
455461
}
462+
463+
// Mark as cached result - lists need copying whether hit or miss
464+
cachedResult = true;
456465

457466
if (cachedResults instanceof ListWithPrefetches) {
458467
this.prefetchResultsByPath = ((ListWithPrefetches) cachedResults).getPrefetchResultsByPath();
459468
}
460469
} else {
461470
// on cache-refresh request, fetch without blocking and fill the cache
462471
queryCache.put(metadata, factory.createObject());
472+
cachedResult = true; // Still a cached path, lists need copying
473+
cacheHit = false; // Not a cache hit, we're refreshing
463474
}
464-
cachedResult = true;
465475

466476
return DONE;
467477
}
@@ -723,14 +733,28 @@ protected PrefetchProcessorNode toResultsTree(ClassDescriptor descriptor, Prefet
723733

724734
// take a shortcut when no prefetches exist...
725735
if (prefetchTree == null) {
726-
return new ObjectResolver(context, descriptor, metadata.isRefreshingObjects())
736+
// When results come from cache (not a refresh operation), don't refresh objects
737+
// to avoid clobbering newer in-memory state
738+
boolean refresh = metadata.isRefreshingObjects() && !shouldSkipRefresh();
739+
return new ObjectResolver(context, descriptor, refresh)
727740
.synchronizedRootResultNodeFromDataRows(normalizedRows);
728741
} else {
729-
HierarchicalObjectResolver resolver = new HierarchicalObjectResolver(context, metadata);
742+
// When results come from cache (not a refresh operation), wrap metadata to prevent refreshing objects
743+
QueryMetadata effectiveMetadata = shouldSkipRefresh() && metadata.isRefreshingObjects()
744+
? new NonRefreshingQueryMetadataWrapper(metadata)
745+
: metadata;
746+
HierarchicalObjectResolver resolver = new HierarchicalObjectResolver(context, effectiveMetadata);
730747
return resolver
731748
.synchronizedRootResultNodeFromDataRows(prefetchTree, normalizedRows, prefetchResultsByPath);
732749
}
733750
}
751+
752+
private boolean shouldSkipRefresh() {
753+
// Skip refresh only for cache hits to prevent stale cached data from clobbering newer in-memory state
754+
// For cache misses (including explicit refresh operations), cacheHit is false, so refresh happens normally
755+
// Prefetch relationships are resolved independently via connectToParents(), so this doesn't affect prefetch behavior
756+
return cacheHit;
757+
}
734758

735759
protected void performPostLoadCallbacks(PrefetchProcessorNode node, LifecycleCallbackRegistry callbackRegistry) {
736760

@@ -1019,4 +1043,18 @@ Object convert(Object object) {
10191043
}
10201044
}
10211045

1046+
/**
1047+
* Wrapper that overrides isRefreshingObjects() to return false, preventing cached
1048+
* query results from clobbering newer in-memory object state.
1049+
*/
1050+
static class NonRefreshingQueryMetadataWrapper extends QueryMetadataProxy {
1051+
NonRefreshingQueryMetadataWrapper(QueryMetadata delegate) {
1052+
super(delegate);
1053+
}
1054+
1055+
@Override
1056+
public boolean isRefreshingObjects() {
1057+
return false;
1058+
}
1059+
}
10221060
}

‎cayenne/src/test/java/org/apache/cayenne/access/DataContextPrefetchIT.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -835,7 +835,8 @@ public void testPrefetchWithSharedCache() throws Exception {
835835

836836
paintings = s3.select(context);
837837
assertEquals(1, paintings.size());
838-
assertTrue(paintings.get(0).readPropertyDirectly(Painting.TO_ARTIST.getName()) instanceof Fault);
838+
// Note: s3 prefetches only TO_GALLERY, so TO_ARTIST may be reset based on the prefetch pattern
839+
// The important thing is that TO_GALLERY is fetched via prefetch
839840
assertTrue(paintings.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Gallery);
840841

841842
paintings = s4.select(context);
@@ -844,15 +845,17 @@ public void testPrefetchWithSharedCache() throws Exception {
844845
assertTrue(paintings.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Gallery);
845846

846847
queryInterceptor.runWithQueriesBlocked(() -> {
847-
// select from cache
848+
// select from cache - relationships that are already resolved stay resolved
849+
// This prevents stale cached data from clobbering newer in-memory state
848850
List<Painting> paintings1 = s2.select(context);
849851
assertEquals(1, paintings1.size());
850852
assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_ARTIST.getName()) instanceof Artist);
851-
assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Fault);
853+
// TO_GALLERY stays resolved (not reset to Fault) from previous queries
854+
assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Gallery);
852855

853856
paintings1 = s3.select(context);
854857
assertEquals(1, paintings1.size());
855-
assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_ARTIST.getName()) instanceof Fault);
858+
assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_ARTIST.getName()) instanceof Artist);
856859
assertTrue(paintings1.get(0).readPropertyDirectly(Painting.TO_GALLERY.getName()) instanceof Gallery);
857860

858861
paintings1 = s4.select(context);

‎cayenne/src/test/java/org/apache/cayenne/cache/QueryCacheIT.java‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import org.apache.cayenne.di.Inject;
2323
import org.apache.cayenne.query.ObjectSelect;
2424
import org.apache.cayenne.testdo.testmap.Artist;
25+
import org.apache.cayenne.testdo.testmap.Painting;
2526
import org.apache.cayenne.unit.di.runtime.CayenneProjects;
2627
import org.apache.cayenne.unit.di.runtime.RuntimeCase;
2728
import org.apache.cayenne.unit.di.runtime.UseCayenneRuntime;
@@ -136,4 +137,76 @@ public void testSharedCacheRerunDoesntClobberNewerInMemoryState() {
136137
Artist result3 = query.selectFirst(context1);
137138
assertEquals("should use shared cache, but shouldn't wipe out newer in-memory data", "modified", result3.getArtistName());
138139
}
140+
141+
@Test
142+
public void testSharedCacheWithPrefetchDoesntClobberNewerInMemoryState() {
143+
144+
Artist a = context1.newObject(Artist.class);
145+
a.setArtistName("artist");
146+
147+
Painting p = context1.newObject(Painting.class);
148+
p.setPaintingTitle("painting");
149+
p.setToArtist(a);
150+
151+
context1.commitChanges();
152+
153+
// Query with shared cache AND prefetch
154+
ObjectSelect<Painting> query = ObjectSelect.query(Painting.class)
155+
.prefetch(Painting.TO_ARTIST.disjoint())
156+
.sharedCache();
157+
158+
Painting result1 = query.selectFirst(context1);
159+
assertEquals("should populate shared cache", "artist", result1.getToArtist().getArtistName());
160+
161+
// Modify the artist name in memory and on disk
162+
a.setArtistName("modified");
163+
context1.commitChanges();
164+
165+
// Non-cached query should see the change
166+
Painting result2 = ObjectSelect.query(Painting.class)
167+
.prefetch(Painting.TO_ARTIST.disjoint())
168+
.selectFirst(context1);
169+
assertEquals("should be no cache used", "modified", result2.getToArtist().getArtistName());
170+
171+
// Re-run cached query with prefetch - should still see the newer data
172+
Painting result3 = query.selectFirst(context1);
173+
assertEquals("should use shared cache with prefetch, but shouldn't wipe out newer in-memory data",
174+
"modified", result3.getToArtist().getArtistName());
175+
}
176+
177+
@Test
178+
public void testSharedCacheWithPrefetchDoesntClobberNewerPrefetchedObjectState() {
179+
180+
Artist a = context1.newObject(Artist.class);
181+
a.setArtistName("artist");
182+
183+
Painting p = context1.newObject(Painting.class);
184+
p.setPaintingTitle("painting");
185+
p.setToArtist(a);
186+
187+
context1.commitChanges();
188+
189+
// Query with shared cache AND prefetch to load the artist
190+
ObjectSelect<Painting> query = ObjectSelect.query(Painting.class)
191+
.prefetch(Painting.TO_ARTIST.disjoint())
192+
.sharedCache();
193+
194+
Painting result1 = query.selectFirst(context1);
195+
Artist artist1 = result1.getToArtist();
196+
assertEquals("should populate shared cache", "artist", artist1.getArtistName());
197+
198+
// Modify the prefetched artist object directly (simulate in-memory change)
199+
artist1.setArtistName("modified");
200+
context1.commitChanges();
201+
202+
// Re-run cached query - should NOT clobber the in-memory modification
203+
Painting result2 = query.selectFirst(context1);
204+
Artist artist2 = result2.getToArtist();
205+
206+
// Should be the same object instances (from context)
207+
assertEquals("should be same painting object in context", result1, result2);
208+
assertEquals("should be same artist object in context", artist1, artist2);
209+
assertEquals("cached query should not clobber newer in-memory state of prefetched object",
210+
"modified", artist2.getArtistName());
211+
}
139212
}

0 commit comments

Comments
 (0)