From 470ea977e1126eb0318ca46f0f4f02553042a8c7 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 23 Dec 2015 20:47:28 +0100 Subject: [PATCH] Consistent validation of annotated methods behind AOP proxies Issue: SPR-13816 --- .../springframework/aop/support/AopUtils.java | 26 +++++++++++++++ .../event/EventListenerMethodProcessor.java | 3 +- .../ScheduledAnnotationBeanPostProcessor.java | 33 ++----------------- .../AnnotationDrivenEventListenerTests.java | 22 +++++++++++++ .../core/MethodIntrospector.java | 8 +++-- ...msListenerAnnotationBeanPostProcessor.java | 2 +- 6 files changed, 59 insertions(+), 35 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/support/AopUtils.java b/spring-aop/src/main/java/org/springframework/aop/support/AopUtils.java index 1ed373caf6..184b8ef7e2 100644 --- a/spring-aop/src/main/java/org/springframework/aop/support/AopUtils.java +++ b/spring-aop/src/main/java/org/springframework/aop/support/AopUtils.java @@ -18,6 +18,7 @@ package org.springframework.aop.support; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.lang.reflect.Proxy; import java.util.LinkedHashSet; import java.util.LinkedList; @@ -34,6 +35,7 @@ import org.springframework.aop.PointcutAdvisor; import org.springframework.aop.SpringProxy; import org.springframework.aop.TargetClassAware; import org.springframework.core.BridgeMethodResolver; +import org.springframework.core.MethodIntrospector; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; @@ -112,6 +114,30 @@ public abstract class AopUtils { return result; } + /** + * Select an invocable method on the target type: either the given method itself + * if actually exposed on the target type, or otherwise a corresponding method + * on one of the target type's interfaces or on the target type itself. + * @param method the method to check + * @param targetType the target type to search methods on (typically an AOP proxy) + * @return a corresponding invocable method on the target type + * @throws IllegalStateException if the given method is not invocable on the given + * target type (typically due to a proxy mismatch) + * @since 4.3 + * @see MethodIntrospector#selectInvocableMethod(Method, Class) + */ + public static Method selectInvocableMethod(Method method, Class targetType) { + Method methodToUse = MethodIntrospector.selectInvocableMethod(method, targetType); + if (Modifier.isPrivate(methodToUse.getModifiers()) && !Modifier.isStatic(methodToUse.getModifiers()) && + SpringProxy.class.isAssignableFrom(targetType)) { + throw new IllegalStateException(String.format( + "Need to invoke method '%s' found on proxy for target class '%s' but cannot " + + "be delegated to target bean. Switch its visibility to package or protected.", + method.getName(), method.getDeclaringClass().getSimpleName())); + } + return methodToUse; + } + /** * Determine whether the given method is an "equals" method. * @see java.lang.Object#equals diff --git a/spring-context/src/main/java/org/springframework/context/event/EventListenerMethodProcessor.java b/spring-context/src/main/java/org/springframework/context/event/EventListenerMethodProcessor.java index d551d509e5..108ee54b22 100644 --- a/spring-context/src/main/java/org/springframework/context/event/EventListenerMethodProcessor.java +++ b/spring-context/src/main/java/org/springframework/context/event/EventListenerMethodProcessor.java @@ -30,6 +30,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.aop.framework.autoproxy.AutoProxyUtils; import org.springframework.aop.scope.ScopedObject; import org.springframework.aop.scope.ScopedProxyUtils; +import org.springframework.aop.support.AopUtils; import org.springframework.beans.BeansException; import org.springframework.beans.factory.BeanInitializationException; import org.springframework.beans.factory.SmartInitializingSingleton; @@ -142,7 +143,7 @@ public class EventListenerMethodProcessor implements SmartInitializingSingleton, for (Method method : annotatedMethods.keySet()) { for (EventListenerFactory factory : factories) { if (factory.supportsMethod(method)) { - Method methodToUse = MethodIntrospector.selectInvocableMethod( + Method methodToUse = AopUtils.selectInvocableMethod( method, this.applicationContext.getType(beanName)); ApplicationListener applicationListener = factory.createApplicationListener(beanName, targetType, methodToUse); diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java index a0ac5d481b..9aa2c9c7ad 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java @@ -17,7 +17,6 @@ package org.springframework.scheduling.annotation; import java.lang.reflect.Method; -import java.lang.reflect.Modifier; import java.util.Collections; import java.util.Map; import java.util.Set; @@ -53,7 +52,6 @@ import org.springframework.scheduling.config.ScheduledTaskRegistrar; import org.springframework.scheduling.support.CronTrigger; import org.springframework.scheduling.support.ScheduledMethodRunnable; import org.springframework.util.Assert; -import org.springframework.util.ReflectionUtils; import org.springframework.util.StringUtils; import org.springframework.util.StringValueResolver; @@ -274,35 +272,8 @@ public class ScheduledAnnotationBeanPostProcessor implements BeanPostProcessor, Assert.isTrue(method.getParameterTypes().length == 0, "Only no-arg methods may be annotated with @Scheduled"); - if (AopUtils.isJdkDynamicProxy(bean)) { - try { - // Found a @Scheduled method on the target class for this JDK proxy -> - // is it also present on the proxy itself? - method = bean.getClass().getMethod(method.getName(), method.getParameterTypes()); - } - catch (SecurityException ex) { - ReflectionUtils.handleReflectionException(ex); - } - catch (NoSuchMethodException ex) { - throw new IllegalStateException(String.format( - "@Scheduled method '%s' found on bean target class '%s' but not " + - "found in any interface(s) for a dynamic proxy. Either pull the " + - "method up to a declared interface or switch to subclass (CGLIB) " + - "proxies by setting proxy-target-class/proxyTargetClass to 'true'.", - method.getName(), method.getDeclaringClass().getSimpleName())); - } - } - else if (AopUtils.isCglibProxy(bean)) { - // Common problem: private methods end up in the proxy instance, not getting delegated. - if (Modifier.isPrivate(method.getModifiers())) { - throw new IllegalStateException(String.format( - "@Scheduled method '%s' found on CGLIB proxy for target class '%s' but cannot " + - "be delegated to target bean. Switch its visibility to package or protected.", - method.getName(), method.getDeclaringClass().getSimpleName())); - } - } - - Runnable runnable = new ScheduledMethodRunnable(bean, method); + Method invocableMethod = AopUtils.selectInvocableMethod(method, bean.getClass()); + Runnable runnable = new ScheduledMethodRunnable(bean, invocableMethod); boolean processedSchedule = false; String errorMessage = "Exactly one of the 'cron', 'fixedDelay(String)', or 'fixedRate(String)' attributes is required"; diff --git a/spring-context/src/test/java/org/springframework/context/event/AnnotationDrivenEventListenerTests.java b/spring-context/src/test/java/org/springframework/context/event/AnnotationDrivenEventListenerTests.java index 7cb3ac00e3..18a9a97cbb 100644 --- a/spring-context/src/test/java/org/springframework/context/event/AnnotationDrivenEventListenerTests.java +++ b/spring-context/src/test/java/org/springframework/context/event/AnnotationDrivenEventListenerTests.java @@ -284,6 +284,17 @@ public class AnnotationDrivenEventListenerTests { this.eventCollector.assertTotalEventsCount(1); } + @Test + public void privateMethodOnCglibProxyFails() throws Exception { + try { + load(CglibProxyWithPrivateMethod.class); + fail("Should have thrown BeanInitializationException"); + } + catch (BeanInitializationException ex) { + assertTrue(ex.getCause() instanceof IllegalStateException); + } + } + @Test public void eventListenerWorksWithCustomScope() throws Exception { load(CustomScopeTestBean.class); @@ -790,6 +801,17 @@ public class AnnotationDrivenEventListenerTests { } + @Component + @Scope(proxyMode = ScopedProxyMode.TARGET_CLASS) + static class CglibProxyWithPrivateMethod extends AbstractTestEventListener { + + @EventListener + private void handleIt(TestEvent event) { + collectEvent(event); + } + } + + @Component @Scope(scopeName = "custom", proxyMode = ScopedProxyMode.TARGET_CLASS) static class CustomScopeTestBean extends AbstractTestEventListener { diff --git a/spring-core/src/main/java/org/springframework/core/MethodIntrospector.java b/spring-core/src/main/java/org/springframework/core/MethodIntrospector.java index d402c10b6d..5a2b2df765 100644 --- a/spring-core/src/main/java/org/springframework/core/MethodIntrospector.java +++ b/spring-core/src/main/java/org/springframework/core/MethodIntrospector.java @@ -111,22 +111,26 @@ public abstract class MethodIntrospector { * @param targetType the target type to search methods on * (typically an interface-based JDK proxy) * @return a corresponding invocable method on the target type + * @throws IllegalStateException if the given method is not invocable on the given + * target type (typically due to a proxy mismatch) */ public static Method selectInvocableMethod(Method method, Class targetType) { if (method.getDeclaringClass().isAssignableFrom(targetType)) { return method; } try { + String methodName = method.getName(); + Class[] parameterTypes = method.getParameterTypes(); for (Class ifc : targetType.getInterfaces()) { try { - return ifc.getMethod(method.getName(), method.getParameterTypes()); + return ifc.getMethod(methodName, parameterTypes); } catch (NoSuchMethodException ex) { // Alright, not on this interface then... } } // A final desperate attempt on the proxy class itself... - return targetType.getMethod(method.getName(), method.getParameterTypes()); + return targetType.getMethod(methodName, parameterTypes); } catch (NoSuchMethodException ex) { throw new IllegalStateException(String.format( diff --git a/spring-jms/src/main/java/org/springframework/jms/annotation/JmsListenerAnnotationBeanPostProcessor.java b/spring-jms/src/main/java/org/springframework/jms/annotation/JmsListenerAnnotationBeanPostProcessor.java index bda5965ed6..fa9b61b25b 100644 --- a/spring-jms/src/main/java/org/springframework/jms/annotation/JmsListenerAnnotationBeanPostProcessor.java +++ b/spring-jms/src/main/java/org/springframework/jms/annotation/JmsListenerAnnotationBeanPostProcessor.java @@ -236,7 +236,7 @@ public class JmsListenerAnnotationBeanPostProcessor * @see JmsListenerEndpointRegistrar#registerEndpoint */ protected void processJmsListener(JmsListener jmsListener, Method mostSpecificMethod, Object bean) { - Method invocableMethod = MethodIntrospector.selectInvocableMethod(mostSpecificMethod, bean.getClass()); + Method invocableMethod = AopUtils.selectInvocableMethod(mostSpecificMethod, bean.getClass()); MethodJmsListenerEndpoint endpoint = createMethodJmsListenerEndpoint(); endpoint.setBean(bean);