SPR-8653

+ refactor a bit the internals of CacheAspect to allow invocations that do not throw any exceptions (AspectJ)

org.springframework.aspects/src/main/java/org/springframework/cache/aspectj/AbstractCacheAspect.aj

@@ -24,6 +24,7 @@ import org.aspectj.lang.annotation.SuppressAjWarnings;
 import org.aspectj.lang.reflect.MethodSignature;
 import org.springframework.cache.interceptor.CacheAspectSupport;
 import org.springframework.cache.interceptor.CacheOperationSource;
+import org.springframework.cache.interceptor.CacheAspectSupport.Invoker;
 
 /**
  * Abstract superaspect for AspectJ cache aspects. Concrete
@@ -60,17 +61,13 @@ public abstract aspect AbstractCacheAspect extends CacheAspectSupport {
 		MethodSignature methodSignature = (MethodSignature) thisJoinPoint.getSignature();
 		Method method = methodSignature.getMethod();
 
-		Callable<Object> ajInvocation = new Callable<Object>() {
-			public Object call() {
+		Invoker aspectJInvoker = new Invoker() {
+			public Object invoke() {
 				return proceed(cachedObject);
 			}
 		};
 
-		try{
-			return execute(ajInvocation, thisJoinPoint.getTarget(), method, thisJoinPoint.getArgs());	
-		} catch (Exception ex){
-			throw new RuntimeException("Cannot cache target ", ex);
-		}
+		return execute(aspectJInvoker, thisJoinPoint.getTarget(), method, thisJoinPoint.getArgs());	
 	}
 
 	/**

org.springframework.aspects/src/test/java/org/springframework/cache/aspectj/AbstractAnnotationTest.java

@@ -16,8 +16,9 @@
 
 package org.springframework.cache.aspectj;
 
-import static org.junit.Assert.assertNotSame;
-import static org.junit.Assert.assertSame;
+import static org.junit.Assert.*;
+
+import java.util.UUID;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -97,6 +98,26 @@ public abstract class AbstractAnnotationTest {
 		assertNotSame(r3, r4);
 	}
 
+	public void testCheckedThrowable(CacheableService service) throws Exception {
+		String arg = UUID.randomUUID().toString();
+		try {
+			service.throwChecked(arg);
+			fail("Excepted exception");
+		} catch (Exception ex) {
+			assertEquals(arg, ex.getMessage());
+		}
+	}
+
+	public void testUncheckedThrowable(CacheableService service) throws Exception {
+		try {
+			service.throwUnchecked(Long.valueOf(1));
+			fail("Excepted exception");
+		} catch (RuntimeException ex) {
+			assertTrue(ex instanceof UnsupportedOperationException);
+			// expected
+		}
+	}
+
 	@Test
 	public void testCacheable() throws Exception {
 		testCacheable(cs);
@@ -127,4 +148,24 @@ public abstract class AbstractAnnotationTest {
 		testInvalidate(ccs);
 	}
 
+	@Test
+	public void testCheckedException() throws Exception {
+		testCheckedThrowable(cs);
+	}
+
+	@Test
+	public void testClassCheckedException() throws Exception {
+		testCheckedThrowable(ccs);
+	}
+
+	@Test
+	public void testUncheckedException() throws Exception {
+		testUncheckedThrowable(cs);
+	}
+
+	@Test
+	public void testClassUncheckedException() throws Exception {
+		testUncheckedThrowable(ccs);
+	}
+
 }

org.springframework.aspects/src/test/java/org/springframework/cache/aspectj/AspectJAnnotationTest.java

@@ -1,29 +0,0 @@
-/*
- * Copyright 2010 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.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.springframework.cache.aspectj;
-
-
-/**
- * @author Costin Leau
- */
-public class AspectJAnnotationTest extends AbstractAnnotationTest {
-
-	@Override
-	protected String getConfig() {
-		return "/org/springframework/cache/config/annotation-cache-aspectj.xml";
-	}
-}

org.springframework.aspects/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java

@@ -45,4 +45,12 @@ public class AnnotatedClassCacheableService implements CacheableService {
 	public Object key(Object arg1, Object arg2) {
 		return counter.getAndIncrement();
 	}
+
+	public Long throwChecked(Object arg1) throws Exception {
+		throw new UnsupportedOperationException(arg1.toString());
+	}
+
+	public Long throwUnchecked(Object arg1) {
+		throw new UnsupportedOperationException();
+	}
 }

org.springframework.aspects/src/test/java/org/springframework/cache/config/CacheableService.java

@@ -16,6 +16,7 @@
 
 package org.springframework.cache.config;
 
+
 /**
  * Basic service interface.
  * 
@@ -31,4 +32,7 @@ public interface CacheableService<T> {
 
 	T key(Object arg1, Object arg2);
 
+	T throwChecked(Object arg1) throws Exception;
+
+	T throwUnchecked(Object arg1);
 }

org.springframework.aspects/src/test/java/org/springframework/cache/config/DefaultCacheableService.java

@@ -49,4 +49,14 @@ public class DefaultCacheableService implements CacheableService<Long> {
 	public Long key(Object arg1, Object arg2) {
 		return counter.getAndIncrement();
 	}
+
+	@Cacheable("default")
+	public Long throwChecked(Object arg1) throws Exception {
+		throw new Exception(arg1.toString());
+	}
+
+	@Cacheable("default")
+	public Long throwUnchecked(Object arg1) {
+		throw new UnsupportedOperationException(arg1.toString());
+	}
 }

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

@@ -21,11 +21,9 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.Set;
-import java.util.concurrent.Callable;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-
 import org.springframework.aop.framework.AopProxyUtils;
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.cache.Cache;
@@ -57,6 +55,10 @@ import org.springframework.util.StringUtils;
  */
 public abstract class CacheAspectSupport implements InitializingBean {
 
+	public interface Invoker {
+		Object invoke();
+	}
+
 	protected final Log logger = LogFactory.getLog(getClass());
 
 	private CacheManager cacheManager;
@@ -171,11 +173,11 @@ public abstract class CacheAspectSupport implements InitializingBean {
 		return new CacheOperationContext(operation, method, args, target, targetClass);
 	}
 
-	protected Object execute(Callable<Object> invocation, Object target, Method method, Object[] args) throws Exception {
+	protected Object execute(Invoker invoker, Object target, Method method, Object[] args) {
 		// check whether aspect is enabled
 		// to cope with cases where the AJ is pulled in automatically
 		if (!this.initialized) {
-			return invocation.call();
+			return invoker.invoke();
 		}
 
 		boolean log = logger.isTraceEnabled();
@@ -224,7 +226,7 @@ public abstract class CacheAspectSupport implements InitializingBean {
 							logger.trace("Key " + key + " NOT found in cache(s), invoking cached target method  "
 									+ method);
 						}
-						retVal = invocation.call();
+						retVal = invoker.invoke();
 						// update all caches
 						for (Cache cache : caches) {
 							cache.put(key, retVal);
@@ -239,7 +241,6 @@ public abstract class CacheAspectSupport implements InitializingBean {
 
 				if (cacheOp instanceof CacheEvictOperation) {
 					CacheEvictOperation evictOp = (CacheEvictOperation) cacheOp;
-					retVal = invocation.call();
 
 					// for each cache
 					// lazy key initialization
@@ -266,6 +267,7 @@ public abstract class CacheAspectSupport implements InitializingBean {
 							cache.evict(key);
 						}
 					}
+					retVal = invoker.invoke();
 				}
 				return retVal;
 			}
@@ -276,7 +278,7 @@ public abstract class CacheAspectSupport implements InitializingBean {
 			}
 		}
 
-		return invocation.call();
+		return invoker.invoke();
 	}
 
 

org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheInterceptor.java

@@ -18,13 +18,10 @@ package org.springframework.cache.interceptor;
 
 import java.io.Serializable;
 import java.lang.reflect.Method;
-import java.util.concurrent.Callable;
 
 import org.aopalliance.intercept.MethodInterceptor;
 import org.aopalliance.intercept.MethodInvocation;
 
-import org.springframework.util.ReflectionUtils;
-
 /**
  * AOP Alliance MethodInterceptor for declarative cache
  * management using the common Spring caching infrastructure
@@ -44,22 +41,31 @@ import org.springframework.util.ReflectionUtils;
 @SuppressWarnings("serial")
 public class CacheInterceptor extends CacheAspectSupport implements MethodInterceptor, Serializable {
 
+	private static class ThrowableWrapper extends RuntimeException {
+		private final Throwable original;
+
+		ThrowableWrapper(Throwable original) {
+			this.original = original;
+		}
+	}
+
 	public Object invoke(final MethodInvocation invocation) throws Throwable {
 		Method method = invocation.getMethod();
 
-		Callable<Object> aopAllianceInvocation = new Callable<Object>() {
-			public Object call() throws Exception {
+		Invoker aopAllianceInvoker = new Invoker() {
+			public Object invoke() {
 				try {
 					return invocation.proceed();
-				}
-				catch (Throwable ex) {
-					ReflectionUtils.rethrowException(ex);
-					return null;
+				} catch (Throwable ex) {
+					throw new ThrowableWrapper(ex);
 				}
 			}
 		};
 
-		return execute(aopAllianceInvocation, invocation.getThis(), method, invocation.getArguments());
+		try {
+			return execute(aopAllianceInvoker, invocation.getThis(), method, invocation.getArguments());
+		} catch (ThrowableWrapper th) {
+			throw th.original;
+		}
 	}
-
-}
+}

org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheProxyFactoryBean.java

@@ -18,6 +18,7 @@ package org.springframework.cache.interceptor;
 
 import org.springframework.aop.Pointcut;
 import org.springframework.aop.framework.AbstractSingletonProxyFactoryBean;
+import org.springframework.aop.support.DefaultPointcutAdvisor;
 
 /**
  * Proxy factory bean for simplified declarative caching handling.
@@ -38,9 +39,26 @@ public class CacheProxyFactoryBean extends AbstractSingletonProxyFactoryBean {
 	private final CacheInterceptor cachingInterceptor = new CacheInterceptor();
 	private Pointcut pointcut;
 
+	/**
+	 * Set a pointcut, i.e a bean that can cause conditional invocation
+	 * of the CacheInterceptor depending on method and attributes passed.
+	 * Note: Additional interceptors are always invoked.
+	 * @see #setPreInterceptors
+	 * @see #setPostInterceptors
+	 */
+	public void setPointcut(Pointcut pointcut) {
+		this.pointcut = pointcut;
+	}
+
 	@Override
 	protected Object createMainInterceptor() {
-		return null;
+		this.cachingInterceptor.afterPropertiesSet();
+		if (this.pointcut != null) {
+			return new DefaultPointcutAdvisor(this.pointcut, this.cachingInterceptor);
+		} else {
+			// Rely on default pointcut.
+			throw new UnsupportedOperationException();
+		}
 	}
 
 	/**

org.springframework.context/src/test/java/org/springframework/cache/config/AbstractAnnotationTests.java

@@ -18,6 +18,8 @@ package org.springframework.cache.config;
 
 import static org.junit.Assert.*;
 
+import java.util.UUID;
+
 import org.junit.Before;
 import org.junit.Test;
 import org.springframework.aop.framework.AopProxyUtils;
@@ -148,6 +150,27 @@ public abstract class AbstractAnnotationTests {
 		assertNotNull(cache.get(expectedKey));
 	}
 
+	public void testCheckedThrowable(CacheableService service) throws Exception {
+		String arg = UUID.randomUUID().toString();
+		try {
+			service.throwChecked(arg);
+			fail("Excepted exception");
+		} catch (Exception ex) {
+			assertEquals(arg, ex.getMessage());
+		}
+	}
+
+	public void testUncheckedThrowable(CacheableService service) throws Exception {
+		try {
+			service.throwUnchecked(Long.valueOf(1));
+			fail("Excepted exception");
+		} catch (RuntimeException ex) {
+			assertTrue("Excepted different exception type and got " + ex.getClass(),
+					ex instanceof UnsupportedOperationException);
+			// expected
+		}
+	}
+
 	public void testNullArg(CacheableService service) {
 		Object r1 = service.cache(null);
 		assertSame(r1, service.cache(null));
@@ -241,4 +264,24 @@ public abstract class AbstractAnnotationTests {
 	public void testClassNullArg() throws Exception {
 		testNullArg(ccs);
 	}
+
+	@Test
+	public void testCheckedException() throws Exception {
+		testCheckedThrowable(cs);
+	}
+
+	@Test
+	public void testClassCheckedException() throws Exception {
+		testCheckedThrowable(ccs);
+	}
+
+	@Test
+	public void testUncheckedException() throws Exception {
+		testUncheckedThrowable(cs);
+	}
+
+	@Test
+	public void testClassUncheckedException() throws Exception {
+		testUncheckedThrowable(ccs);
+	}
 }

org.springframework.context/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java

@@ -69,4 +69,12 @@ public class AnnotatedClassCacheableService implements CacheableService {
 	public Number nullInvocations() {
 		return nullInvocations.get();
 	}
+
+	public Long throwChecked(Object arg1) throws Exception {
+		throw new UnsupportedOperationException(arg1.toString());
+	}
+
+	public Long throwUnchecked(Object arg1) {
+		throw new UnsupportedOperationException();
+	}
 }

org.springframework.context/src/test/java/org/springframework/cache/config/CacheableService.java

@@ -42,4 +42,8 @@ public interface CacheableService<T> {
 
 	T rootVars(Object arg1);
 
+	T throwChecked(Object arg1) throws Exception;
+
+	T throwUnchecked(Object arg1);
+
 }

org.springframework.context/src/test/java/org/springframework/cache/config/DefaultCacheableService.java

@@ -73,4 +73,14 @@ public class DefaultCacheableService implements CacheableService<Long> {
 	public Number nullInvocations() {
 		return nullInvocations.get();
 	}
+
+	@Cacheable("default")
+	public Long throwChecked(Object arg1) throws Exception {
+		throw new Exception(arg1.toString());
+	}
+
+	@Cacheable("default")
+	public Long throwUnchecked(Object arg1) {
+		throw new UnsupportedOperationException(arg1.toString());
+	}
 }