From 7063d363862636ece7a8bf6a2b7b781589320dff Mon Sep 17 00:00:00 2001 From: Gary Russell Date: Wed, 18 May 2022 12:13:14 -0400 Subject: [PATCH] GH-299: Fix @Recover with Class Level @Retryable Resolves https://github.com/spring-projects/spring-retry/issues/299 Caused by the fix for #244, recover methods should not be considered as retryable, with class level annotation. **cherry-pick to 1.3.x** * Fix conflicts after cherry-picking: JUnit 4, proper `MethodCallback` import --- ...tationAwareRetryOperationsInterceptor.java | 19 +++- .../annotation/DontRetryRecovererTests.java | 94 +++++++++++++++++++ 2 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 src/test/java/org/springframework/retry/annotation/DontRetryRecovererTests.java diff --git a/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java b/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java index a851852..25478f5 100644 --- a/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java +++ b/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java @@ -173,7 +173,7 @@ public class AnnotationAwareRetryOperationsInterceptor implements IntroductionIn MethodInterceptor interceptor = NULL_INTERCEPTOR; Retryable retryable = AnnotatedElementUtils.findMergedAnnotation(method, Retryable.class); if (retryable == null) { - retryable = AnnotatedElementUtils.findMergedAnnotation(method.getDeclaringClass(), Retryable.class); + retryable = classLevelAnnotation(method, Retryable.class); } if (retryable == null) { retryable = findAnnotationOnTarget(target, method, Retryable.class); @@ -202,7 +202,7 @@ public class AnnotationAwareRetryOperationsInterceptor implements IntroductionIn Method targetMethod = target.getClass().getMethod(method.getName(), method.getParameterTypes()); A retryable = AnnotatedElementUtils.findMergedAnnotation(targetMethod, annotation); if (retryable == null) { - retryable = AnnotatedElementUtils.findMergedAnnotation(targetMethod.getDeclaringClass(), annotation); + retryable = classLevelAnnotation(targetMethod, annotation); } return retryable; @@ -212,6 +212,17 @@ public class AnnotationAwareRetryOperationsInterceptor implements IntroductionIn } } + /* + * With a class level annotation, exclude @Recover methods. + */ + private A classLevelAnnotation(Method method, Class annotation) { + A ann = AnnotatedElementUtils.findMergedAnnotation(method.getDeclaringClass(), annotation); + if (ann != null && AnnotatedElementUtils.findMergedAnnotation(method, Recover.class) != null) { + ann = null; + } + return ann; + } + private MethodInterceptor getStatelessInterceptor(Object target, Method method, Retryable retryable) { RetryTemplate template = createTemplate(retryable.listeners()); template.setRetryPolicy(getRetryPolicy(retryable)); @@ -316,9 +327,9 @@ public class AnnotationAwareRetryOperationsInterceptor implements IntroductionIn return (MethodInvocationRecoverer) target; } final AtomicBoolean foundRecoverable = new AtomicBoolean(false); - ReflectionUtils.doWithMethods(target.getClass(), new MethodCallback() { + ReflectionUtils.doWithMethods(target.getClass(), new ReflectionUtils.MethodCallback() { @Override - public void doWith(Method method) throws IllegalArgumentException, IllegalAccessException { + public void doWith(Method method) throws IllegalArgumentException { if (AnnotatedElementUtils.findMergedAnnotation(method, Recover.class) != null) { foundRecoverable.set(true); } diff --git a/src/test/java/org/springframework/retry/annotation/DontRetryRecovererTests.java b/src/test/java/org/springframework/retry/annotation/DontRetryRecovererTests.java new file mode 100644 index 0000000..4729449 --- /dev/null +++ b/src/test/java/org/springframework/retry/annotation/DontRetryRecovererTests.java @@ -0,0 +1,94 @@ +/* + * Copyright 2019 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.retry.annotation; + +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.test.context.junit4.SpringRunner; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +/** + * @author Gary Russell + * @author Artem Bilan + * @since 1.3.4 + */ +@RunWith(SpringRunner.class) +public class DontRetryRecovererTests { + + @Autowired + Service service; + + @Test + public void dontRetry() { + try { + this.service.foo("x"); + fail("Exception expected"); + } + catch (Exception e) { + assertEquals("test", e.getMessage()); + } + + assertEquals(3, this.service.getCallCount()); + assertEquals(1, this.service.getRecoverCount()); + } + + @Configuration + @EnableRetry + public static class Config { + + @Bean + Service service() { + return new Service(); + } + + } + + @Retryable + public static class Service { + + int callCount; + + int recoverCount; + + public void foo(String in) { + callCount++; + throw new RuntimeException(); + } + + @Recover + public void recover(Exception ex, String in) { + this.recoverCount++; + throw new RuntimeException("test"); + } + + public int getCallCount() { + return callCount; + } + + public int getRecoverCount() { + return recoverCount; + } + + } + +}