From d9de19d7b31c684fe65f51233a70ada6ecc15221 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Fri, 2 Sep 2011 15:37:42 +0000 Subject: [PATCH] SPR-8653 + refactor a bit the internals of CacheAspect to allow invocations that do not throw any exceptions (AspectJ) --- .../cache/aspectj/AbstractCacheAspect.aj | 11 ++--- .../cache/aspectj/AbstractAnnotationTest.java | 45 ++++++++++++++++++- .../cache/aspectj/AspectJAnnotationTest.java | 29 ------------ .../AnnotatedClassCacheableService.java | 8 ++++ .../cache/config/CacheableService.java | 4 ++ .../cache/config/DefaultCacheableService.java | 10 +++++ .../cache/interceptor/CacheAspectSupport.java | 16 ++++--- .../cache/interceptor/CacheInterceptor.java | 30 ++++++++----- .../interceptor/CacheProxyFactoryBean.java | 20 ++++++++- .../cache/config/AbstractAnnotationTests.java | 43 ++++++++++++++++++ .../AnnotatedClassCacheableService.java | 8 ++++ .../cache/config/CacheableService.java | 4 ++ .../cache/config/DefaultCacheableService.java | 10 +++++ 13 files changed, 180 insertions(+), 58 deletions(-) delete mode 100644 org.springframework.aspects/src/test/java/org/springframework/cache/aspectj/AspectJAnnotationTest.java diff --git a/org.springframework.aspects/src/main/java/org/springframework/cache/aspectj/AbstractCacheAspect.aj b/org.springframework.aspects/src/main/java/org/springframework/cache/aspectj/AbstractCacheAspect.aj index e90d943ba7..7955fcdcbd 100644 --- a/org.springframework.aspects/src/main/java/org/springframework/cache/aspectj/AbstractCacheAspect.aj +++ b/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 ajInvocation = new Callable() { - 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()); } /** diff --git a/org.springframework.aspects/src/test/java/org/springframework/cache/aspectj/AbstractAnnotationTest.java b/org.springframework.aspects/src/test/java/org/springframework/cache/aspectj/AbstractAnnotationTest.java index 934a97fff5..8650d5b116 100644 --- a/org.springframework.aspects/src/test/java/org/springframework/cache/aspectj/AbstractAnnotationTest.java +++ b/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); + } + } \ No newline at end of file diff --git a/org.springframework.aspects/src/test/java/org/springframework/cache/aspectj/AspectJAnnotationTest.java b/org.springframework.aspects/src/test/java/org/springframework/cache/aspectj/AspectJAnnotationTest.java deleted file mode 100644 index 23fb0ab2db..0000000000 --- a/org.springframework.aspects/src/test/java/org/springframework/cache/aspectj/AspectJAnnotationTest.java +++ /dev/null @@ -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"; - } -} diff --git a/org.springframework.aspects/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java b/org.springframework.aspects/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java index 431fada220..96f2410bd2 100644 --- a/org.springframework.aspects/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java +++ b/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(); + } } diff --git a/org.springframework.aspects/src/test/java/org/springframework/cache/config/CacheableService.java b/org.springframework.aspects/src/test/java/org/springframework/cache/config/CacheableService.java index 5ec9b0c155..80338a6de0 100644 --- a/org.springframework.aspects/src/test/java/org/springframework/cache/config/CacheableService.java +++ b/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 key(Object arg1, Object arg2); + T throwChecked(Object arg1) throws Exception; + + T throwUnchecked(Object arg1); } \ No newline at end of file diff --git a/org.springframework.aspects/src/test/java/org/springframework/cache/config/DefaultCacheableService.java b/org.springframework.aspects/src/test/java/org/springframework/cache/config/DefaultCacheableService.java index 5a1e149057..45a2c2c4df 100644 --- a/org.springframework.aspects/src/test/java/org/springframework/cache/config/DefaultCacheableService.java +++ b/org.springframework.aspects/src/test/java/org/springframework/cache/config/DefaultCacheableService.java @@ -49,4 +49,14 @@ public class DefaultCacheableService implements CacheableService { 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()); + } } \ No newline at end of file diff --git a/org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java b/org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java index 0a057e59aa..a090dcaed8 100644 --- a/org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java +++ b/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 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(); } diff --git a/org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheInterceptor.java b/org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheInterceptor.java index 5deefeb725..a6f9836c82 100644 --- a/org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheInterceptor.java +++ b/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 aopAllianceInvocation = new Callable() { - 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; + } } - -} +} \ No newline at end of file diff --git a/org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheProxyFactoryBean.java b/org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheProxyFactoryBean.java index acfca0c2b7..297f4db4df 100644 --- a/org.springframework.context/src/main/java/org/springframework/cache/interceptor/CacheProxyFactoryBean.java +++ b/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(); + } } /** diff --git a/org.springframework.context/src/test/java/org/springframework/cache/config/AbstractAnnotationTests.java b/org.springframework.context/src/test/java/org/springframework/cache/config/AbstractAnnotationTests.java index 933efb8549..cad5e47040 100644 --- a/org.springframework.context/src/test/java/org/springframework/cache/config/AbstractAnnotationTests.java +++ b/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); + } } \ No newline at end of file diff --git a/org.springframework.context/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java b/org.springframework.context/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java index adacf9efd3..66961cc42f 100644 --- a/org.springframework.context/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java +++ b/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(); + } } diff --git a/org.springframework.context/src/test/java/org/springframework/cache/config/CacheableService.java b/org.springframework.context/src/test/java/org/springframework/cache/config/CacheableService.java index a6f3702ead..98f6252547 100644 --- a/org.springframework.context/src/test/java/org/springframework/cache/config/CacheableService.java +++ b/org.springframework.context/src/test/java/org/springframework/cache/config/CacheableService.java @@ -42,4 +42,8 @@ public interface CacheableService { T rootVars(Object arg1); + T throwChecked(Object arg1) throws Exception; + + T throwUnchecked(Object arg1); + } \ No newline at end of file diff --git a/org.springframework.context/src/test/java/org/springframework/cache/config/DefaultCacheableService.java b/org.springframework.context/src/test/java/org/springframework/cache/config/DefaultCacheableService.java index 057806358a..d02248f930 100644 --- a/org.springframework.context/src/test/java/org/springframework/cache/config/DefaultCacheableService.java +++ b/org.springframework.context/src/test/java/org/springframework/cache/config/DefaultCacheableService.java @@ -73,4 +73,14 @@ public class DefaultCacheableService implements CacheableService { 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()); + } } \ No newline at end of file