spring-cloud / spring-cloud-openfeign

Support for using OpenFeign in Spring Cloud apps
Apache License 2.0
1.17k stars 756 forks source link

Fallback functionality should be selectively enabled depending on the actual presense of fallback implementation #994

Closed djkeh closed 3 months ago

djkeh commented 4 months ago

Is your feature request related to a problem? Please describe. It is not directly related to a certain problem, but I think some people could feel it uncomfortable.

Describe the solution you'd like What do we do if we'd like to use the fallback functionality using fallback or fallbackFactory in @FeignClient annotation to have full control over the error responses? We are supposed to do:

  1. Add spring-cloud-starter-circuitbreaker-resilience4j dependency
  2. Use spring.cloud.openfeign.circuitbreaker.enabled=true configuration property
  3. Implement Fallback or FallbackFactory class
  4. Put it in the @FeignClient annotation.

All of these are necessary procedures. For example, if you see the annotation through step 4 like:

@FeignClient(fallback = MyFallback::class)

You feel by instinct that this feign client definitely has fallback MyFallback, so it should be triggered when the error response comes in. But, no. You can't see the fallback logic work until you follow step 2.

What feels unnatural to me is the relation between step 2 and step 4. First I thought that, even I enabled the fallback functionality through step 2, the fallback would only happen to the feign clients that explicitly have fallback or fallback factory. However, it is not. Step 2 basically activates fallback for the entire feign clients you have in the project. So what happens if one @Feignclient has no fallback class? It throws NoFallbackAvailableException. This means after enabling circuit breaker (only to use fallback), you have to implement fallback classes for every feign client you have. Suppose you got 10 feign client interfaces and you're about to put 11th feign client, but this time you think it's right about time to try fallback logic on the 11th feign client. But no, after the study and test you realize that you have to implement fallbacks for 11 feign clients, if you want to turn on the fallback feature.

Well? Is no way there to solve this trouble? Fortunately yes there is. According to the reference, you put:

class NoFallBack {
    @Bean
    @Scope("prototype")
    fun feignBuilder(): Feign.Builder = Feign.builder()
}

This creates default feign builder instead of FeignCircuitBreaker.Builder which contains fallback action. You put NoFallBack to the certain feign client to prevent it from running fallback. So? I'm putting NoFallBack configuration to the 10 feign clients except the 11th one.

Well?

I'm not saying this is wrong. But.. seriously? The original feign client didn't have fallback implementation from the start, like:

@FeignClient(
    name = "myClient",
    url = "https://my.client",
)

Now the annotation goes like:

@FeignClient(
    name = "myClient",
    url = "https://my.client",
    configuration = [NoFallBack::class],
)

Why do we need that line? It doesn't have fallback option and it delivers enough information that we don't need fallback for this feign client. The framework, however, looks for the fallback implementation, there isn't, so it throws NoFallbackAvailableException. Meanwhile, the 11th feign client annotation will look like:

@FeignClient(
    name = "my11thClient",
    url = "https://my11th.client",
    fallback = MyFallback::class,
)

which describes enough information that it has a fallback.

Describe alternatives you've considered There might be a few simple ideas to make things better:

  1. FeignClientsConfiguration provides defaultFeignBuilder() as the default feign builder instead of circuitBreakerFeignBuilder(). circuitBreakerFeignBuilder bean only works when the feign client has fallback.
  2. CircuitBreakerPresentFeignBuilderConfiguration provides default fallback which acts just like default ErrorDecoder. If you enabled circuit breaker and there's no fallback for the feign client, instead of throwing NoFallbackAvailableException it runs default fallback. What should default fallback do? Simple. Do exactly what ErrorDecoder does. Throw DecodeException. Actually, bypass the exception.

The point is that you don't need to write every fallback implementation after enabling circuit breaker. fallback | fallbackFactory options in @FeignClient annotation looks so descriptive enough that it has fallback. Whether the client has fallback is up to the client, if it doesn't have fallback so be it! Feign client already has beautiful default behavior for the error response inside ErrorDecoder. It doesn't really have to throw the exception if you enabled the circuit breaker and you just didn't give fallback to it.

Let's go for "Enable some if it explicitly says yes", not for "Enable All, exclude some if it explicitly says no". There's technically no drawback. If we go "enable all, exclude some", we still have to put fallback | fallbackClass to every @FeignClient because if we don't, it doesn't work!

Additional context

Here are some related references.

OlgaMaciaszek commented 3 months ago

Hello @djkeh, Spring Cloud OpenFeign is now in maintenance only mode (we're suggesting migrating over to Spring interface clients). We are not planning to work on new features in this project.