From d8854a2f3f1c5aa03de445a3f7c2bd82b1b7a01b Mon Sep 17 00:00:00 2001
From: Stephane Nicoll <snicoll@vmware.com>
Date: Fri, 14 Jul 2023 10:23:25 +0200
Subject: [PATCH] Polish "Evaluate key only if necessary"

See gh-22769
---
 .../cache/annotation/CacheEvict.java          |  4 --
 .../cache/annotation/Cacheable.java           |  3 -
 .../cache/interceptor/CacheAspectSupport.java |  3 +-
 .../cache/config/cache-advice.xml             |  2 +
 .../cache/AbstractCacheAnnotationTests.java   | 60 +++++++++----------
 .../beans/AnnotatedClassCacheableService.java | 13 ++--
 .../cache/beans/CacheableService.java         |  6 +-
 .../cache/beans/DefaultCacheableService.java  | 13 ++--
 8 files changed, 40 insertions(+), 64 deletions(-)

diff --git a/spring-context/src/main/java/org/springframework/cache/annotation/CacheEvict.java b/spring-context/src/main/java/org/springframework/cache/annotation/CacheEvict.java
index 5fb2f90289..43f095236a 100644
--- a/spring-context/src/main/java/org/springframework/cache/annotation/CacheEvict.java
+++ b/spring-context/src/main/java/org/springframework/cache/annotation/CacheEvict.java
@@ -119,10 +119,6 @@ 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>
diff --git a/spring-context/src/main/java/org/springframework/cache/annotation/Cacheable.java b/spring-context/src/main/java/org/springframework/cache/annotation/Cacheable.java
index bc68080cda..51ea3ec258 100644
--- a/spring-context/src/main/java/org/springframework/cache/annotation/Cacheable.java
+++ b/spring-context/src/main/java/org/springframework/cache/annotation/Cacheable.java
@@ -131,9 +131,6 @@ 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>
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
index 56ccd961ab..53dd096034 100644
--- a/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java
+++ b/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java
@@ -21,7 +21,6 @@ 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;
@@ -417,7 +416,7 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 		}
 
 		// Collect puts from any @Cacheable miss, if no cached item is found
-		List<CachePutRequest> cachePutRequests = new LinkedList<>();
+		List<CachePutRequest> cachePutRequests = new ArrayList<>();
 		if (cacheHit == null) {
 			collectPutRequests(contexts.get(CacheableOperation.class), cacheValue, cachePutRequests);
 		}
diff --git a/spring-context/src/test/resources/org/springframework/cache/config/cache-advice.xml b/spring-context/src/test/resources/org/springframework/cache/config/cache-advice.xml
index ea6901d7a2..eb9ffce842 100644
--- a/spring-context/src/test/resources/org/springframework/cache/config/cache-advice.xml
+++ b/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>
 
diff --git a/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/AbstractCacheAnnotationTests.java b/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/AbstractCacheAnnotationTests.java
index 66ccb45bdb..149636bf13 100644
--- a/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/AbstractCacheAnnotationTests.java
+++ b/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/AbstractCacheAnnotationTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 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.
@@ -31,13 +31,10 @@ 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).
@@ -507,27 +504,24 @@ public abstract class AbstractCacheAnnotationTests {
 		assertThat(primary.get(id).get()).isSameAs(entity);
 	}
 
-	public void testPutRefersToNullResult(CacheableService<?> service) {
-		Long id = Long.MIN_VALUE;
+	protected void testPutRefersToResultWithUnless(CacheableService<?> service) {
+		Long id = 42L;
 		TestEntity entity = new TestEntity();
+		entity.setId(id);
 		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));
+		assertThat(primary.get(id)).isNull();
+		assertThat(service.putEvaluatesUnlessBeforeKey(entity)).isNotNull();
+		assertThat(primary.get(id).get()).isSameAs(entity);
 	}
 
-	public void testPutRefersToNullResultWithUnless(CacheableService<?> service) {
-		Long id = Long.MIN_VALUE;
+	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");
-		assertNull(primary.get(id));
-		service.putRefersToNullResultWithUnless(entity);
-		assertNull(primary.get(id));
+		assertThat(primary.get(id)).isNull();
+		assertThat(service.putEvaluatesUnlessBeforeKey(entity)).isNull();
+		assertThat(primary.get(id)).isNull();
 	}
 
 	protected void testMultiCacheAndEvict(CacheableService<?> service) {
@@ -875,29 +869,29 @@ 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 testPutRefersToNullResult() throws Exception {
-		testPutRefersToNullResult(this.cs);
+	public void testClassPutRefersToResultWithUnless(){
+		testPutRefersToResultWithUnless(this.ccs);
 	}
 
 	@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);
+	public void testClassPutEvaluatesUnlessBeforeKey(){
+		testPutEvaluatesUnlessBeforeKey(this.ccs);
 	}
 
 	@Test
diff --git a/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/AnnotatedClassCacheableService.java b/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/AnnotatedClassCacheableService.java
index a6891f6f18..0f9c920112 100644
--- a/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/AnnotatedClassCacheableService.java
+++ b/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/AnnotatedClassCacheableService.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 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.
@@ -236,14 +236,9 @@ public class AnnotatedClassCacheableService implements CacheableService<Object>
 	}
 
 	@Override
-	@CachePut(cacheNames = "primary", key = "#result.id")
-	public TestEntity putRefersToNullResult(TestEntity arg1) {
-		return null;
+	@CachePut(cacheNames = "primary", key = "#result.id", unless = "#result == null")
+	public TestEntity putEvaluatesUnlessBeforeKey(TestEntity arg1) {
+		return (arg1.getId() != Long.MIN_VALUE ? arg1 : null);
 	}
 
-	@Override
-	@CachePut(cacheNames = "primary", key = "#result.id", unless = "#result == null")
-	public TestEntity putRefersToNullResultWithUnless(TestEntity arg1) {
-		return null;
-	}
 }
diff --git a/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/CacheableService.java b/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/CacheableService.java
index 5de383fcdc..0bcaed01b9 100644
--- a/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/CacheableService.java
+++ b/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/CacheableService.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 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,8 +93,6 @@ public interface CacheableService<T> {
 
 	TestEntity putRefersToResult(TestEntity arg1);
 
-	TestEntity putRefersToNullResult(TestEntity arg1);
-
-	TestEntity putRefersToNullResultWithUnless(TestEntity arg1);
+	TestEntity putEvaluatesUnlessBeforeKey(TestEntity arg1);
 
 }
diff --git a/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/DefaultCacheableService.java b/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/DefaultCacheableService.java
index 86408b8d5b..87275a13d1 100644
--- a/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/DefaultCacheableService.java
+++ b/spring-context/src/testFixtures/java/org/springframework/context/testfixture/cache/beans/DefaultCacheableService.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 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.
@@ -244,14 +244,9 @@ public class DefaultCacheableService implements CacheableService<Long> {
 	}
 
 	@Override
-	@CachePut(cacheNames = "primary", key = "#result.id")
-	public TestEntity putRefersToNullResult(TestEntity arg1) {
-		return null;
+	@CachePut(cacheNames = "primary", key = "#result.id", unless = "#result == null")
+	public TestEntity putEvaluatesUnlessBeforeKey(TestEntity arg1) {
+		return (arg1.getId() != Long.MIN_VALUE ? arg1 : null);
 	}
 
-	@Override
-	@CachePut(cacheNames = "primary", key = "#result.id", unless = "#result == null")
-	public TestEntity putRefersToNullResultWithUnless(TestEntity arg1) {
-		return null;
-	}
 }