Merge pull request #99 from dsyer/SPR-4675

* SPR-4675:
  Throw on advise returning null for primitive type
  Upgrade to Hamcrest 1.3

Author: philwebb

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

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

@@ -41,6 +41,7 @@
 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 @@
  * @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 @@ private Callback[] getCallbacks(Class<?> rootClass) throws Exception {
 	}
 
 	/**
-	 * 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 @@ private static Object massageReturnTypeIfNecessary(Object proxy, Object target,
 			// 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 @@ public StaticUnadvisedInterceptor(Object target) {
 
 		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 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
 			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 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
 			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 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
 			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 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
 					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 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
 					// 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 {

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.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 @@
  * @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 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
 			}
 
 			// 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;
 		}

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

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.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 void testHttpStatus() throws Exception {
 		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