Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spring-Retry @Retryable not working with Spring-data Repositories #214

Closed
jopuneet opened this issue Aug 31, 2020 · 16 comments
Closed

Spring-Retry @Retryable not working with Spring-data Repositories #214

jopuneet opened this issue Aug 31, 2020 · 16 comments
Labels

Comments

@jopuneet
Copy link

I was trying @retryable with Spring-data and testing it out using Mockito @MockBean but the retry for the repository is not working and it was always called only once.

@Repository
public interface TestRepository extends CrudRepository<Test, String> {

  @Retryable(value = { Exception.class }, maxAttempts = 4, backoff = @Backoff(delay = 2000))
  List<Test> findAll();
}
when(testRepository.findAll())
            .thenThrow(new RecoverableDataAccessException("test RecoverableDataAccessException 1"))
            .thenThrow(new RecoverableDataAccessException("test RecoverableDataAccessException 2"))
            .thenReturn(Lists.newArrayList(test));
verify(testRepository, times(3)).findAll(any());
org.mockito.exceptions.verification.TooFewActualInvocations: 
testRepository bean.findAll(<any string>);
Wanted 3 times:
-> at com.*.findAllTest(TestIT.java:10)
But was 1 time:

Referring to this Stackoverflow answer

Spring-boot: 2.3.0.RELEASE
Spring-Retry: 1.2.5.RELEASE
Spring-data: 2.3.0.RELEASE

@garyrussell
Copy link
Contributor

garyrussell commented Aug 31, 2020

If you are mocking the TestRepository then it's a mock instead of a Spring Bean so, of course, retry won't work; Mockito doesn't know anything about Spring.

Show the rest of your test case.

@jopuneet
Copy link
Author

jopuneet commented Sep 1, 2020

@garyrussell I am using @MockBean to mock the TestRepository not @Mock and @MockBean provide spring context with the mock. As referenced here Baeldung MockBean

And it is a part of the integration test, so the Spring context is present.

If spring retry will not work with Mock here, what is the way to test Spring-retry over Spring-Jpa? We already have a question on Stackoverflow .

@jopuneet
Copy link
Author

jopuneet commented Sep 1, 2020

Also, I tested this in real-time (not only in the test case), Spring-retry is not working on Spring-data repository. On retrying on any exception and returning a value from @Query() which is not expected, it is not retrying rather it is throwing this:

org.springframework.beans.factory.BeanNotOfRequiredTypeException: Bean named 'testRepository' is expected to be of type 'com.*.TestRepository' but was actually of type 'com.sun.proxy.$Proxy223'

@garyrussell
Copy link
Contributor

@MockBean provide spring context with the mock.

As said in that article (and the @MockBean javadocs).

The mock will replace any existing bean of the same type in the application context.

@garyrussell
Copy link
Contributor

For the "real" case, it is because spring-retry is wrapping the JPA proxy in another proxy that only implements Retryable.

So, yes, @Retryable is not supported directly on JPA repositories.

@garyrussell
Copy link
Contributor

Here is a work around - remove @Retryable and wire an interceptor directly into the JPA proxy.

@SpringBootApplication
public class Srgh214Application {


	private static final Logger log = LoggerFactory.getLogger(Srgh214Application.class);


	public static void main(String[] args) {
		SpringApplication.run(Srgh214Application.class, args);
	}

	@Bean
	public ApplicationRunner runner(TestRepository repo) {
		return args -> {
			repo.findAll();
		};
	}

	@Bean
	static BPP bpp() {
		return new BPP();
	}

}

class BPP implements BeanPostProcessor, Ordered {

	private static final Logger log = LoggerFactory.getLogger(BPP.class);

	@Override
	public int getOrder() {
		return Integer.MAX_VALUE;
	}

	@Override
	@Nullable
	public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
		if (bean instanceof TestRepository) {
			Advised advised = (Advised) bean;
			RetryOperationsInterceptor interceptor = RetryInterceptorBuilder.stateless()
					.maxAttempts(4)
					.backOffOptions(2000, 1.0, 2000)
					.recoverer((args, cause) -> {
						log.info("retries exhausted: " + cause.getMessage());
						throw (RuntimeException) cause;
					})
					.build();
			advised.addAdvice(0, interceptor);
		}
		return bean;
	}

}

@jopuneet
Copy link
Author

jopuneet commented Sep 19, 2020

@garyrussell Thanks this works perfectly fine.
We can close this issue if this is expected and if we will not add any capability on Spring-retry to get integrated with Spring-Data Repositories or We can add it as a feature request in the roadmap.

@kaganuk
Copy link

kaganuk commented Oct 2, 2020

adding support for that can be useful :)

@kunaldoshi98
Copy link

@garyrussell, the code you mentioned, will it be helpful to add retries on JPA repo which is doing DB calls present in the main

@mp911de
Copy link
Member

mp911de commented Jun 8, 2021

What would be the best place for a generic Spring Data integration? A BeanPostProcessor or similar can add another interceptor to Spring Data's ProxyFactory:

RepositoryFactoryBeanSupport factoryBean = …;
factoryBean.addRepositoryFactoryCustomizer(repositoryFactory -> {
	
	repositoryFactory.addRepositoryProxyPostProcessor((proxyFactory, repositoryInformation) -> {
		proxyFactory.addAdvisor(…);
	});
});

Adding Spring Retry logic including annotation scanning to Spring Data isn't probably the best approach as we would duplicate already existing code to some extent.

@garyrussell
Copy link
Contributor

garyrussell commented Jun 8, 2021

spring-retry currently uses an AbstractAutoProxyCreator which is a BPP (from spring-aop) that unconditionally wraps the "bean" in another proxy that only implements Retryable.

Injecting the repo fails because the outer proxy only implements Retryable.

We can somehow fix it by adding the retry interceptor advice to the existing proxy, but we would also need to prevent the auto proxy BPP from wrapping it in another proxy.

I don't think this will need any changes in spring-jpa.

I'll dig around some more...

@garyrussell
Copy link
Contributor

I have found a solution; will submit a PR soon.

However, I have discovered that the root cause of the problem is Boot sets proxyTargetClass to true by default.

It works for me with

 spring.aop.proxy-target-class=false

When it is true, the auto-proxy feature fails to copy the inner proxy's interface to the outer proxy; when it is false, the interfaces are copied.

My solution (for when it is true) is to re-create a single proxy with a merged set of interfaces and advisors.

@artembilan
Copy link
Member

a single proxy with a merged set of interfaces and advisors.

And that is actually what we always do in Spring Integration. If that is already a proxy we add our advises to it otherwise create:

if (AopUtils.isAopProxy(source)) {
	Advised advised = (Advised) source;
	chain.forEach(advice -> advised.addAdvisor(advice));
}
else {
	ProxyFactory proxyFactory = new ProxyFactory(source);
	chain.forEach(advice -> proxyFactory.addAdvisor(advice));
	source = proxyFactory.getProxy(getBeanClassLoader());
}

@garyrussell
Copy link
Contributor

Yes; but it's more complicated here, because Spring AOP auto-proxy creates a new proxy unconditionally. It only copies the interfaces to the outer proxy if proxyTargetClass is false (Boot sets it to true by default).

garyrussell added a commit to garyrussell/spring-retry that referenced this issue Jun 8, 2021
Resolves spring-projects#214

When `proxyTargetClass` is `true` (Boot's default), the JPA interfaces
are not copied to the outer retry proxy created by auto-proxy.

Add a BPP to fix up the proxy, when needed.
garyrussell added a commit to garyrussell/spring-retry that referenced this issue Jun 8, 2021
Resolves spring-projects#214

When `proxyTargetClass` is `true` (Boot's default), the JPA interfaces
are not copied to the outer retry proxy created by auto-proxy.

Add a BPP to fix up the proxy, when needed.

Also, the pointcut matchers must match on the `SimpleJpaRepository`.
garyrussell added a commit to garyrussell/spring-retry that referenced this issue Jun 8, 2021
Resolves spring-projects#214

When `proxyTargetClass` is `true` (Boot's default), the JPA interfaces
are not copied to the outer retry proxy created by auto-proxy.

Add a BPP to fix up the proxy, when needed.

Also, the pointcut filter must match on the `SimpleJpaRepository`.
garyrussell added a commit to garyrussell/spring-retry that referenced this issue Jun 8, 2021
Resolves spring-projects#214

When `proxyTargetClass` is `true` (Boot's default), the JPA interfaces
are not copied to the outer retry proxy created by auto-proxy.

Add a BPP to fix up the proxy, when needed.

Also, the pointcut filter must match on the `SimpleJpaRepository`.
@elafontaine
Copy link

Hi, how are we to use the Retryable with Repository today ? is it still the same as #214 (comment)

Just got bitten by it.

@garyrussell
Copy link
Contributor

garyrussell commented Feb 15, 2022

Fixed in Spring Framework 5.3.9 spring-projects/spring-framework#27044

Current 5.3.x is 5.3.15 https://spring.io/projects/spring-framework#learn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants