fix: make RecoverAnnotationRecoveryHandler deterministic

When multiple @recover candidates exist, choose the most specific and
deterministic handler instead of relying on reflection order.

Signed-off-by: Chih-Yu Huang <[email protected]>

Author: chihyuh3

src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java

@@ -19,8 +19,14 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
+import java.util.Arrays;
+import java.util.List;
+import java.util.ArrayList;
+import java.util.Comparator;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.stream.Collectors;
 
 import org.springframework.classify.SubclassClassifier;
 import org.springframework.core.annotation.AnnotatedElementUtils;
@@ -55,12 +61,13 @@
  * @author Gianluca Medici
  * @author Lijinliang
  * @author Yanming Zhou
+ * @author Chih-Yu Huang
  */
 public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationRecoverer<T> {
 
 	private final SubclassClassifier<Throwable, Method> classifier = new SubclassClassifier<>();
 
-	private final Map<Method, SimpleMetadata> methods = new HashMap<>();
+	private final Map<Method, SimpleMetadata> methods = new LinkedHashMap<>();
 
 	private final Object target;
 
@@ -73,7 +80,8 @@ public RecoverAnnotationRecoveryHandler(Object target, Method method) {
 
 	@Override
 	public T recover(Object[] args, Throwable cause) {
-		Method method = findClosestMatch(args, cause.getClass());
+		Class<? extends Throwable> causeType = (cause == null) ? null : cause.getClass();
+		Method method = findClosestMatch(args, causeType);
 		if (method == null) {
 			throw new ExhaustedRetryException("Cannot locate recovery method", cause);
 		}
@@ -111,48 +119,59 @@ private Method findMethodOnProxy(Method method, Object proxy) {
 		}
 	}
 
-	private Method findClosestMatch(Object[] args, Class<? extends Throwable> cause) {
-		Method result = null;
-
-		if (!StringUtils.hasText(this.recoverMethodName)) {
-			int min = Integer.MAX_VALUE;
-			for (Map.Entry<Method, SimpleMetadata> entry : this.methods.entrySet()) {
-				Method method = entry.getKey();
-				SimpleMetadata meta = entry.getValue();
-				Class<? extends Throwable> type = meta.getType();
-				if (type == null) {
-					type = Throwable.class;
+	private Method findClosestMatch(Object[] args, Class<? extends Throwable> causeType) {
+		List<Method> methodsWithThrowable = new ArrayList<>();
+		List<Method> methodsWithoutThrowable = new ArrayList<>();
+
+		for (Method method : this.methods.keySet()) {
+			SimpleMetadata meta = this.methods.get(method);
+			if (meta.getType() != null) {
+				methodsWithThrowable.add(method);
+			}
+			else {
+				methodsWithoutThrowable.add(method);
+			}
+		}
+
+		Method bestMatch = null;
+
+		if (causeType != null) {
+			int minDistance = Integer.MAX_VALUE;
+			for (Method method : methodsWithThrowable) {
+				if (!compareParameters(args, method.getParameterCount(), method.getParameterTypes(), false)) {
+					continue;
 				}
-				if (type.isAssignableFrom(cause)) {
-					int distance = calculateDistance(cause, type);
-					if (distance < min) {
-						min = distance;
-						result = method;
-					}
-					else if (distance == min) {
-						boolean parametersMatch = compareParameters(args, meta.getArgCount(),
-								method.getParameterTypes(), false);
-						if (parametersMatch) {
-							result = method;
-						}
+
+				SimpleMetadata meta = this.methods.get(method);
+				Class<? extends Throwable> methodExceptionType = meta.getType();
+				int distance = calculateDistance(causeType, methodExceptionType);
+
+				if (bestMatch == null) {
+					bestMatch = method;
+					minDistance = distance;
+				}
+				else if (distance < minDistance) {
+					minDistance = distance;
+					bestMatch = method;
+				}
+				else if (distance == minDistance) {
+					if (method.getParameterCount() > bestMatch.getParameterCount()) {
+						bestMatch = method;
 					}
 				}
 			}
 		}
-		else {
-			for (Map.Entry<Method, SimpleMetadata> entry : this.methods.entrySet()) {
-				Method method = entry.getKey();
-				if (method.getName().equals(this.recoverMethodName)) {
-					SimpleMetadata meta = entry.getValue();
-					if ((meta.type == null || meta.type.isAssignableFrom(cause))
-							&& compareParameters(args, meta.getArgCount(), method.getParameterTypes(), true)) {
-						result = method;
-						break;
-					}
+
+		if (bestMatch == null) {
+			for (Method method : methodsWithoutThrowable) {
+				if (compareParameters(args, method.getParameterCount(), method.getParameterTypes(), false)) {
+					bestMatch = method;
+					break;
 				}
 			}
 		}
-		return result;
+
+		return bestMatch;
 	}
 
 	private int calculateDistance(Class<? extends Throwable> cause, Class<? extends Throwable> type) {
@@ -175,6 +194,9 @@ private boolean compareParameters(Object[] args, int argCount, Class<?>[] parame
 			for (int i = startingIndex; i < parameterTypes.length; i++) {
 				final Object argument = i - startingIndex < args.length ? args[i - startingIndex] : null;
 				if (argument == null) {
+					if (parameterTypes[i].isPrimitive()) {
+						return false;
+					}
 					continue;
 				}
 				Class<?> parameterType = parameterTypes[i];
@@ -189,13 +211,19 @@ private boolean compareParameters(Object[] args, int argCount, Class<?>[] parame
 	}
 
 	private void init(final Object target, Method method) {
-		final Map<Class<? extends Throwable>, Method> types = new HashMap<>();
+		final Map<Class<? extends Throwable>, Method> types = new LinkedHashMap<>();
 		final Method failingMethod = method;
 		Retryable retryable = AnnotatedElementUtils.findMergedAnnotation(method, Retryable.class);
 		if (retryable != null) {
 			this.recoverMethodName = retryable.recover();
 		}
-		ReflectionUtils.doWithMethods(target.getClass(), candidate -> {
+		Method[] declared = target.getClass().getDeclaredMethods();
+		Arrays.sort(declared, Comparator.comparing(Method::getName)
+			.thenComparingInt(Method::getParameterCount)
+			.thenComparing(
+					m -> Arrays.stream(m.getParameterTypes()).map(Class::getName).collect(Collectors.joining(","))));
+
+		for (Method candidate : declared) {
 			Recover recover = AnnotatedElementUtils.findMergedAnnotation(candidate, Recover.class);
 			if (recover == null) {
 				recover = findAnnotationOnTarget(target, candidate);
@@ -210,7 +238,7 @@ private void init(final Object target, Method method) {
 			else if (recover != null && candidate.getReturnType().isAssignableFrom(failingMethod.getReturnType())) {
 				putToMethodsMap(candidate, types);
 			}
-		});
+		}
 		this.classifier.setTypeMap(types);
 		optionallyFilterMethodsBy(failingMethod.getReturnType());
 	}
@@ -280,7 +308,7 @@ private Recover findAnnotationOnTarget(Object target, Method method) {
 	}
 
 	private void optionallyFilterMethodsBy(Class<?> returnClass) {
-		Map<Method, SimpleMetadata> filteredMethods = new HashMap<>();
+		Map<Method, SimpleMetadata> filteredMethods = new LinkedHashMap<>();
 		for (Method method : this.methods.keySet()) {
 			if (method.getReturnType() == returnClass) {
 				filteredMethods.put(method, this.methods.get(method));