12.4 -> 12.5 changes behaviour with (not) using fallback client? (Resilience4jFeign)
stefanhendriks opened this issue · 14 comments
I noticed after upgrading from 12.4 to 12.5 that one integration test fails.
The integration test basically stubs one endpoint for the client to call, but, then later other calls by the client are not stubbed hence it falls back to a fallback client returning default responses.
However, in 12.5 I noticed that the 2nd call on our client (which is not stubbed), will simply return a 404. As if the fallbackClient is not used there (set up using Resilience4jFeign)?
Some code snippets:
Creation of our feign client:
@Bean
public PontoFeign pontoFeign(ApplicationSettings applicationSettings, ObjectMapper objectMapper) {
FeignDecorators decorators = FeignDecorators.builder()
.withFallback(new PontoFeignFallback(), FeignException.class)
.build();
return Resilience4jFeign.builder(decorators)
.encoder(new JacksonEncoder(objectMapper))
.decoder(new JacksonDecoder(objectMapper))
.target(PontoFeign.class, applicationSettings.getPontoBasePath());
}
Fallback client:
public class PontoFeignFallback implements PontoFeign {
// basically stubs methods here (returns empty list, returns true, etc)
}
Although I am unsure if this is a bug in Resilience4jFeign; since I noticed it happening after upgrading to the latest openFeign library I thought of posting it here first.
Well; I did notice we use version 1.7.1 of Resilience4jFeign, which seems outdated. I have upgraded that dependency to 2.1.0 and it works fine now. My bad!
Unfortunately I was too hastily with my conclusion. Even with 2.1.0 of Resilience4jFeign it breaks.
See resilience4j/resilience4j#2024 for example of integration test I am running that fails
I don't familiar with WireMock. Is there any chance that WireMock does not expect second call?
I bet this is related to #2117 where build() became final and internalBuild was introduced. All while Resilience4jFeign overrides build to insert invocation handler factory: see https://github.com/resilience4j/resilience4j/blob/master/resilience4j-feign/src/main/java/io/github/resilience4j/feign/Resilience4jFeign.java#L60
The difference is that these insertions suppose to happen before enrich, but with a new final build, enrich happens before internalBuild(), breaking the sequence.
The way I made it work is to move my custom Feign builders for Resilience4j to feign package and override enrich instead:
@Override
Feign.Builder enrich() {
super.invocationHandlerFactory(
(target, dispatch) -> new DecoratorInvocationHandler(target, dispatch,
classDecorator, methodDecorators));
return super.enrich();
}
That's not pretty, though, and might not work for the original Resilience4jFeign
My main concern is that the fallback mechanism should continue to work. I can live with our tests being broken - we can probably fix that in a different way. But if the actual fallback mechanism is broken, I don't think I'd like to merge this. Although this probably is more of a Resiliance4JFeign issue then I suppose.
Hi @stefanhendriks , maybe, Resileance4J should become a capability for feign, instead of a custom builder.
So user would just:
var client = Feign.builder()
.capability(new Resilent4JCapability())
.target(XYZ.class)
12.5 has made a breaking change by making the public build method final, which broke Resilience4j (resilience4j-feign). @edudar has provided the details. Should this API contract change go into the 13.x release instead of 12.x?
12.5 is on maven central and once it's out there is no easy way to take it down.
Anyway, ideally this wouldn't happen, but it did.
I recommend moving Resilient4J work to capability API which will be much more stable then extending our builder.
I don't think there is anything that can be done at this stage, so I will be closing this ticket.
But, feel free to open a PR with any changes needed.
This is how we implemented Resilience4jFeignCapability (in Kotlin) as @stefanhendriks suggested:
class CapabilityInvocationHandlerFactory(private val invocationDecorator: FeignDecorator) : InvocationHandlerFactory {
override fun create(
target: Target<*>,
dispatch: MutableMap<Method, InvocationHandlerFactory.MethodHandler>,
): InvocationHandler {
return DecoratorInvocationHandler(target, dispatch, invocationDecorator)
}
}
class Resilience4jFeignCapability(private val invocationDecorator: FeignDecorator) : Capability {
override fun enrich(invocationHandlerFactory: InvocationHandlerFactory): InvocationHandlerFactory {
return CapabilityInvocationHandlerFactory(invocationDecorator)
}
}
Looks perfect
Hey @alfredalbrecht, I have the exact same issue and I tried with your latest solution, however, DecoratorInvocationHandler is package-private and cannot be instantiated. How did you solve this? Thanks in advance.
FYI: I made a PR that addresses this here in resilience4j resilience4j/resilience4j#2127