Merge pull request #22769 from lgxbslgx

* pr/22769:
  Polish "Evaluate key only if necessary"
  Evaluate key only if necessary

Closes gh-22769

spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java

@@ -401,13 +401,6 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 		// Check if we have a cached item matching the conditions
 		Cache.ValueWrapper cacheHit = findCachedItem(contexts.get(CacheableOperation.class));
 
-		// Collect puts from any @Cacheable miss, if no cached item is found
-		List<CachePutRequest> cachePutRequests = new ArrayList<>();
-		if (cacheHit == null) {
-			collectPutRequests(contexts.get(CacheableOperation.class),
-					CacheOperationExpressionEvaluator.NO_RESULT, cachePutRequests);
-		}
-
 		Object cacheValue;
 		Object returnValue;
 
@@ -422,6 +415,12 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 			cacheValue = unwrapReturnValue(returnValue);
 		}
 
+		// Collect puts from any @Cacheable miss, if no cached item is found
+		List<CachePutRequest> cachePutRequests = new ArrayList<>();
+		if (cacheHit == null) {
+			collectPutRequests(contexts.get(CacheableOperation.class), cacheValue, cachePutRequests);
+		}
+
 		// Collect any explicit @CachePuts
 		collectPutRequests(contexts.get(CachePutOperation.class), cacheValue, cachePutRequests);
 
@@ -558,7 +557,7 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 			@Nullable Object result, Collection<CachePutRequest> putRequests) {
 
 		for (CacheOperationContext context : contexts) {
-			if (isConditionPassing(context, result)) {
+			if (isConditionPassing(context, result) && context.canPutToCache(result)) {
 				Object key = generateKey(context, result);
 				putRequests.add(new CachePutRequest(context, key));
 			}
@@ -832,10 +831,8 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 		}
 
 		public void apply(@Nullable Object result) {
-			if (this.context.canPutToCache(result)) {
-				for (Cache cache : this.context.getCaches()) {
-					doPut(cache, this.key, result);
-				}
+			for (Cache cache : this.context.getCaches()) {
+				doPut(cache, this.key, result);
 			}
 		}
 	}

spring-context/src/test/resources/org/springframework/cache/config/cache-advice.xml

@@ -54,6 +54,7 @@
 			<cache:cache-put method="multiUpdate" cache="primary"/>
 			<cache:cache-put method="multiUpdate" cache="secondary"/>
 			<cache:cache-put method="putRefersToResult" cache="primary" key="#result.id"/>
+			<cache:cache-put method="putEvaluatesUnlessBeforeKey" cache="primary" key="#result.id" unless="#result == null"/>
 		</cache:caching>
 	</cache:advice>
 
@@ -93,6 +94,7 @@
 			<cache:cache-put method="multiUpdate" cache="primary"/>
 			<cache:cache-put method="multiUpdate" cache="secondary"/>
 			<cache:cache-put method="putRefersToResult" cache="primary" key="#result.id"/>
+			<cache:cache-put method="putEvaluatesUnlessBeforeKey" cache="primary" key="#result.id" unless="#result == null"/>
 		</cache:caching>
 	</cache:advice>
 

spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/AbstractCacheAnnotationTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2020 the original author or authors.
+ * Copyright 2002-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -504,6 +504,26 @@ public abstract class AbstractCacheAnnotationTests {
 		assertThat(primary.get(id).get()).isSameAs(entity);
 	}
 
+	protected void testPutRefersToResultWithUnless(CacheableService<?> service) {
+		Long id = 42L;
+		TestEntity entity = new TestEntity();
+		entity.setId(id);
+		Cache primary = this.cm.getCache("primary");
+		assertThat(primary.get(id)).isNull();
+		assertThat(service.putEvaluatesUnlessBeforeKey(entity)).isNotNull();
+		assertThat(primary.get(id).get()).isSameAs(entity);
+	}
+
+	protected void testPutEvaluatesUnlessBeforeKey(CacheableService<?> service) {
+		Long id = Long.MIN_VALUE; // return null
+		TestEntity entity = new TestEntity();
+		entity.setId(id);
+		Cache primary = this.cm.getCache("primary");
+		assertThat(primary.get(id)).isNull();
+		assertThat(service.putEvaluatesUnlessBeforeKey(entity)).isNull();
+		assertThat(primary.get(id)).isNull();
+	}
+
 	protected void testMultiCacheAndEvict(CacheableService<?> service) {
 		String methodName = "multiCacheAndEvict";
 
@@ -849,11 +869,31 @@ public abstract class AbstractCacheAnnotationTests {
 		testPutRefersToResult(this.cs);
 	}
 
+	@Test
+	public void testPutRefersToResultWithUnless() {
+		testPutRefersToResultWithUnless(this.cs);
+	}
+
+	@Test
+	public void testPutEvaluatesUnlessBeforeKey() {
+		testPutEvaluatesUnlessBeforeKey(this.cs);
+	}
+
 	@Test
 	public void testClassPutRefersToResult() {
 		testPutRefersToResult(this.ccs);
 	}
 
+	@Test
+	public void testClassPutRefersToResultWithUnless(){
+		testPutRefersToResultWithUnless(this.ccs);
+	}
+
+	@Test
+	public void testClassPutEvaluatesUnlessBeforeKey(){
+		testPutEvaluatesUnlessBeforeKey(this.ccs);
+	}
+
 	@Test
 	public void testMultiCacheAndEvict() {
 		testMultiCacheAndEvict(this.cs);

spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/AnnotatedClassCacheableService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -235,4 +235,10 @@ public class AnnotatedClassCacheableService implements CacheableService<Object>
 		return arg1;
 	}
 
+	@Override
+	@CachePut(cacheNames = "primary", key = "#result.id", unless = "#result == null")
+	public TestEntity putEvaluatesUnlessBeforeKey(TestEntity arg1) {
+		return (arg1.getId() != Long.MIN_VALUE ? arg1 : null);
+	}
+
 }

spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/CacheableService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -93,4 +93,6 @@ public interface CacheableService<T> {
 
 	TestEntity putRefersToResult(TestEntity arg1);
 
+	TestEntity putEvaluatesUnlessBeforeKey(TestEntity arg1);
+
 }

spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/DefaultCacheableService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -243,4 +243,10 @@ public class DefaultCacheableService implements CacheableService<Long> {
 		return arg1;
 	}
 
+	@Override
+	@CachePut(cacheNames = "primary", key = "#result.id", unless = "#result == null")
+	public TestEntity putEvaluatesUnlessBeforeKey(TestEntity arg1) {
+		return (arg1.getId() != Long.MIN_VALUE ? arg1 : null);
+	}
+
 }