Spring/spring-retry
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix counting errors in args matchers for recovery

Browse Source 
There were off-by-one errors in the recovery argument matching,
and also in the copying of arguments from source to target method.

Fixes gh-169
This commit is contained in:
Dave Syer
2019-04-04 13:48:09 +01:00
parent 7429a6994b
commit 9474abb17e
2 changed files with 140 additions and 89 deletions
Download Patch File Download Diff File Expand all files Collapse all files

75
src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java
View File

@@ -46,7 +46,9 @@ import org.springframework.util.ReflectionUtils.MethodCallback;
public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationRecoverer<T> {
private SubclassClassifier<Throwable, Method> classifier = new SubclassClassifier<Throwable, Method>();
private Map<Method, SimpleMetadata> methods = new HashMap<Method, SimpleMetadata>();
private Object target;
public RecoverAnnotationRecoveryHandler(Object target, Method method) {
@@ -60,13 +62,13 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
if (method == null) {
throw new ExhaustedRetryException("Cannot locate recovery method", cause);
}
SimpleMetadata meta = methods.get(method);
SimpleMetadata meta = this.methods.get(method);
Object[] argsToUse = meta.getArgs(cause, args);
boolean methodAccessible = method.isAccessible();
try {
ReflectionUtils.makeAccessible(method);
@SuppressWarnings("unchecked")
T result = (T) ReflectionUtils.invokeMethod(method, target, argsToUse);
T result = (T) ReflectionUtils.invokeMethod(method, this.target, argsToUse);
return result;
}
finally {
@@ -79,8 +81,8 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
private Method findClosestMatch(Object[] args, Class<? extends Throwable> cause) {
int min = Integer.MAX_VALUE;
Method result = null;
for (Method method : methods.keySet()) {
SimpleMetadata meta = methods.get(method);
for (Method method : this.methods.keySet()) {
SimpleMetadata meta = this.methods.get(method);
Class<? extends Throwable> type = meta.getType();
if (type == null) {
type = Throwable.class;
@@ -92,8 +94,8 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
result = method;
}
else if (distance == min) {
boolean parametersMatch = compareParameters( args, meta.getArgCount(),
method.getParameterTypes() );
boolean parametersMatch = compareParameters(args, meta.getArgCount(),
method.getParameterTypes());
if (parametersMatch) {
result = method;
}
@@ -114,14 +116,17 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
return result;
}
private boolean compareParameters(Object[] args, int argCount, Class<?>[] parameterTypes) {
private boolean compareParameters(Object[] args, int argCount,
Class<?>[] parameterTypes) {
if (argCount == (args.length + 1)) {
int startingIndex = 0;
if (parameterTypes.length > 0 && Throwable.class.isAssignableFrom(parameterTypes[0])) {
if (parameterTypes.length > 0
&& Throwable.class.isAssignableFrom(parameterTypes[0])) {
startingIndex = 1;
}
for (int i = startingIndex; i < parameterTypes.length; i++) {
final Object argument = args[i-1];
final Object argument = i - startingIndex < args.length
? args[i - startingIndex] : null;
if (argument == null) {
continue;
}
@@ -140,48 +145,50 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
ReflectionUtils.doWithMethods(failingMethod.getDeclaringClass(),
new MethodCallback() {
@Override
public void doWith(Method method) throws IllegalArgumentException,
IllegalAccessException {
public void doWith(Method method)
throws IllegalArgumentException, IllegalAccessException {
Recover recover = AnnotationUtils.findAnnotation(method,
Recover.class);
if (recover != null
&& method.getReturnType().isAssignableFrom(
failingMethod.getReturnType())) {
if (recover != null && method.getReturnType()
.isAssignableFrom(failingMethod.getReturnType())) {
Class<?>[] parameterTypes = method.getParameterTypes();
if (parameterTypes.length > 0
&& Throwable.class
.isAssignableFrom(parameterTypes[0])) {
if (parameterTypes.length > 0 && Throwable.class
.isAssignableFrom(parameterTypes[0])) {
@SuppressWarnings("unchecked")
Class<? extends Throwable> type = (Class<? extends Throwable>) parameterTypes[0];
types.put(type, method);
methods.put(method, new SimpleMetadata(
parameterTypes.length, type));
} else {
classifier.setDefaultValue(method);
methods.put(method, new SimpleMetadata(
parameterTypes.length, null));
RecoverAnnotationRecoveryHandler.this.methods.put(method,
new SimpleMetadata(parameterTypes.length, type));
}
else {
RecoverAnnotationRecoveryHandler.this.classifier
.setDefaultValue(method);
RecoverAnnotationRecoveryHandler.this.methods.put(method,
new SimpleMetadata(parameterTypes.length, null));
}
}
}
});
classifier.setTypeMap(types);
this.classifier.setTypeMap(types);
optionallyFilterMethodsBy(failingMethod.getReturnType());
}
private void optionallyFilterMethodsBy(Class<?> returnClass) {
Map<Method, SimpleMetadata> filteredMethods = new HashMap<Method, SimpleMetadata>();
for (Method method : methods.keySet()) {
for (Method method : this.methods.keySet()) {
if (method.getReturnType() == returnClass) {
filteredMethods.put(method, methods.get(method));
filteredMethods.put(method, this.methods.get(method));
}
}
if (filteredMethods.size() > 0) {
methods = filteredMethods;
this.methods = filteredMethods;
}
}
private static class SimpleMetadata {
private int argCount;
private Class<? extends Throwable> type;
public SimpleMetadata(int argCount, Class<? extends Throwable> type) {
@@ -191,23 +198,29 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
}
public int getArgCount() {
return argCount;
return this.argCount;
}
public Class<? extends Throwable> getType() {
return type;
return this.type;
}
public Object[] getArgs(Throwable t, Object[] args) {
Object[] result = new Object[getArgCount()];
int startArgs = 0;
if (type != null) {
if (this.type != null) {
result[0] = t;
startArgs = 1;
}
System.arraycopy(args, 0, result, startArgs, result.length - startArgs);
int length = result.length - startArgs > args.length ? args.length
: result.length - startArgs;
if (length == 0) {
return result;
}
System.arraycopy(args, 0, result, startArgs, length);
return result;
}
}
}

154
src/test/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandlerTests.java
View File

@@ -16,15 +16,16 @@
package org.springframework.retry.annotation;
import static org.junit.Assert.assertEquals;
import java.lang.reflect.Method;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.springframework.retry.ExhaustedRetryException;
import org.springframework.util.ReflectionUtils;
import java.lang.reflect.Method;
import static org.junit.Assert.assertEquals;
/**
* @author Dave Syer
@@ -39,10 +40,10 @@ public class RecoverAnnotationRecoveryHandlerTests {
@Test
public void defaultRecoverMethod() {
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new DefaultRecover(), ReflectionUtils.findMethod(DefaultRecover.class,
"foo", String.class));
assertEquals(1,
handler.recover(new Object[] { "Dave" }, new RuntimeException("Planned")));
new DefaultRecover(),
ReflectionUtils.findMethod(DefaultRecover.class, "foo", String.class));
assertEquals(1, handler.recover(new Object[] { "Dave" },
new RuntimeException("Planned")));
}
@Test
@@ -50,8 +51,8 @@ public class RecoverAnnotationRecoveryHandlerTests {
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new FewerArgs(), ReflectionUtils.findMethod(FewerArgs.class, "foo",
String.class, int.class));
assertEquals(1,
handler.recover(new Object[] { "Dave" }, new RuntimeException("Planned")));
assertEquals(1, handler.recover(new Object[] { "Dave" },
new RuntimeException("Planned")));
}
@Test
@@ -66,69 +67,69 @@ public class RecoverAnnotationRecoveryHandlerTests {
@Test
public void noMatch() {
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new SpecificException(), ReflectionUtils.findMethod(
SpecificException.class, "foo", String.class));
expected.expect(ExhaustedRetryException.class);
new SpecificException(),
ReflectionUtils.findMethod(SpecificException.class, "foo", String.class));
this.expected.expect(ExhaustedRetryException.class);
handler.recover(new Object[] { "Dave" }, new Error("Planned"));
}
@Test
public void specificRecoverMethod() {
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new SpecificRecover(), ReflectionUtils.findMethod(SpecificRecover.class,
"foo", String.class));
assertEquals(2,
handler.recover(new Object[] { "Dave" }, new RuntimeException("Planned")));
new SpecificRecover(),
ReflectionUtils.findMethod(SpecificRecover.class, "foo", String.class));
assertEquals(2, handler.recover(new Object[] { "Dave" },
new RuntimeException("Planned")));
}
@Test
public void inAccessibleRecoverMethods (){
Method foo = ReflectionUtils.findMethod(InAccessibleRecover.class,
"foo", String.class);
public void inAccessibleRecoverMethods() {
Method foo = ReflectionUtils.findMethod(InAccessibleRecover.class, "foo",
String.class);
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new InAccessibleRecover(), foo);
assertEquals(1,
handler.recover(new Object[] { "Dave" }, new RuntimeException("Planned")));
assertEquals(1, handler.recover(new Object[] { "Dave" },
new RuntimeException("Planned")));
}
@Test
public void specificReturnTypeRecoverMethod() {
RecoverAnnotationRecoveryHandler<?> fooHandler = new RecoverAnnotationRecoveryHandler<Integer>(
new InheritanceReturnTypeRecover(), ReflectionUtils.findMethod(InheritanceReturnTypeRecover.class,
"foo", String.class));
assertEquals(1,
fooHandler.recover(new Object[] { "Aldo" }, new RuntimeException("Planned")));
assertEquals(2,
fooHandler.recover(new Object[] { "Aldo" }, new IllegalStateException("Planned")));
new InheritanceReturnTypeRecover(), ReflectionUtils.findMethod(
InheritanceReturnTypeRecover.class, "foo", String.class));
assertEquals(1, fooHandler.recover(new Object[] { "Aldo" },
new RuntimeException("Planned")));
assertEquals(2, fooHandler.recover(new Object[] { "Aldo" },
new IllegalStateException("Planned")));
}
@Test
public void parentReturnTypeRecoverMethod() {
RecoverAnnotationRecoveryHandler<?> barHandler = new RecoverAnnotationRecoveryHandler<Double>(
new InheritanceReturnTypeRecover(), ReflectionUtils.findMethod(InheritanceReturnTypeRecover.class,
"bar", String.class));
assertEquals(3,
barHandler.recover(new Object[] { "Aldo" }, new RuntimeException("Planned")));
new InheritanceReturnTypeRecover(), ReflectionUtils.findMethod(
InheritanceReturnTypeRecover.class, "bar", String.class));
assertEquals(3, barHandler.recover(new Object[] { "Aldo" },
new RuntimeException("Planned")));
}
@Test
public void multipleQualifyingRecoverMethods(){
Method foo = ReflectionUtils.findMethod(MultipleQualifyingRecovers.class,
"foo", String.class);
public void multipleQualifyingRecoverMethods() {
Method foo = ReflectionUtils.findMethod(MultipleQualifyingRecovers.class, "foo",
String.class);
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new MultipleQualifyingRecovers(), foo);
assertEquals(1,
handler.recover(new Object[] { "Randell" }, new RuntimeException("Planned")));
assertEquals(1, handler.recover(new Object[] { "Randell" },
new RuntimeException("Planned")));
}
@Test
public void multipleQualifyingRecoverMethodsWithNull(){
Method foo = ReflectionUtils.findMethod(MultipleQualifyingRecovers.class,
"foo", String.class);
public void multipleQualifyingRecoverMethodsWithNull() {
Method foo = ReflectionUtils.findMethod(MultipleQualifyingRecovers.class, "foo",
String.class);
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new MultipleQualifyingRecovers(), foo);
assertEquals(1,
@@ -137,40 +138,52 @@ public class RecoverAnnotationRecoveryHandlerTests {
}
@Test
public void multipleQualifyingRecoverMethodsReOrdered(){
public void multipleQualifyingRecoverMethodsWithNoThrowable() {
Method foo = ReflectionUtils.findMethod(
MultipleQualifyingRecoversNoThrowable.class, "foo", String.class);
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new MultipleQualifyingRecoversNoThrowable(), foo);
assertEquals(1,
handler.recover(new Object[] { null }, new RuntimeException("Planned")));
}
@Test
public void multipleQualifyingRecoverMethodsReOrdered() {
Method foo = ReflectionUtils.findMethod(MultipleQualifyingRecoversReOrdered.class,
"foo", String.class);
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new MultipleQualifyingRecoversReOrdered(), foo);
assertEquals(3,
handler.recover(new Object[] { "Randell" }, new RuntimeException("Planned")));
assertEquals(3, handler.recover(new Object[] { "Randell" },
new RuntimeException("Planned")));
}
@Test
public void multipleQualifyingRecoverMethodsExtendsThrowable(){
Method foo = ReflectionUtils.findMethod(MultipleQualifyingRecoversExtendsThrowable.class,
"foo", String.class);
@Test
public void multipleQualifyingRecoverMethodsExtendsThrowable() {
Method foo = ReflectionUtils.findMethod(
MultipleQualifyingRecoversExtendsThrowable.class, "foo", String.class);
RecoverAnnotationRecoveryHandler<?> handler = new RecoverAnnotationRecoveryHandler<Integer>(
new MultipleQualifyingRecoversExtendsThrowable(), foo);
assertEquals(2,
handler.recover(new Object[] { "Kevin" }, new IllegalArgumentException("Planned")));
assertEquals(3,
handler.recover(new Object[] { "Kevin" }, new UnsupportedOperationException("Planned")));
assertEquals(2, handler.recover(new Object[] { "Kevin" },
new IllegalArgumentException("Planned")));
assertEquals(3, handler.recover(new Object[] { "Kevin" },
new UnsupportedOperationException("Planned")));
}
private static class InAccessibleRecover {
@Retryable
private int foo (String n) {
private int foo(String n) {
throw new RuntimeException("error trying to foo('" + n + "')");
}
@Recover
private int bar(String n){
return 1 ;
private int bar(String n) {
return 1;
}
}
protected static class DefaultRecover {
@@ -184,9 +197,11 @@ public class RecoverAnnotationRecoveryHandlerTests {
public int bar(String name) {
return 1;
}
}
protected static class NoArgs {
private Throwable cause;
@Retryable
@@ -199,11 +214,13 @@ public class RecoverAnnotationRecoveryHandlerTests {
}
public Throwable getCause() {
return cause;
return this.cause;
}
}
protected static class SpecificRecover {
@Retryable
public int foo(String name) {
return 0;
@@ -222,6 +239,7 @@ public class RecoverAnnotationRecoveryHandlerTests {
}
protected static class FewerArgs {
@Retryable
public int foo(String name, int value) {
return 0;
@@ -235,6 +253,7 @@ public class RecoverAnnotationRecoveryHandlerTests {
}
protected static class SpecificException {
@Retryable
public int foo(String name) {
return 0;
@@ -276,6 +295,25 @@ public class RecoverAnnotationRecoveryHandlerTests {
}
protected static class MultipleQualifyingRecoversNoThrowable {
@Retryable
public int foo(String name) {
return 0;
}
@Recover
public int fooRecover(String name, String nullable) {
return 1;
}
@Recover
public int fooRecover(int other, String nullable) {
return 2;
}
}
protected static class MultipleQualifyingRecovers {
@Retryable
@@ -323,8 +361,8 @@ public class RecoverAnnotationRecoveryHandlerTests {
}
}
protected static class MultipleQualifyingRecoversExtendsThrowable {
protected static class MultipleQualifyingRecoversExtendsThrowable {
@Retryable
public int foo(String name) {
@@ -335,8 +373,8 @@ public class RecoverAnnotationRecoveryHandlerTests {
public int fooRecover(IllegalArgumentException e, String name) {
return 1;
}
@Recover
@Recover
public int barRecover(IllegalArgumentException e, String name) {
return 2;
}