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

Unable to define recover method where the method is returning a generic List #402

Closed
bgiaccio opened this issue Nov 20, 2023 · 8 comments
Closed
Milestone

Comments

@bgiaccio
Copy link

We are upgrading our spring 1.3.4 properly handles a method public List<T> myMethod() where the recovery is defined as public List<T> recoverMyMethod(Throwable t) but after #328 using version 2.0.4 it would seem that no longer works.
The offending code is what was added in that bug !methodArgType.equals(failingMethodArgType) because the two Ts are not equal as far as the reflection is concerned since they are on different methods; in theory they could be different but for the purpose of @Retryable/@recover they won't be.

@artembilan
Copy link
Member

See the test provided for that fix:

	private static class ParameterTest<T, M> {

		List<T> m1() {
			return null;
		}

		List<T> m2() {
			return null;
		}

		List<M> m2_1() {
			return null;
		}
...

		assertThat(isParameterizedTypeAssignable.invoke(null, getGenericReturnTypeByName("m1"),
				getGenericReturnTypeByName("m2"))).isEqualTo(Boolean.TRUE);
		assertThat(isParameterizedTypeAssignable.invoke(null, getGenericReturnTypeByName("m2"),
				getGenericReturnTypeByName("m2_1"))).isEqualTo(Boolean.FALSE);

So, since T is exposed on a class by itself, it is indeed the same type on those methods, but M is not equal to T.
In this case:

class MyClass {
   
		<T> List<T> m1() {
			return null;
		}

		<T> List<T> m2() {
			return null;
		}
}

Ts are really different because they are local to the methods they are declared on.
It is no difference with any local variables even if they have the same name.

@bgiaccio
Copy link
Author

bgiaccio commented Nov 21, 2023

So basically I have

public class MyClass {
    	@Retryable(value = {Throwable.class}, exclude = { NotAcceptableException.class }, backoff = @Backoff(delay = 1000, multiplier = 2), maxAttemptsExpression = "5")
	@Transactional(propagation = Propagation.REQUIRES_NEW, rollbackFor = Throwable.class)
	public <T> List<T> runQuery( Class<T> returnType, otherArgs ) { ...}

	@Recover
	private <T> List<T> recoverRunQuery(Exception ex, Class<T> returnType, otherArgs ) throws Exception { ... }

With spring-recover 2.0.0 that worked just fine but it fails with 2.0.4 which I've traced back to #328

This class is a helper for fetching data from numerous classes so T varies by the caller, so creating an instance for each would be a substantial code change. Hoping you have a suggestion on how I might make a recover method for that.

@artembilan
Copy link
Member

You just relied on a bug or rather some side-effect which didn't honor generics.
If you still insist on using those local <T>, then consider to use this attribute on the @Retryable:

	/**
	 * Name of method in this class to use for recover. Method had to be marked with
	 * {@link Recover} annotation.
	 * @return the name of recover method
	 */
	String recover() default "";

instead of trying to trick reflection which we treat as a bug and therefore the fix you are mentioning.

@swampy2b
Copy link

We did try with setting the recover attribute in the @Retryable annotation, but it had no effect, because the method was never added to list of potential recovery methods due to the return type mismatch.

@bgiaccio
Copy link
Author

So using the recover attribute on the annotation selects the method but when trying to mach it winds up back in isParameterizedTypeAssignable passing sun.reflect.generics.reflectiveObjects.TypeVariableImpl but fails because methodArgType.equals(failingMethodArgType) is false

@artembilan
Copy link
Member

How about to make your @Recover method as Object for return type (if something like List<?> doesn't work)?

My point is that we need to agree that <T> List<T> is local declaration and according to Java it is really not equal on different methods.

@bgiaccio
Copy link
Author

I wound up reworking the code to use https://github.com/spring-projects/spring-retry#using-recoverycallback
Small documentation issue on that page it should be Foo foo = template.execute(new RetryCallback<Foo, Exception>() {

So in case someone else is reading this bug the solution looks like

retryTemplate = RetryTemplate.builder()
	.maxAttempts(maxAttempts)
	.notRetryOn(List.of(NotAcceptableException.class, InstantiationException.class))
	.customBackoff(BackOffPolicyBuilder.newBuilder().delay(delay).multiplier(multiplier).build())
	.build();
return retryTemplate.execute(
	(RetryCallback<List<T>, Exception>) context -> self.retryRunQuery(queryName, returnType, otherArgs),
	context -> self.recoverQuery(context.getLastThrowable(), queryName)
);

@artembilan
Copy link
Member

Thank you for feedback!

Those docs are fixed now.

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

No branches or pull requests

3 participants