Evaluate key only if necessary

Prior to this commit a @CachePut operation would fail if the key
expression is invalid, but guarded with an unless condition as the
former was evaluated too early. This commit makes sure that key for
a put is only evaluated if the put operation is active.

Note that this does not apply for @Cacheable as the key needs to be
computed early to determine if a matching entry exists in the cache.

See gh-22769

spring-context/src/main/java/org/springframework/cache/annotation/CacheEvict.java

@@ -119,6 +119,10 @@ public @interface CacheEvict {
 	 * <p>The SpEL expression evaluates against a dedicated context that provides the
 	 * following meta-data:
 	 * <ul>
+	 * <li>{@code #result} for a reference to the result of the method invocation, which
+	 * can only be used if {@link #beforeInvocation()} is {@code false}. For supported
+	 * wrappers such as {@code Optional}, {@code #result} refers to the actual object,
+	 * not the wrapper</li>
 	 * <li>{@code #root.method}, {@code #root.target}, and {@code #root.caches} for
 	 * references to the {@link java.lang.reflect.Method method}, target object, and
 	 * affected cache(s) respectively.</li>

spring-context/src/main/java/org/springframework/cache/annotation/Cacheable.java

@@ -131,6 +131,9 @@ public @interface Cacheable {
 	 * <p>The SpEL expression evaluates against a dedicated context that provides the
 	 * following meta-data:
 	 * <ul>
+	 * <li>{@code #result} for a reference to the result of the method invocation. For
+	 * supported wrappers such as {@code Optional}, {@code #result} refers to the actual
+	 * object, not the wrapper</li>
 	 * <li>{@code #root.method}, {@code #root.target}, and {@code #root.caches} for
 	 * references to the {@link java.lang.reflect.Method method}, target object, and
 	 * affected cache(s) respectively.</li>

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

@@ -21,6 +21,7 @@ import java.lang.reflect.Proxy;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -401,13 +402,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 +416,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 LinkedList<>();
+		if (cacheHit == null) {
+			collectPutRequests(contexts.get(CacheableOperation.class), cacheValue, cachePutRequests);
+		}
+
 		// Collect any explicit @CachePuts
 		collectPutRequests(contexts.get(CachePutOperation.class), cacheValue, cachePutRequests);
 
@@ -558,7 +558,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 +832,8 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 		}
 
 		public void apply(@Nullable Object result) {
-			if (this.context.canPutToCache(result)) {
+			for (Cache cache : this.context.getCaches()) {
-				for (Cache cache : this.context.getCaches()) {
+				doPut(cache, this.key, result);
-					doPut(cache, this.key, result);
-				}
 			}
 		}
 	}

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-2022 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.
@@ -31,10 +31,13 @@ import org.springframework.context.ConfigurableApplicationContext;
 import org.springframework.context.testfixture.cache.beans.AnnotatedClassCacheableService;
 import org.springframework.context.testfixture.cache.beans.CacheableService;
 import org.springframework.context.testfixture.cache.beans.TestEntity;
+import org.springframework.expression.spel.SpelEvaluationException;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.assertThatIOException;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
 
 /**
  * Abstract cache annotation tests (containing several reusable methods).
@@ -504,6 +507,29 @@ public abstract class AbstractCacheAnnotationTests {
 		assertThat(primary.get(id).get()).isSameAs(entity);
 	}
 
+	public void testPutRefersToNullResult(CacheableService<?> service) {
+		Long id = Long.MIN_VALUE;
+		TestEntity entity = new TestEntity();
+		Cache primary = this.cm.getCache("primary");
+		assertNull(primary.get(id));
+		try {
+			service.putRefersToNullResult(entity);
+		} catch (Exception e) {
+			assertEquals(SpelEvaluationException.class, e.getClass());
+			assertEquals("EL1007E: Property or field 'id' cannot be found on null", e.getMessage());
+		}
+		assertNull(primary.get(id));
+	}
+
+	public void testPutRefersToNullResultWithUnless(CacheableService<?> service) {
+		Long id = Long.MIN_VALUE;
+		TestEntity entity = new TestEntity();
+		Cache primary = this.cm.getCache("primary");
+		assertNull(primary.get(id));
+		service.putRefersToNullResultWithUnless(entity);
+		assertNull(primary.get(id));
+	}
+
 	protected void testMultiCacheAndEvict(CacheableService<?> service) {
 		String methodName = "multiCacheAndEvict";
 
@@ -854,6 +880,26 @@ public abstract class AbstractCacheAnnotationTests {
 		testPutRefersToResult(this.ccs);
 	}
 
+	@Test
+	public void testPutRefersToNullResult() throws Exception {
+		testPutRefersToNullResult(this.cs);
+	}
+
+	@Test
+	public void testClassPutRefersToNullResult() throws Exception {
+		testPutRefersToNullResult(this.ccs);
+	}
+
+	@Test
+	public void testPutRefersToNullResultWithUnless() throws Exception {
+		testPutRefersToNullResultWithUnless(this.cs);
+	}
+
+	@Test
+	public void testClassPutRefersToNullResultWithUnless() throws Exception {
+		testPutRefersToNullResultWithUnless(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-2022 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,15 @@ public class AnnotatedClassCacheableService implements CacheableService<Object>
 		return arg1;
 	}
 
+	@Override
+	@CachePut(cacheNames = "primary", key = "#result.id")
+	public TestEntity putRefersToNullResult(TestEntity arg1) {
+		return null;
+	}
+
+	@Override
+	@CachePut(cacheNames = "primary", key = "#result.id", unless = "#result == null")
+	public TestEntity putRefersToNullResultWithUnless(TestEntity arg1) {
+		return 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-2022 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,8 @@ public interface CacheableService<T> {
 
 	TestEntity putRefersToResult(TestEntity arg1);
 
+	TestEntity putRefersToNullResult(TestEntity arg1);
+
+	TestEntity putRefersToNullResultWithUnless(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-2022 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,15 @@ public class DefaultCacheableService implements CacheableService<Long> {
 		return arg1;
 	}
 
+	@Override
+	@CachePut(cacheNames = "primary", key = "#result.id")
+	public TestEntity putRefersToNullResult(TestEntity arg1) {
+		return null;
+	}
+
+	@Override
+	@CachePut(cacheNames = "primary", key = "#result.id", unless = "#result == null")
+	public TestEntity putRefersToNullResultWithUnless(TestEntity arg1) {
+		return null;
+	}
 }