Spring/spring-framework
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #99 from dsyer/SPR-4675

Browse Source 
* SPR-4675:
  Throw on advise returning null for primitive type
  Upgrade to Hamcrest 1.3
This commit is contained in:
Phillip Webb
2012-10-30 16:11:00 -07:00
parent 22dfe93f2f 3e296974c4
commit 1c7e8cfbf6
5 changed files with 117 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
build.gradle
View File

@@ -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"
}

23
spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java
View File

@@ -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 {

9
spring-aop/src/main/java/org/springframework/aop/framework/JdkDynamicAopProxy.java
View File

@@ -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;
}

91
spring-aop/src/test/java/org/springframework/aop/framework/NullPrimitiveTests.java Normal file
View File

@@ -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());
}
}

6
spring-test-mvc/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/StatusAssertionTests.java
View File

@@ -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<Integer> matcher = allOf(greaterThanOrEqualTo(400), lessThan(500));
this.mockMvc.perform(get("/badRequest")).andExpect(status().is(matcher));
}
@Test