Fix circuit breaker Feign builder direct usage - #1390
Conversation
409c73a to
2163b3e
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes gh-1210 by ensuring the circuit breaker Feign.Builder bean is fully configured (factory, group flag, and name resolver) when it is used directly, so it behaves consistently with the normal Feign client targeter path. It also makes circuit breaker name/group resolution work when the builder is used outside the named Feign client path by falling back to the Feign Target name.
Changes:
- Configure the circuit-breaker
Feign.Builderbean withCircuitBreakerFactory, group-enabled flag, and a resolver fallback when noCircuitBreakerNameResolverbean is present. - Update
FeignCircuitBreakerInvocationHandlerto usetarget.name()whenfeignClientNameis not set (for direct builder usage). - Add tests covering direct builder configuration and target-name fallback behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignAutoConfigurationTests.java | Adds tests for direct circuit breaker builder configuration and target-name fallback behavior. |
| spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java | Configures the circuit breaker Feign.Builder bean with factory/group/resolver (including a default resolver fallback). |
| spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java | Falls back to target.name() when feignClientName is null for circuit/group naming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assertThat(builder).isInstanceOf(FeignCircuitBreaker.Builder.class) | ||
| .hasFieldOrPropertyWithValue("circuitBreakerFactory", circuitBreakerFactory) | ||
| .hasFieldOrProperty("circuitBreakerNameResolver"); | ||
| }); |
There was a problem hiding this comment.
The current head verifies the fallback behavior directly. The direct builder test obtains the resolver from the builder, asserts the default AlphanumericCircuitBreakerNameResolver, and invokes it with a real target and method. The disabled-property variant verifies DefaultCircuitBreakerNameResolver and the unmodified config key. Verified with the focused module test: 11 tests, 0 failures/errors.
| .circuitBreakerFactory(circuitBreakerFactory) | ||
| .circuitBreakerGroupEnabled(circuitBreakerGroupEnabled) | ||
| .circuitBreakerNameResolver(circuitBreakerNameResolver | ||
| .getIfAvailable(() -> (feignClientName, target, method) -> Feign.configKey(target.type(), method))); |
There was a problem hiding this comment.
Technically I believe the fallback name resolver would be AlphanumericCircuitBreakerNameResolver by default if alphanumeric-ids.enabled has matchIfMissing = true. We should probably check that property and use the correct name resolver
There was a problem hiding this comment.
The fallback now follows spring.cloud.openfeign.circuitbreaker.alphanumeric-ids.enabled: it uses AlphanumericCircuitBreakerNameResolver when enabled or omitted, and DefaultCircuitBreakerNameResolver when disabled. Both cases are exercised by the direct-builder tests. Verified with the focused module test: 11 tests, 0 failures/errors.
9ac0c21 to
9a18726
Compare
| @ConditionalOnBean(CircuitBreakerFactory.class) | ||
| public Feign.Builder circuitBreakerFeignBuilder(CircuitBreakerFactory circuitBreakerFactory, | ||
| @Value("${spring.cloud.openfeign.circuitbreaker.group.enabled:false}") boolean circuitBreakerGroupEnabled, | ||
| @Value("${spring.cloud.openfeign.circuitbreaker.alphanumeric-ids.enabled:true}") boolean alphanumericIdsEnabled, |
There was a problem hiding this comment.
Can you introduce a new public method with the new parameter and delegate to that? I do not want to change the signature of public methods
There was a problem hiding this comment.
The no-argument public bean method is preserved and delegates to the overload that accepts the configuration values. The test verifies that the public no-argument circuitBreakerFeignBuilder method remains present. Verified with the focused module test: 11 tests, 0 failures/errors.
|
Please sign your commits so the DCO passes |
d52596c to
34f202a
Compare
| circuitBreakerNameResolver); | ||
| } | ||
|
|
||
| public Feign.Builder circuitBreakerFeignBuilder(CircuitBreakerFactory circuitBreakerFactory, |
There was a problem hiding this comment.
Remove
@Bean
@Scope("prototype")
@ConditionalOnMissingBean
@ConditionalOnBean(CircuitBreakerFactory.class)
From the old method, add them here and then inject the beans you need instead of autowiring them
| @Autowired | ||
| private CircuitBreakerFactory circuitBreakerFactory; | ||
|
|
||
| @Value("${spring.cloud.openfeign.circuitbreaker.group.enabled:false}") |
There was a problem hiding this comment.
I think we should probably create a Properties class for these, the group property is already used in FeignAutoConfiguration
|
Can you rebase the PR? |
Signed-off-by: hutiefang <hutiefang@qq.com>
Signed-off-by: hutiefang <hutiefang@qq.com>
Signed-off-by: hutiefang <hutiefang@qq.com>
Signed-off-by: hutiefang76 <137664623+hutiefang76@users.noreply.github.com>
34f202a to
1129178
Compare
|
@ryanjbaxter Rebased onto the current
Validation: |
| @@ -189,10 +188,10 @@ public CircuitBreakerNameResolver alphanumericCircuitBreakerNameResolver() { | |||
| @ConditionalOnMissingBean | |||
| @ConditionalOnBean(CircuitBreakerFactory.class) | |||
| public Targeter circuitBreakerFeignTargeter(CircuitBreakerFactory circuitBreakerFactory, | |||
There was a problem hiding this comment.
Same overload pattern needs to be applied here
ryanjbaxter
left a comment
There was a problem hiding this comment.
PR build passes, one small change
Fixes gh-1210.
When the circuit breaker
Feign.Builderbean is used directly, it currently gets created without theCircuitBreakerFactory, group setting, orCircuitBreakerNameResolverthat the normal Feign client targeter path applies later. That makes the direct builder path fail at invocation time instead of behaving like the regular circuit breaker Feign client path.This change configures the circuit breaker builder bean with the available factory, group flag, and resolver. It also falls back to the Feign target name when the builder is used outside the named Feign client targeter path.
Tests added for:
Feign.Builderbean configuration with circuit breaker enabledfeignClientNameis not setTested with: