From 39682b7d3f3b81124fb7d88874169d877ad2e6cc Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 30 Oct 2012 15:52:46 -0700 Subject: [PATCH 1/2] Upgrade to Hamcrest 1.3 --- build.gradle | 2 +- .../standalone/resultmatchers/StatusAssertionTests.java | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/build.gradle b/build.gradle index 7c727ded22b..37941ffb206 100644 --- a/build.gradle +++ b/build.gradle @@ -34,7 +34,7 @@ configure(allprojects) { } dependencies { - testCompile "org.hamcrest:hamcrest-all:1.1" + testCompile "org.hamcrest:hamcrest-all:1.3" testCompile "org.easymock:easymock:2.5.1" } diff --git a/spring-test-mvc/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/StatusAssertionTests.java b/spring-test-mvc/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/StatusAssertionTests.java index 2233018309c..96b456b187c 100644 --- a/spring-test-mvc/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/StatusAssertionTests.java +++ b/spring-test-mvc/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/StatusAssertionTests.java @@ -25,6 +25,7 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilder import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import static org.springframework.test.web.servlet.setup.MockMvcBuilders.standaloneSetup; +import org.hamcrest.Matcher; import org.junit.Before; import org.junit.Test; import org.springframework.http.HttpStatus; @@ -60,11 +61,10 @@ public class StatusAssertionTests { this.mockMvc.perform(get("/badRequest")).andExpect(status().isBadRequest()); } - @SuppressWarnings("unchecked") @Test public void testMatcher() throws Exception { - this.mockMvc.perform(get("/badRequest")) - .andExpect(status().is(allOf(greaterThanOrEqualTo(400), lessThan(500)))); + Matcher matcher = allOf(greaterThanOrEqualTo(400), lessThan(500)); + this.mockMvc.perform(get("/badRequest")).andExpect(status().is(matcher)); } @Test From 3e296974c4b4e6b11d97728073c1df77bb9cc8f6 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Tue, 30 Oct 2012 15:54:57 -0700 Subject: [PATCH 2/2] Throw on advise returning null for primitive type Throw an AopInvocationException when an AOP advisor returns null on a method that should return a primitive type. Issue: SPR-4675 --- .../aop/framework/CglibAopProxy.java | 23 +++-- .../aop/framework/JdkDynamicAopProxy.java | 9 +- .../aop/framework/NullPrimitiveTests.java | 91 +++++++++++++++++++ 3 files changed, 113 insertions(+), 10 deletions(-) create mode 100644 spring-aop/src/test/java/org/springframework/aop/framework/NullPrimitiveTests.java diff --git a/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java b/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java index 49bfe5fc2b4..bbf7015e0a6 100644 --- a/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java +++ b/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java @@ -41,6 +41,7 @@ import org.aopalliance.intercept.MethodInvocation; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.aop.Advisor; +import org.springframework.aop.AopInvocationException; import org.springframework.aop.PointcutAdvisor; import org.springframework.aop.RawTargetAccess; import org.springframework.aop.TargetSource; @@ -72,6 +73,7 @@ import org.springframework.util.ObjectUtils; * @author Juergen Hoeller * @author Ramnivas Laddad * @author Chris Beams + * @author Dave Syer * @see org.springframework.cglib.proxy.Enhancer * @see AdvisedSupport#setProxyTargetClass * @see DefaultAopProxyFactory @@ -330,9 +332,10 @@ final class CglibAopProxy implements AopProxy, Serializable { } /** - * Wrap a return of this if necessary to be the proxy + * Process a return value. Wraps a return of {@code this} if necessary to be the + * {@code proxy} and also verifies that {@code null} is not returned as a primitive. */ - private static Object massageReturnTypeIfNecessary(Object proxy, Object target, Method method, Object retVal) { + private static Object processReturnType(Object proxy, Object target, Method method, Object retVal) { // Massage return value if necessary if (retVal != null && retVal == target && !RawTargetAccess.class.isAssignableFrom(method.getDeclaringClass())) { @@ -341,6 +344,10 @@ final class CglibAopProxy implements AopProxy, Serializable { // to itself in another returned object. retVal = proxy; } + Class returnType = method.getReturnType(); + if (retVal == null && returnType != Void.TYPE && returnType.isPrimitive()) { + throw new AopInvocationException("Null return value from advice does not match primitive return type for: " + method); + } return retVal; } @@ -381,7 +388,7 @@ final class CglibAopProxy implements AopProxy, Serializable { public Object intercept(Object proxy, Method method, Object[] args, MethodProxy methodProxy) throws Throwable { Object retVal = methodProxy.invoke(this.target, args); - return massageReturnTypeIfNecessary(proxy, this.target, method, retVal); + return processReturnType(proxy, this.target, method, retVal); } } @@ -403,7 +410,7 @@ final class CglibAopProxy implements AopProxy, Serializable { try { oldProxy = AopContext.setCurrentProxy(proxy); Object retVal = methodProxy.invoke(this.target, args); - return massageReturnTypeIfNecessary(proxy, this.target, method, retVal); + return processReturnType(proxy, this.target, method, retVal); } finally { AopContext.setCurrentProxy(oldProxy); @@ -429,7 +436,7 @@ final class CglibAopProxy implements AopProxy, Serializable { Object target = this.targetSource.getTarget(); try { Object retVal = methodProxy.invoke(target, args); - return massageReturnTypeIfNecessary(proxy, target, method, retVal); + return processReturnType(proxy, target, method, retVal); } finally { this.targetSource.releaseTarget(target); @@ -455,7 +462,7 @@ final class CglibAopProxy implements AopProxy, Serializable { try { oldProxy = AopContext.setCurrentProxy(proxy); Object retVal = methodProxy.invoke(target, args); - return massageReturnTypeIfNecessary(proxy, target, method, retVal); + return processReturnType(proxy, target, method, retVal); } finally { AopContext.setCurrentProxy(oldProxy); @@ -573,7 +580,7 @@ final class CglibAopProxy implements AopProxy, Serializable { this.targetClass, this.adviceChain, methodProxy); // If we get here, we need to create a MethodInvocation. Object retVal = invocation.proceed(); - retVal = massageReturnTypeIfNecessary(proxy, this.target, method, retVal); + retVal = processReturnType(proxy, this.target, method, retVal); return retVal; } } @@ -623,7 +630,7 @@ final class CglibAopProxy implements AopProxy, Serializable { // We need to create a method invocation... retVal = new CglibMethodInvocation(proxy, target, method, args, targetClass, chain, methodProxy).proceed(); } - retVal = massageReturnTypeIfNecessary(proxy, target, method, retVal); + retVal = processReturnType(proxy, target, method, retVal); return retVal; } finally { diff --git a/spring-aop/src/main/java/org/springframework/aop/framework/JdkDynamicAopProxy.java b/spring-aop/src/main/java/org/springframework/aop/framework/JdkDynamicAopProxy.java index 5b2e4992b54..24c0610c08b 100644 --- a/spring-aop/src/main/java/org/springframework/aop/framework/JdkDynamicAopProxy.java +++ b/spring-aop/src/main/java/org/springframework/aop/framework/JdkDynamicAopProxy.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2010 the original author or authors. + * Copyright 2002-2012 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. @@ -26,6 +26,7 @@ import org.aopalliance.intercept.MethodInvocation; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.aop.AopInvocationException; import org.springframework.aop.RawTargetAccess; import org.springframework.aop.TargetSource; import org.springframework.aop.support.AopUtils; @@ -53,6 +54,7 @@ import org.springframework.util.ClassUtils; * @author Rod Johnson * @author Juergen Hoeller * @author Rob Harrop + * @author Dave Syer * @see java.lang.reflect.Proxy * @see AdvisedSupport * @see ProxyFactory @@ -203,12 +205,15 @@ final class JdkDynamicAopProxy implements AopProxy, InvocationHandler, Serializa } // Massage return value if necessary. - if (retVal != null && retVal == target && method.getReturnType().isInstance(proxy) && + Class returnType = method.getReturnType(); + if (retVal != null && retVal == target && returnType.isInstance(proxy) && !RawTargetAccess.class.isAssignableFrom(method.getDeclaringClass())) { // Special case: it returned "this" and the return type of the method // is type-compatible. Note that we can't help if the target sets // a reference to itself in another returned object. retVal = proxy; + } else if (retVal == null && returnType != Void.TYPE && returnType.isPrimitive()) { + throw new AopInvocationException("Null return value from advice does not match primitive return type for: " + method); } return retVal; } diff --git a/spring-aop/src/test/java/org/springframework/aop/framework/NullPrimitiveTests.java b/spring-aop/src/test/java/org/springframework/aop/framework/NullPrimitiveTests.java new file mode 100644 index 00000000000..1879f8a4cd4 --- /dev/null +++ b/spring-aop/src/test/java/org/springframework/aop/framework/NullPrimitiveTests.java @@ -0,0 +1,91 @@ +/* + * Copyright 2002-2012 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.aop.framework; + +import static org.junit.Assert.assertEquals; + +import org.aopalliance.intercept.MethodInterceptor; +import org.aopalliance.intercept.MethodInvocation; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.springframework.aop.AopInvocationException; + +/** + * Test for SPR-4675. A null value returned from around advice is very hard to debug if + * the caller expects a primitive. + * + * @author Dave Syer + */ +public class NullPrimitiveTests { + + @Rule + public ExpectedException thrown = ExpectedException.none(); + + static interface Foo { + int getValue(); + } + + @Test + public void testNullPrimitiveWithJdkProxy() { + + class SimpleFoo implements Foo { + public int getValue() { + return 100; + } + } + + SimpleFoo target = new SimpleFoo(); + ProxyFactory factory = new ProxyFactory(target); + factory.addAdvice(new MethodInterceptor() { + public Object invoke(MethodInvocation invocation) throws Throwable { + return null; + } + }); + + Foo foo = (Foo) factory.getProxy(); + + thrown.expect(AopInvocationException.class); + thrown.expectMessage("Foo.getValue()"); + assertEquals(0, foo.getValue()); + } + + public static class Bar { + public int getValue() { + return 100; + } + } + + @Test + public void testNullPrimitiveWithCglibProxy() { + + Bar target = new Bar(); + ProxyFactory factory = new ProxyFactory(target); + factory.addAdvice(new MethodInterceptor() { + public Object invoke(MethodInvocation invocation) throws Throwable { + return null; + } + }); + + Bar bar = (Bar) factory.getProxy(); + + thrown.expect(AopInvocationException.class); + thrown.expectMessage("Bar.getValue()"); + assertEquals(0, bar.getValue()); + } + +}