Late key generation for consistent evaluation of unless expressions

Includes removal of evict step on pipeline exception, retaining a previous cache value and avoiding an incomplete key (for consistency with non-reactive caching). Closes gh-31626
spring-projects · Nov 21, 2023 · 5a3ad6b · 5a3ad6b
1 parent 1410c46
commit 5a3ad6b
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 36 deletions.
diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java b/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java
@@ -652,9 +652,8 @@ private void collectPutRequests(Collection<CacheOperationContext> contexts,
 			@Nullable Object result, Collection<CachePutRequest> putRequests) {
 
 		for (CacheOperationContext context : contexts) {
-			if (isConditionPassing(context, result) && context.canPutToCache(result)) {
-				Object key = generateKey(context, result);
-				putRequests.add(new CachePutRequest(context, key));
+			if (isConditionPassing(context, result)) {
+				putRequests.add(new CachePutRequest(context));
 			}
 		}
 	}
@@ -943,22 +942,16 @@ private class CachePutRequest {
 
 		private final CacheOperationContext context;
 
-		private final Object key;
-
-		public CachePutRequest(CacheOperationContext context, Object key) {
+		public CachePutRequest(CacheOperationContext context) {
 			this.context = context;
-			this.key = key;
 		}
 
 		@Nullable
 		public Object apply(@Nullable Object result) {
 			if (result instanceof CompletableFuture<?> future) {
 				return future.whenComplete((value, ex) -> {
-					if (ex != null) {
-						performEvict(ex);
-					}
-					else {
-						performPut(value);
+					if (ex == null) {
+						performCachePut(value);
 					}
 				});
 			}
@@ -968,27 +961,20 @@ public Object apply(@Nullable Object result) {
 					return returnValue;
 				}
 			}
-			performPut(result);
+			performCachePut(result);
 			return null;
 		}
 
-		void performPut(@Nullable Object value) {
-			if (logger.isTraceEnabled()) {
-				logger.trace("Creating cache entry for key '" + this.key + "' in cache(s) " +
-						this.context.getCacheNames());
-			}
-			for (Cache cache : this.context.getCaches()) {
-				doPut(cache, this.key, value);
-			}
-		}
-
-		void performEvict(Throwable cause) {
-			if (logger.isTraceEnabled()) {
-				logger.trace("Removing cache entry for key '" + this.key + "' from cache(s) " +
-						this.context.getCacheNames() + " due to exception: " + cause);
-			}
-			for (Cache cache : this.context.getCaches()) {
-				doEvict(cache, this.key, false);
+		public void performCachePut(@Nullable Object value) {
+			if (this.context.canPutToCache(value)) {
+				Object key = generateKey(this.context, value);
+				if (logger.isTraceEnabled()) {
+					logger.trace("Creating cache entry for key '" + key + "' in cache(s) " +
+							this.context.getCacheNames());
+				}
+				for (Cache cache : this.context.getCaches()) {
+					doPut(cache, key, value);
+				}
 			}
 		}
 	}
@@ -1017,11 +1003,10 @@ public void onNext(Object o) {
 		}
 		@Override
 		public void onError(Throwable t) {
-			this.request.performEvict(t);
 		}
 		@Override
 		public void onComplete() {
-			this.request.performPut(this.cacheValue);
+			this.request.performCachePut(this.cacheValue);
 		}
 	}
 
@@ -1107,7 +1092,7 @@ public Object processPutRequest(CachePutRequest request, @Nullable Object result
 				}
 				else {
 					return adapter.fromPublisher(Mono.from(adapter.toPublisher(result))
-							.doOnSuccess(request::performPut).doOnError(request::performEvict));
+							.doOnSuccess(request::performCachePut));
 				}
 			}
 			return NOT_HANDLED;

diff --git a/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java b/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java
@@ -197,6 +197,11 @@ void spr14235AdaptsToCompletableFuture() {
 		assertThat(bean.findById("tb1").join()).isSameAs(tb);
 		assertThat(cache.get("tb1").get()).isSameAs(tb);
 
+		tb = bean.findById("tb2").join();
+		assertThat(tb).isNotNull();
+		assertThat(bean.findById("tb2").join()).isNotSameAs(tb);
+		assertThat(cache.get("tb2")).isNull();
+
 		context.close();
 	}
 
@@ -247,6 +252,11 @@ void spr14235AdaptsToReactorMono() {
 		assertThat(bean.findById("tb1").block()).isSameAs(tb);
 		assertThat(cache.get("tb1").get()).isSameAs(tb);
 
+		tb = bean.findById("tb2").block();
+		assertThat(tb).isNotNull();
+		assertThat(bean.findById("tb2").block()).isNotSameAs(tb);
+		assertThat(cache.get("tb2")).isNull();
+
 		context.close();
 	}
 
@@ -297,6 +307,11 @@ void spr14235AdaptsToReactorFlux() {
 		assertThat(bean.findById("tb1").collectList().block()).isEqualTo(tb);
 		assertThat(cache.get("tb1").get()).isEqualTo(tb);
 
+		tb = bean.findById("tb2").collectList().block();
+		assertThat(tb).isNotEmpty();
+		assertThat(bean.findById("tb2").collectList().block()).isNotEqualTo(tb);
+		assertThat(cache.get("tb2")).isNull();
+
 		context.close();
 	}
 
@@ -548,7 +563,7 @@ public Spr14230Service service() {
 
 	public static class Spr14235FutureService {
 
-		@Cacheable(value = "itemCache")
+		@Cacheable(value = "itemCache", unless = "#result.name == 'tb2'")
 		public CompletableFuture<TestBean> findById(String id) {
 			return CompletableFuture.completedFuture(new TestBean(id));
 		}
@@ -581,7 +596,7 @@ public TestBean insertItem(TestBean item) {
 
 	public static class Spr14235MonoService {
 
-		@Cacheable(value = "itemCache")
+		@Cacheable(value = "itemCache", unless = "#result.name == 'tb2'")
 		public Mono<TestBean> findById(String id) {
 			return Mono.just(new TestBean(id));
 		}
@@ -616,7 +631,7 @@ public static class Spr14235FluxService {
 
 		private int counter = 0;
 
-		@Cacheable(value = "itemCache")
+		@Cacheable(value = "itemCache", unless = "#result[0].name == 'tb2'")
 		public Flux<TestBean> findById(String id) {
 			return Flux.just(new TestBean(id), new TestBean(id + (counter++)));
 		}