Allow @Cacheable method to return Optional

This commit further refines 240f254 to also support java.util.Optional
for synchronized cache access (i.e. when the `sync` attribute on
`@Cacheable` is set to `true`).

Issue: SPR-14853

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

@@ -40,7 +40,6 @@ import org.springframework.beans.factory.SmartInitializingSingleton;
 import org.springframework.beans.factory.annotation.BeanFactoryAnnotationUtils;
 import org.springframework.cache.Cache;
 import org.springframework.cache.CacheManager;
-import org.springframework.context.ApplicationContext;
 import org.springframework.context.expression.AnnotatedElementKey;
 import org.springframework.expression.EvaluationContext;
 import org.springframework.lang.UsesJava8;
@@ -361,12 +360,12 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 				Object key = generateKey(context, CacheOperationExpressionEvaluator.NO_RESULT);
 				Cache cache = context.getCaches().iterator().next();
 				try {
-					return cache.get(key, new Callable<Object>() {
+					return wrapCacheValue(method, cache.get(key, new Callable<Object>() {
 						@Override
 						public Object call() throws Exception {
-							return invokeOperation(invoker);
+							return unwrapReturnValue(invokeOperation(invoker));
 						}
-					});
+					}));
 				}
 				catch (Cache.ValueRetrievalException ex) {
 					// The invoker wraps any Throwable in a ThrowableWrapper instance so we
@@ -401,23 +400,12 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 		if (cacheHit != null && cachePutRequests.isEmpty() && !hasCachePut(contexts)) {
 			// If there are no put requests, just use the cache hit
 			cacheValue = cacheHit.get();
-			if (method.getReturnType() == javaUtilOptionalClass &&
+			returnValue = wrapCacheValue(method, cacheValue);
-					(cacheValue == null || cacheValue.getClass() != javaUtilOptionalClass)) {
-				returnValue = OptionalUnwrapper.wrap(cacheValue);
-			}
-			else {
-				returnValue = cacheValue;
-			}
 		}
 		else {
 			// Invoke the method if we don't have a cache hit
 			returnValue = invokeOperation(invoker);
-			if (returnValue != null && returnValue.getClass() == javaUtilOptionalClass) {
+			cacheValue = unwrapReturnValue(returnValue);
-				cacheValue = OptionalUnwrapper.unwrap(returnValue);
-			}
-			else {
-				cacheValue = returnValue;
-			}
 		}
 
 		// Collect any explicit @CachePuts
@@ -434,6 +422,21 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 		return returnValue;
 	}
 
+	private Object wrapCacheValue(Method method, Object cacheValue) {
+		if (method.getReturnType() == Optional.class &&
+				(cacheValue == null || cacheValue.getClass() != Optional.class)) {
+			return Optional.ofNullable(cacheValue);
+		}
+		return cacheValue;
+	}
+
+	private Object unwrapReturnValue(Object returnValue) {
+		if (returnValue != null && returnValue.getClass() == javaUtilOptionalClass) {
+			return OptionalUnwrapper.unwrap(returnValue);
+		}
+		return returnValue;
+	}
+
 	private boolean hasCachePut(CacheOperationContexts contexts) {
 		// Evaluate the conditions *without* the result object because we don't have it yet...
 		Collection<CacheOperationContext> cachePutContexts = contexts.get(CachePutOperation.class);

spring-context/src/test/java/org/springframework/cache/CacheReproTests.java

@@ -150,6 +150,22 @@ public class CacheReproTests {
 		assertSame(tb2, cache.get("tb1").get());
 	}
 
+	@Test
+	public void spr14853AdaptsToOptionalWithSync() {
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr14853Config.class);
+		Spr14853Service bean = context.getBean(Spr14853Service.class);
+		Cache cache = context.getBean(CacheManager.class).getCache("itemCache");
+
+		TestBean tb = new TestBean("tb1");
+		bean.insertItem(tb);
+		assertSame(tb, bean.findById("tb1").get());
+		assertSame(tb, cache.get("tb1").get());
+
+		cache.clear();
+		TestBean tb2 = bean.findById("tb1").get();
+		assertNotSame(tb, tb2);
+		assertSame(tb2, cache.get("tb1").get());
+	}
 
 	@Configuration
 	@EnableCaching
@@ -342,4 +358,33 @@ public class CacheReproTests {
 		}
 	}
 
+	public static class Spr14853Service {
+
+		@Cacheable(value = "itemCache", sync = true)
+		public Optional<TestBean> findById(String id) {
+			return Optional.of(new TestBean(id));
+		}
+
+		@CachePut(cacheNames = "itemCache", key = "#item.name")
+		public TestBean insertItem(TestBean item) {
+			return item;
+		}
+
+	}
+
+	@Configuration
+	@EnableCaching
+	public static class Spr14853Config {
+
+		@Bean
+		public CacheManager cacheManager() {
+			return new ConcurrentMapCacheManager();
+		}
+
+		@Bean
+		public Spr14853Service service() {
+			return new Spr14853Service();
+		}
+	}
+
 }