Skip to content

Commit 3e29697

Browse files
Dave Syerphilwebb
authored andcommitted
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
1 parent 39682b7 commit 3e29697

File tree

3 files changed

+113
-10
lines changed

3 files changed

+113
-10
lines changed

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

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import org.apache.commons.logging.Log;
4242
import org.apache.commons.logging.LogFactory;
4343
import org.springframework.aop.Advisor;
44+
import org.springframework.aop.AopInvocationException;
4445
import org.springframework.aop.PointcutAdvisor;
4546
import org.springframework.aop.RawTargetAccess;
4647
import org.springframework.aop.TargetSource;
@@ -72,6 +73,7 @@
7273
* @author Juergen Hoeller
7374
* @author Ramnivas Laddad
7475
* @author Chris Beams
76+
* @author Dave Syer
7577
* @see org.springframework.cglib.proxy.Enhancer
7678
* @see AdvisedSupport#setProxyTargetClass
7779
* @see DefaultAopProxyFactory
@@ -330,9 +332,10 @@ private Callback[] getCallbacks(Class<?> rootClass) throws Exception {
330332
}
331333

332334
/**
333-
* Wrap a return of this if necessary to be the proxy
335+
* Process a return value. Wraps a return of {@code this} if necessary to be the
336+
* {@code proxy} and also verifies that {@code null} is not returned as a primitive.
334337
*/
335-
private static Object massageReturnTypeIfNecessary(Object proxy, Object target, Method method, Object retVal) {
338+
private static Object processReturnType(Object proxy, Object target, Method method, Object retVal) {
336339
// Massage return value if necessary
337340
if (retVal != null && retVal == target &&
338341
!RawTargetAccess.class.isAssignableFrom(method.getDeclaringClass())) {
@@ -341,6 +344,10 @@ private static Object massageReturnTypeIfNecessary(Object proxy, Object target,
341344
// to itself in another returned object.
342345
retVal = proxy;
343346
}
347+
Class<?> returnType = method.getReturnType();
348+
if (retVal == null && returnType != Void.TYPE && returnType.isPrimitive()) {
349+
throw new AopInvocationException("Null return value from advice does not match primitive return type for: " + method);
350+
}
344351
return retVal;
345352
}
346353

@@ -381,7 +388,7 @@ public StaticUnadvisedInterceptor(Object target) {
381388

382389
public Object intercept(Object proxy, Method method, Object[] args, MethodProxy methodProxy) throws Throwable {
383390
Object retVal = methodProxy.invoke(this.target, args);
384-
return massageReturnTypeIfNecessary(proxy, this.target, method, retVal);
391+
return processReturnType(proxy, this.target, method, retVal);
385392
}
386393
}
387394

@@ -403,7 +410,7 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
403410
try {
404411
oldProxy = AopContext.setCurrentProxy(proxy);
405412
Object retVal = methodProxy.invoke(this.target, args);
406-
return massageReturnTypeIfNecessary(proxy, this.target, method, retVal);
413+
return processReturnType(proxy, this.target, method, retVal);
407414
}
408415
finally {
409416
AopContext.setCurrentProxy(oldProxy);
@@ -429,7 +436,7 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
429436
Object target = this.targetSource.getTarget();
430437
try {
431438
Object retVal = methodProxy.invoke(target, args);
432-
return massageReturnTypeIfNecessary(proxy, target, method, retVal);
439+
return processReturnType(proxy, target, method, retVal);
433440
}
434441
finally {
435442
this.targetSource.releaseTarget(target);
@@ -455,7 +462,7 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
455462
try {
456463
oldProxy = AopContext.setCurrentProxy(proxy);
457464
Object retVal = methodProxy.invoke(target, args);
458-
return massageReturnTypeIfNecessary(proxy, target, method, retVal);
465+
return processReturnType(proxy, target, method, retVal);
459466
}
460467
finally {
461468
AopContext.setCurrentProxy(oldProxy);
@@ -573,7 +580,7 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
573580
this.targetClass, this.adviceChain, methodProxy);
574581
// If we get here, we need to create a MethodInvocation.
575582
Object retVal = invocation.proceed();
576-
retVal = massageReturnTypeIfNecessary(proxy, this.target, method, retVal);
583+
retVal = processReturnType(proxy, this.target, method, retVal);
577584
return retVal;
578585
}
579586
}
@@ -623,7 +630,7 @@ public Object intercept(Object proxy, Method method, Object[] args, MethodProxy
623630
// We need to create a method invocation...
624631
retVal = new CglibMethodInvocation(proxy, target, method, args, targetClass, chain, methodProxy).proceed();
625632
}
626-
retVal = massageReturnTypeIfNecessary(proxy, target, method, retVal);
633+
retVal = processReturnType(proxy, target, method, retVal);
627634
return retVal;
628635
}
629636
finally {

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2010 the original author or authors.
2+
* Copyright 2002-2012 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -26,6 +26,7 @@
2626
import org.apache.commons.logging.Log;
2727
import org.apache.commons.logging.LogFactory;
2828

29+
import org.springframework.aop.AopInvocationException;
2930
import org.springframework.aop.RawTargetAccess;
3031
import org.springframework.aop.TargetSource;
3132
import org.springframework.aop.support.AopUtils;
@@ -53,6 +54,7 @@
5354
* @author Rod Johnson
5455
* @author Juergen Hoeller
5556
* @author Rob Harrop
57+
* @author Dave Syer
5658
* @see java.lang.reflect.Proxy
5759
* @see AdvisedSupport
5860
* @see ProxyFactory
@@ -203,12 +205,15 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
203205
}
204206

205207
// Massage return value if necessary.
206-
if (retVal != null && retVal == target && method.getReturnType().isInstance(proxy) &&
208+
Class<?> returnType = method.getReturnType();
209+
if (retVal != null && retVal == target && returnType.isInstance(proxy) &&
207210
!RawTargetAccess.class.isAssignableFrom(method.getDeclaringClass())) {
208211
// Special case: it returned "this" and the return type of the method
209212
// is type-compatible. Note that we can't help if the target sets
210213
// a reference to itself in another returned object.
211214
retVal = proxy;
215+
} else if (retVal == null && returnType != Void.TYPE && returnType.isPrimitive()) {
216+
throw new AopInvocationException("Null return value from advice does not match primitive return type for: " + method);
212217
}
213218
return retVal;
214219
}
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/*
2+
* Copyright 2002-2012 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.aop.framework;
18+
19+
import static org.junit.Assert.assertEquals;
20+
21+
import org.aopalliance.intercept.MethodInterceptor;
22+
import org.aopalliance.intercept.MethodInvocation;
23+
import org.junit.Rule;
24+
import org.junit.Test;
25+
import org.junit.rules.ExpectedException;
26+
import org.springframework.aop.AopInvocationException;
27+
28+
/**
29+
* Test for SPR-4675. A null value returned from around advice is very hard to debug if
30+
* the caller expects a primitive.
31+
*
32+
* @author Dave Syer
33+
*/
34+
public class NullPrimitiveTests {
35+
36+
@Rule
37+
public ExpectedException thrown = ExpectedException.none();
38+
39+
static interface Foo {
40+
int getValue();
41+
}
42+
43+
@Test
44+
public void testNullPrimitiveWithJdkProxy() {
45+
46+
class SimpleFoo implements Foo {
47+
public int getValue() {
48+
return 100;
49+
}
50+
}
51+
52+
SimpleFoo target = new SimpleFoo();
53+
ProxyFactory factory = new ProxyFactory(target);
54+
factory.addAdvice(new MethodInterceptor() {
55+
public Object invoke(MethodInvocation invocation) throws Throwable {
56+
return null;
57+
}
58+
});
59+
60+
Foo foo = (Foo) factory.getProxy();
61+
62+
thrown.expect(AopInvocationException.class);
63+
thrown.expectMessage("Foo.getValue()");
64+
assertEquals(0, foo.getValue());
65+
}
66+
67+
public static class Bar {
68+
public int getValue() {
69+
return 100;
70+
}
71+
}
72+
73+
@Test
74+
public void testNullPrimitiveWithCglibProxy() {
75+
76+
Bar target = new Bar();
77+
ProxyFactory factory = new ProxyFactory(target);
78+
factory.addAdvice(new MethodInterceptor() {
79+
public Object invoke(MethodInvocation invocation) throws Throwable {
80+
return null;
81+
}
82+
});
83+
84+
Bar bar = (Bar) factory.getProxy();
85+
86+
thrown.expect(AopInvocationException.class);
87+
thrown.expectMessage("Bar.getValue()");
88+
assertEquals(0, bar.getValue());
89+
}
90+
91+
}

0 commit comments

Comments
 (0)