Consistent target method resolution for event and caching expressions

Issue: SPR-16779

spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java

@@ -17,6 +17,7 @@
 package org.springframework.cache.interceptor;
 
 import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -30,6 +31,7 @@ import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
 import org.springframework.aop.framework.AopProxyUtils;
+import org.springframework.aop.support.AopUtils;
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryAware;
 import org.springframework.beans.factory.InitializingBean;
@@ -40,6 +42,7 @@ import org.springframework.beans.factory.annotation.BeanFactoryAnnotationUtils;
 import org.springframework.cache.Cache;
 import org.springframework.cache.CacheManager;
 import org.springframework.context.expression.AnnotatedElementKey;
+import org.springframework.core.BridgeMethodResolver;
 import org.springframework.expression.EvaluationContext;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
@@ -624,6 +627,10 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 
 		private final Class<?> targetClass;
 
+		private final Method targetMethod;
+
+		private final AnnotatedElementKey methodKey;
+
 		private final KeyGenerator keyGenerator;
 
 		private final CacheResolver cacheResolver;
@@ -632,8 +639,11 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 				KeyGenerator keyGenerator, CacheResolver cacheResolver) {
 
 			this.operation = operation;
-			this.method = method;
+			this.method = BridgeMethodResolver.findBridgedMethod(method);
 			this.targetClass = targetClass;
+			this.targetMethod = (!Proxy.isProxyClass(targetClass) ?
+					AopUtils.getMostSpecificMethod(method, targetClass) : this.method);
+			this.methodKey = new AnnotatedElementKey(this.targetMethod, targetClass);
 			this.keyGenerator = keyGenerator;
 			this.cacheResolver = cacheResolver;
 		}
@@ -652,15 +662,12 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 
 		private final Collection<String> cacheNames;
 
-		private final AnnotatedElementKey methodCacheKey;
-
 		public CacheOperationContext(CacheOperationMetadata metadata, Object[] args, Object target) {
 			this.metadata = metadata;
 			this.args = extractArgs(metadata.method, args);
 			this.target = target;
 			this.caches = CacheAspectSupport.this.getCaches(this, metadata.cacheResolver);
 			this.cacheNames = createCacheNames(this.caches);
-			this.methodCacheKey = new AnnotatedElementKey(metadata.method, metadata.targetClass);
 		}
 
 		@Override
@@ -698,7 +705,7 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 			if (StringUtils.hasText(this.metadata.operation.getCondition())) {
 				EvaluationContext evaluationContext = createEvaluationContext(result);
 				return evaluator.condition(this.metadata.operation.getCondition(),
-						this.methodCacheKey, evaluationContext);
+						this.metadata.methodKey, evaluationContext);
 			}
 			return true;
 		}
@@ -713,7 +720,7 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 			}
 			if (StringUtils.hasText(unless)) {
 				EvaluationContext evaluationContext = createEvaluationContext(value);
-				return !evaluator.unless(unless, this.methodCacheKey, evaluationContext);
+				return !evaluator.unless(unless, this.metadata.methodKey, evaluationContext);
 			}
 			return true;
 		}
@@ -725,14 +732,14 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 		protected Object generateKey(@Nullable Object result) {
 			if (StringUtils.hasText(this.metadata.operation.getKey())) {
 				EvaluationContext evaluationContext = createEvaluationContext(result);
-				return evaluator.key(this.metadata.operation.getKey(), this.methodCacheKey, evaluationContext);
+				return evaluator.key(this.metadata.operation.getKey(), this.metadata.methodKey, evaluationContext);
 			}
 			return this.metadata.keyGenerator.generate(this.target, this.metadata.method, this.args);
 		}
 
 		private EvaluationContext createEvaluationContext(@Nullable Object result) {
 			return evaluator.createEvaluationContext(this.caches, this.metadata.method, this.args,
-					this.target, this.metadata.targetClass, result, beanFactory);
+					this.target, this.metadata.targetClass, this.metadata.targetMethod, result, beanFactory);
 		}
 
 		protected Collection<? extends Cache> getCaches() {

spring-context/src/main/java/org/springframework/cache/interceptor/CacheExpressionRootObject.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2018 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.
@@ -20,7 +20,6 @@ import java.lang.reflect.Method;
 import java.util.Collection;
 
 import org.springframework.cache.Cache;
-import org.springframework.util.Assert;
 
 /**
  * Class describing the root object used during the expression evaluation.
@@ -45,8 +44,6 @@ class CacheExpressionRootObject {
 	public CacheExpressionRootObject(
 			Collection<? extends Cache> caches, Method method, Object[] args, Object target, Class<?> targetClass) {
 
-		Assert.notNull(method, "Method is required");
-		Assert.notNull(targetClass, "targetClass is required");
 		this.method = method;
 		this.target = target;
 		this.targetClass = targetClass;

spring-context/src/main/java/org/springframework/cache/interceptor/CacheOperationExpressionEvaluator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 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.
@@ -21,7 +21,6 @@ import java.util.Collection;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
-import org.springframework.aop.support.AopUtils;
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.cache.Cache;
 import org.springframework.context.expression.AnnotatedElementKey;
@@ -68,18 +67,6 @@ class CacheOperationExpressionEvaluator extends CachedExpressionEvaluator {
 
 	private final Map<ExpressionKey, Expression> unlessCache = new ConcurrentHashMap<>(64);
 
-	private final Map<AnnotatedElementKey, Method> targetMethodCache = new ConcurrentHashMap<>(64);
-
-
-	/**
-	 * Create an {@link EvaluationContext} without a return value.
-	 * @see #createEvaluationContext(Collection, Method, Object[], Object, Class, Object, BeanFactory)
-	 */
-	public EvaluationContext createEvaluationContext(Collection<? extends Cache> caches,
-			Method method, Object[] args, Object target, Class<?> targetClass, BeanFactory beanFactory) {
-
-		return createEvaluationContext(caches, method, args, target, targetClass, NO_RESULT, beanFactory);
-	}
 
 	/**
 	 * Create an {@link EvaluationContext}.
@@ -93,12 +80,11 @@ class CacheOperationExpressionEvaluator extends CachedExpressionEvaluator {
 	 * @return the evaluation context
 	 */
 	public EvaluationContext createEvaluationContext(Collection<? extends Cache> caches,
-			Method method, Object[] args, Object target, Class<?> targetClass, @Nullable Object result,
-			@Nullable BeanFactory beanFactory) {
+			Method method, Object[] args, Object target, Class<?> targetClass, Method targetMethod,
+			@Nullable Object result, @Nullable BeanFactory beanFactory) {
 
 		CacheExpressionRootObject rootObject = new CacheExpressionRootObject(
 				caches, method, args, target, targetClass);
-		Method targetMethod = getTargetMethod(targetClass, method);
 		CacheEvaluationContext evaluationContext = new CacheEvaluationContext(
 				rootObject, targetMethod, args, getParameterNameDiscoverer());
 		if (result == RESULT_UNAVAILABLE) {
@@ -135,18 +121,6 @@ class CacheOperationExpressionEvaluator extends CachedExpressionEvaluator {
 		this.keyCache.clear();
 		this.conditionCache.clear();
 		this.unlessCache.clear();
-		this.targetMethodCache.clear();
-	}
-
-	private Method getTargetMethod(Class<?> targetClass, Method method) {
-		AnnotatedElementKey methodKey = new AnnotatedElementKey(method, targetClass);
-		Method targetMethod = this.targetMethodCache.get(methodKey);
-		if (targetMethod == null) {
-			targetMethod = AopUtils.getMostSpecificMethod(method, targetClass);
-			this.targetMethodCache.put(methodKey, targetMethod);
 	}
-		return targetMethod;
-	}
-
 
 }

spring-context/src/main/java/org/springframework/context/event/ApplicationListenerMethodAdapter.java

@@ -18,6 +18,7 @@ package org.springframework.context.event;
 
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
 import java.lang.reflect.UndeclaredThrowableException;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -27,6 +28,7 @@ import java.util.List;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
+import org.springframework.aop.support.AopUtils;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationEvent;
 import org.springframework.context.PayloadApplicationEvent;
@@ -35,10 +37,8 @@ import org.springframework.core.BridgeMethodResolver;
 import org.springframework.core.ResolvableType;
 import org.springframework.core.annotation.AnnotatedElementUtils;
 import org.springframework.core.annotation.Order;
-import org.springframework.expression.EvaluationContext;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
-import org.springframework.util.ClassUtils;
 import org.springframework.util.ObjectUtils;
 import org.springframework.util.ReflectionUtils;
 import org.springframework.util.StringUtils;
@@ -66,9 +66,9 @@ public class ApplicationListenerMethodAdapter implements GenericApplicationListe
 
 	private final Method method;
 
-	private final Class<?> targetClass;
+	private final Method targetMethod;
 
-	private final Method bridgedMethod;
+	private final AnnotatedElementKey methodKey;
 
 	private final List<ResolvableType> declaredEventTypes;
 
@@ -77,8 +77,6 @@ public class ApplicationListenerMethodAdapter implements GenericApplicationListe
 
 	private final int order;
 
-	private final AnnotatedElementKey methodKey;
-
 	@Nullable
 	private ApplicationContext applicationContext;
 
@@ -88,18 +86,15 @@ public class ApplicationListenerMethodAdapter implements GenericApplicationListe
 
 	public ApplicationListenerMethodAdapter(String beanName, Class<?> targetClass, Method method) {
 		this.beanName = beanName;
-		this.method = method;
-		this.targetClass = targetClass;
-		this.bridgedMethod = BridgeMethodResolver.findBridgedMethod(method);
-
-		Method targetMethod = ClassUtils.getMostSpecificMethod(method, targetClass);
-		EventListener ann = AnnotatedElementUtils.findMergedAnnotation(targetMethod, EventListener.class);
+		this.method = BridgeMethodResolver.findBridgedMethod(method);
+		this.targetMethod = (!Proxy.isProxyClass(targetClass) ?
+				AopUtils.getMostSpecificMethod(method, targetClass) : this.method);
+		this.methodKey = new AnnotatedElementKey(this.targetMethod, targetClass);
 
+		EventListener ann = AnnotatedElementUtils.findMergedAnnotation(this.targetMethod, EventListener.class);
 		this.declaredEventTypes = resolveDeclaredEventTypes(method, ann);
 		this.condition = (ann != null ? ann.condition() : null);
 		this.order = resolveOrder(method);
-
-		this.methodKey = new AnnotatedElementKey(method, targetClass);
 	}
 
 
@@ -109,21 +104,24 @@ public class ApplicationListenerMethodAdapter implements GenericApplicationListe
 			throw new IllegalStateException(
 					"Maximum one parameter is allowed for event listener method: " + method);
 		}
-		if (ann != null && ann.classes().length > 0) {
-			List<ResolvableType> types = new ArrayList<>(ann.classes().length);
-			for (Class<?> eventType : ann.classes()) {
+
+		if (ann != null) {
+			Class<?>[] classes = ann.classes();
+			if (classes.length > 0) {
+				List<ResolvableType> types = new ArrayList<>(classes.length);
+				for (Class<?> eventType : classes) {
 					types.add(ResolvableType.forClass(eventType));
 				}
 				return types;
 			}
-		else {
+		}
+
 		if (count == 0) {
 			throw new IllegalStateException(
 					"Event parameter is mandatory for event listener method: " + method);
 		}
 		return Collections.singletonList(ResolvableType.forMethodParameter(method, 0));
 	}
-	}
 
 	private int resolveOrder(Method method) {
 		Order ann = AnnotatedElementUtils.findMergedAnnotation(method, Order.class);
@@ -245,10 +243,9 @@ public class ApplicationListenerMethodAdapter implements GenericApplicationListe
 		}
 		String condition = getCondition();
 		if (StringUtils.hasText(condition)) {
-			Assert.notNull(this.evaluator, "EventExpressionEvaluator must no be null");
-			EvaluationContext evaluationContext = this.evaluator.createEvaluationContext(
-					event, this.targetClass, this.method, args, this.applicationContext);
-			return this.evaluator.condition(condition, this.methodKey, evaluationContext);
+			Assert.notNull(this.evaluator, "EventExpressionEvaluator must not be null");
+			return this.evaluator.condition(
+					condition, event, this.targetMethod, this.methodKey, args, this.applicationContext);
 		}
 		return true;
 	}
@@ -259,12 +256,12 @@ public class ApplicationListenerMethodAdapter implements GenericApplicationListe
 	@Nullable
 	protected Object doInvoke(Object... args) {
 		Object bean = getTargetBean();
-		ReflectionUtils.makeAccessible(this.bridgedMethod);
+		ReflectionUtils.makeAccessible(this.method);
 		try {
-			return this.bridgedMethod.invoke(bean, args);
+			return this.method.invoke(bean, args);
 		}
 		catch (IllegalArgumentException ex) {
-			assertTargetBean(this.bridgedMethod, bean, args);
+			assertTargetBean(this.method, bean, args);
 			throw new IllegalStateException(getInvocationErrorMessage(bean, ex.getMessage(), args), ex);
 		}
 		catch (IllegalAccessException ex) {
@@ -311,7 +308,7 @@ public class ApplicationListenerMethodAdapter implements GenericApplicationListe
 		StringBuilder sb = new StringBuilder(message).append("\n");
 		sb.append("HandlerMethod details: \n");
 		sb.append("Bean [").append(bean.getClass().getName()).append("]\n");
-		sb.append("Method [").append(this.bridgedMethod.toGenericString()).append("]\n");
+		sb.append("Method [").append(this.method.toGenericString()).append("]\n");
 		return sb.toString();
 	}
 

spring-context/src/main/java/org/springframework/context/event/EventExpressionEvaluator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 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.
@@ -20,14 +20,12 @@ import java.lang.reflect.Method;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
-import org.springframework.aop.support.AopUtils;
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.context.ApplicationEvent;
 import org.springframework.context.expression.AnnotatedElementKey;
 import org.springframework.context.expression.BeanFactoryResolver;
 import org.springframework.context.expression.CachedExpressionEvaluator;
 import org.springframework.context.expression.MethodBasedEvaluationContext;
-import org.springframework.expression.EvaluationContext;
 import org.springframework.expression.Expression;
 import org.springframework.lang.Nullable;
 
@@ -43,42 +41,22 @@ class EventExpressionEvaluator extends CachedExpressionEvaluator {
 
 	private final Map<ExpressionKey, Expression> conditionCache = new ConcurrentHashMap<>(64);
 
-	private final Map<AnnotatedElementKey, Method> targetMethodCache = new ConcurrentHashMap<>(64);
-
 
 	/**
-	 * Create the suitable {@link EvaluationContext} for the specified event handling
-	 * on the specified method.
+	 * Specify if the condition defined by the specified expression matches.
 	 */
-	public EvaluationContext createEvaluationContext(ApplicationEvent event, Class<?> targetClass,
-			Method method, Object[] args, @Nullable BeanFactory beanFactory) {
+	public boolean condition(String conditionExpression, ApplicationEvent event, Method targetMethod,
+			AnnotatedElementKey methodKey, Object[] args, @Nullable BeanFactory beanFactory) {
 
-		Method targetMethod = getTargetMethod(targetClass, method);
 		EventExpressionRootObject root = new EventExpressionRootObject(event, args);
 		MethodBasedEvaluationContext evaluationContext = new MethodBasedEvaluationContext(
 				root, targetMethod, args, getParameterNameDiscoverer());
 		if (beanFactory != null) {
 			evaluationContext.setBeanResolver(new BeanFactoryResolver(beanFactory));
 		}
-		return evaluationContext;
-	}
 
-	/**
-	 * Specify if the condition defined by the specified expression matches.
-	 */
-	public boolean condition(String conditionExpression, AnnotatedElementKey elementKey, EvaluationContext evalContext) {
-		return (Boolean.TRUE.equals(getExpression(this.conditionCache, elementKey, conditionExpression).getValue(
-				evalContext, Boolean.class)));
-	}
-
-	private Method getTargetMethod(Class<?> targetClass, Method method) {
-		AnnotatedElementKey methodKey = new AnnotatedElementKey(method, targetClass);
-		Method targetMethod = this.targetMethodCache.get(methodKey);
-		if (targetMethod == null) {
-			targetMethod = AopUtils.getMostSpecificMethod(method, targetClass);
-			this.targetMethodCache.put(methodKey, targetMethod);
-		}
-		return targetMethod;
+		return (Boolean.TRUE.equals(getExpression(this.conditionCache, methodKey, conditionExpression).getValue(
+				evaluationContext, Boolean.class)));
 	}
 
 }

spring-context/src/test/java/org/springframework/cache/interceptor/ExpressionEvaluatorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2018 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.
@@ -59,7 +59,7 @@ public class ExpressionEvaluatorTests {
 
 
 	@Test
-	public void testMultipleCachingSource() throws Exception {
+	public void testMultipleCachingSource() {
 		Collection<CacheOperation> ops = getOps("multipleCaching");
 		assertEquals(2, ops.size());
 		Iterator<CacheOperation> it = ops.iterator();
@@ -74,19 +74,18 @@ public class ExpressionEvaluatorTests {
 	}
 
 	@Test
-	public void testMultipleCachingEval() throws Exception {
+	public void testMultipleCachingEval() {
 		AnnotatedClass target = new AnnotatedClass();
-		Method method = ReflectionUtils.findMethod(AnnotatedClass.class, "multipleCaching", Object.class,
-				Object.class);
+		Method method = ReflectionUtils.findMethod(
+				AnnotatedClass.class, "multipleCaching", Object.class, Object.class);
 		Object[] args = new Object[] {new Object(), new Object()};
 		Collection<ConcurrentMapCache> caches = Collections.singleton(new ConcurrentMapCache("test"));
 
 		EvaluationContext evalCtx = this.eval.createEvaluationContext(caches, method, args,
-				target, target.getClass(), null);
+				target, target.getClass(), method, CacheOperationExpressionEvaluator.NO_RESULT, null);
 		Collection<CacheOperation> ops = getOps("multipleCaching");
 
 		Iterator<CacheOperation> it = ops.iterator();
-
 		AnnotatedElementKey key = new AnnotatedElementKey(method, AnnotatedClass.class);
 
 		Object keyA = this.eval.key(it.next().getKey(), key, evalCtx);
@@ -97,28 +96,28 @@ public class ExpressionEvaluatorTests {
 	}
 
 	@Test
-	public void withReturnValue() throws Exception {
+	public void withReturnValue() {
 		EvaluationContext context = createEvaluationContext("theResult");
 		Object value = new SpelExpressionParser().parseExpression("#result").getValue(context);
 		assertThat(value, equalTo("theResult"));
 	}
 
 	@Test
-	public void withNullReturn() throws Exception {
+	public void withNullReturn() {
 		EvaluationContext context = createEvaluationContext(null);
 		Object value = new SpelExpressionParser().parseExpression("#result").getValue(context);
 		assertThat(value, nullValue());
 	}
 
 	@Test
-	public void withoutReturnValue() throws Exception {
+	public void withoutReturnValue() {
 		EvaluationContext context = createEvaluationContext(CacheOperationExpressionEvaluator.NO_RESULT);
 		Object value = new SpelExpressionParser().parseExpression("#result").getValue(context);
 		assertThat(value, nullValue());
 	}
 
 	@Test
-	public void unavailableReturnValue() throws Exception {
+	public void unavailableReturnValue() {
 		EvaluationContext context = createEvaluationContext(CacheOperationExpressionEvaluator.RESULT_UNAVAILABLE);
 		try {
 			new SpelExpressionParser().parseExpression("#result").getValue(context);
@@ -130,7 +129,7 @@ public class ExpressionEvaluatorTests {
 	}
 
 	@Test
-	public void resolveBeanReference() throws Exception {
+	public void resolveBeanReference() {
 		StaticApplicationContext applicationContext = new StaticApplicationContext();
 		BeanDefinition beanDefinition = new RootBeanDefinition(String.class);
 		applicationContext.registerBeanDefinition("myBean", beanDefinition);
@@ -147,11 +146,12 @@ public class ExpressionEvaluatorTests {
 
 	private EvaluationContext createEvaluationContext(Object result, BeanFactory beanFactory) {
 		AnnotatedClass target = new AnnotatedClass();
-		Method method = ReflectionUtils.findMethod(AnnotatedClass.class, "multipleCaching", Object.class,
-				Object.class);
+		Method method = ReflectionUtils.findMethod(
+				AnnotatedClass.class, "multipleCaching", Object.class, Object.class);
 		Object[] args = new Object[] {new Object(), new Object()};
 		Collection<ConcurrentMapCache> caches = Collections.singleton(new ConcurrentMapCache("test"));
-		return this.eval.createEvaluationContext(caches, method, args, target, target.getClass(), result, beanFactory);
+		return this.eval.createEvaluationContext(
+				caches, method, args, target, target.getClass(), method, result, beanFactory);
 	}