Skip to content

Commit 9b2f9e6

Browse files
committed
CglibAopProxy logs explicit warning for interface-implementing method marked as final
Issue: SPR-15436 (cherry picked from commit 0d0b879)
1 parent 6c370ed commit 9b2f9e6

File tree

1 file changed

+58
-50
lines changed

1 file changed

+58
-50
lines changed

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

Lines changed: 58 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.HashMap;
2424
import java.util.List;
2525
import java.util.Map;
26+
import java.util.Set;
2627
import java.util.WeakHashMap;
2728

2829
import org.aopalliance.aop.Advice;
@@ -56,9 +57,6 @@
5657
/**
5758
* CGLIB-based {@link AopProxy} implementation for the Spring AOP framework.
5859
*
59-
* <p>Formerly named {@code Cglib2AopProxy}, as of Spring 3.2, this class depends on
60-
* Spring's own internally repackaged version of CGLIB 3.</i>.
61-
*
6260
* <p>Objects of this type should be obtained through proxy factories,
6361
* configured by an {@link AdvisedSupport} object. This class is internal
6462
* to Spring's AOP framework and need not be used directly by client code.
@@ -241,10 +239,11 @@ protected Enhancer createEnhancer() {
241239
* validates it if not.
242240
*/
243241
private void validateClassIfNecessary(Class<?> proxySuperClass, ClassLoader proxyClassLoader) {
244-
if (logger.isInfoEnabled()) {
242+
if (logger.isWarnEnabled()) {
245243
synchronized (validatedClasses) {
246244
if (!validatedClasses.containsKey(proxySuperClass)) {
247-
doValidateClass(proxySuperClass, proxyClassLoader);
245+
doValidateClass(proxySuperClass, proxyClassLoader,
246+
ClassUtils.getAllInterfacesForClassAsSet(proxySuperClass));
248247
validatedClasses.put(proxySuperClass, Boolean.TRUE);
249248
}
250249
}
@@ -255,30 +254,35 @@ private void validateClassIfNecessary(Class<?> proxySuperClass, ClassLoader prox
255254
* Checks for final methods on the given {@code Class}, as well as package-visible
256255
* methods across ClassLoaders, and writes warnings to the log for each one found.
257256
*/
258-
private void doValidateClass(Class<?> proxySuperClass, ClassLoader proxyClassLoader) {
257+
private void doValidateClass(Class<?> proxySuperClass, ClassLoader proxyClassLoader, Set<Class<?>> ifcs) {
259258
if (proxySuperClass != Object.class) {
260259
Method[] methods = proxySuperClass.getDeclaredMethods();
261260
for (Method method : methods) {
262261
int mod = method.getModifiers();
263262
if (!Modifier.isStatic(mod)) {
264263
if (Modifier.isFinal(mod)) {
265-
logger.info("Unable to proxy method [" + method + "] because it is final: " +
266-
"All calls to this method via a proxy will NOT be routed to the target instance.");
264+
if (implementsInterface(method, ifcs)) {
265+
logger.warn("Unable to proxy interface-implmenting method [" + method + "] because " +
266+
"it is marked as final: Consider using interface-based proxies instead!");
267+
}
268+
logger.info("Final method [" + method + "] cannot get proxied via CGLIB: " +
269+
"Calls to this method will NOT be routed to the target instance and " +
270+
"might lead to NPEs against uninitialized fields in the proxy instance.");
267271
}
268272
else if (!Modifier.isPublic(mod) && !Modifier.isProtected(mod) && !Modifier.isPrivate(mod) &&
269273
proxyClassLoader != null && proxySuperClass.getClassLoader() != proxyClassLoader) {
270-
logger.info("Unable to proxy method [" + method + "] because it is package-visible " +
271-
"across different ClassLoaders: All calls to this method via a proxy will " +
272-
"NOT be routed to the target instance.");
274+
logger.info("Method [" + method + "] is package-visible across different ClassLoaders " +
275+
"and cannot get proxied via CGLIB: Declare this method as public or protected " +
276+
"if you need to support invocations through the proxy.");
273277
}
274278
}
275279
}
276-
doValidateClass(proxySuperClass.getSuperclass(), proxyClassLoader);
280+
doValidateClass(proxySuperClass.getSuperclass(), proxyClassLoader, ifcs);
277281
}
278282
}
279283

280284
private Callback[] getCallbacks(Class<?> rootClass) throws Exception {
281-
// Parameters used for optimisation choices...
285+
// Parameters used for optimization choices...
282286
boolean exposeProxy = this.advised.isExposeProxy();
283287
boolean isFrozen = this.advised.isFrozen();
284288
boolean isStatic = this.advised.getTargetSource().isStatic();
@@ -317,14 +321,14 @@ private Callback[] getCallbacks(Class<?> rootClass) throws Exception {
317321
Callback[] callbacks;
318322

319323
// If the target is a static one and the advice chain is frozen,
320-
// then we can make some optimisations by sending the AOP calls
324+
// then we can make some optimizations by sending the AOP calls
321325
// direct to the target using the fixed chain for that method.
322326
if (isStatic && isFrozen) {
323327
Method[] methods = rootClass.getMethods();
324328
Callback[] fixedCallbacks = new Callback[methods.length];
325329
this.fixedInterceptorMap = new HashMap<String, Integer>(methods.length);
326330

327-
// TODO: small memory optimisation here (can skip creation for methods with no advice)
331+
// TODO: small memory optimization here (can skip creation for methods with no advice)
328332
for (int x = 0; x < methods.length; x++) {
329333
List<Object> chain = this.advised.getInterceptorsAndDynamicInterceptionAdvice(methods[x], rootClass);
330334
fixedCallbacks[x] = new FixedChainStaticTargetInterceptor(
@@ -345,6 +349,31 @@ private Callback[] getCallbacks(Class<?> rootClass) throws Exception {
345349
return callbacks;
346350
}
347351

352+
353+
@Override
354+
public boolean equals(Object other) {
355+
return (this == other || (other instanceof CglibAopProxy &&
356+
AopProxyUtils.equalsInProxy(this.advised, ((CglibAopProxy) other).advised)));
357+
}
358+
359+
@Override
360+
public int hashCode() {
361+
return CglibAopProxy.class.hashCode() * 13 + this.advised.getTargetSource().hashCode();
362+
}
363+
364+
365+
/**
366+
* Check whether the given method is declared on any of the given interfaces.
367+
*/
368+
private static boolean implementsInterface(Method method, Set<Class<?>> ifcs) {
369+
for (Class<?> ifc : ifcs) {
370+
if (ClassUtils.hasMethod(ifc, method.getName(), method.getParameterTypes())) {
371+
return true;
372+
}
373+
}
374+
return false;
375+
}
376+
348377
/**
349378
* Process a return value. Wraps a return of {@code this} if necessary to be the
350379
* {@code proxy} and also verifies that {@code null} is not returned as a primitive.
@@ -366,18 +395,6 @@ private static Object processReturnType(Object proxy, Object target, Method meth
366395
}
367396

368397

369-
@Override
370-
public boolean equals(Object other) {
371-
return (this == other || (other instanceof CglibAopProxy &&
372-
AopProxyUtils.equalsInProxy(this.advised, ((CglibAopProxy) other).advised)));
373-
}
374-
375-
@Override
376-
public int hashCode() {
377-
return CglibAopProxy.class.hashCode() * 13 + this.advised.getTargetSource().hashCode();
378-
}
379-
380-
381398
/**
382399
* Serializable replacement for CGLIB's NoOp interface.
383400
* Public to allow use elsewhere in the framework.
@@ -823,51 +840,42 @@ public int accept(Method method) {
823840
// Else use the AOP_PROXY.
824841
if (isStatic && isFrozen && this.fixedInterceptorMap.containsKey(key)) {
825842
if (logger.isDebugEnabled()) {
826-
logger.debug("Method has advice and optimisations are enabled: " + method);
843+
logger.debug("Method has advice and optimizations are enabled: " + method);
827844
}
828-
// We know that we are optimising so we can use the FixedStaticChainInterceptors.
845+
// We know that we are optimizing so we can use the FixedStaticChainInterceptors.
829846
int index = this.fixedInterceptorMap.get(key);
830847
return (index + this.fixedInterceptorOffset);
831848
}
832849
else {
833850
if (logger.isDebugEnabled()) {
834-
logger.debug("Unable to apply any optimisations to advised method: " + method);
851+
logger.debug("Unable to apply any optimizations to advised method: " + method);
835852
}
836853
return AOP_PROXY;
837854
}
838855
}
839856
else {
840-
// See if the return type of the method is outside the class hierarchy
841-
// of the target type. If so we know it never needs to have return type
842-
// massage and can use a dispatcher.
843-
// If the proxy is being exposed, then must use the interceptor the
844-
// correct one is already configured. If the target is not static, then
845-
// cannot use a dispatcher because the target cannot be released.
857+
// See if the return type of the method is outside the class hierarchy of the target type.
858+
// If so we know it never needs to have return type massage and can use a dispatcher.
859+
// If the proxy is being exposed, then must use the interceptor the correct one is already
860+
// configured. If the target is not static, then we cannot use a dispatcher because the
861+
// target needs to be explicitly released after the invocation.
846862
if (exposeProxy || !isStatic) {
847863
return INVOKE_TARGET;
848864
}
849865
Class<?> returnType = method.getReturnType();
850-
if (targetClass == returnType) {
866+
if (returnType.isAssignableFrom(targetClass)) {
851867
if (logger.isDebugEnabled()) {
852-
logger.debug("Method " + method +
853-
"has return type same as target type (may return this) - using INVOKE_TARGET");
868+
logger.debug("Method return type is assignable from target type and " +
869+
"may therefore return 'this' - using INVOKE_TARGET: " + method);
854870
}
855871
return INVOKE_TARGET;
856872
}
857-
else if (returnType.isPrimitive() || !returnType.isAssignableFrom(targetClass)) {
858-
if (logger.isDebugEnabled()) {
859-
logger.debug("Method " + method +
860-
" has return type that ensures this cannot be returned- using DISPATCH_TARGET");
861-
}
862-
return DISPATCH_TARGET;
863-
}
864873
else {
865874
if (logger.isDebugEnabled()) {
866-
logger.debug("Method " + method +
867-
"has return type that is assignable from the target type (may return this) - " +
868-
"using INVOKE_TARGET");
875+
logger.debug("Method return type ensures 'this' cannot be returned - " +
876+
"using DISPATCH_TARGET: " + method);
869877
}
870-
return INVOKE_TARGET;
878+
return DISPATCH_TARGET;
871879
}
872880
}
873881
}

0 commit comments

Comments
 (0)