Skip to content

Fix circuit breaker Feign builder direct usage - #1390

Open
hutiefang76 wants to merge 4 commits into
spring-cloud:mainfrom
hutiefang76:codex/openfeign-1210-circuitbreaker-builder
Open

Fix circuit breaker Feign builder direct usage#1390
hutiefang76 wants to merge 4 commits into
spring-cloud:mainfrom
hutiefang76:codex/openfeign-1210-circuitbreaker-builder

Conversation

@hutiefang76

Copy link
Copy Markdown

Fixes gh-1210.

When the circuit breaker Feign.Builder bean is used directly, it currently gets created without the CircuitBreakerFactory, group setting, or CircuitBreakerNameResolver that 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:

  • direct Feign.Builder bean configuration with circuit breaker enabled
  • fallback resolver behavior when no resolver bean is available
  • using the target name when feignClientName is not set

Tested with:

./mvnw -pl spring-cloud-openfeign-core -Dtest=FeignAutoConfigurationTests test

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Builder bean with CircuitBreakerFactory, group-enabled flag, and a resolver fallback when no CircuitBreakerNameResolver bean is present.
  • Update FeignCircuitBreakerInvocationHandler to use target.name() when feignClientName is 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.

Comment on lines +132 to +135
assertThat(builder).isInstanceOf(FeignCircuitBreaker.Builder.class)
.hasFieldOrPropertyWithValue("circuitBreakerFactory", circuitBreakerFactory)
.hasFieldOrProperty("circuitBreakerNameResolver");
});

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hutiefang76
hutiefang76 force-pushed the codex/openfeign-1210-circuitbreaker-builder branch from 9ac0c21 to 9a18726 Compare July 4, 2026 02:39
@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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ryanjbaxter

Copy link
Copy Markdown
Contributor

Please sign your commits so the DCO passes

circuitBreakerNameResolver);
}

public Feign.Builder circuitBreakerFeignBuilder(CircuitBreakerFactory circuitBreakerFactory,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably create a Properties class for these, the group property is already used in FeignAutoConfiguration

@ryanjbaxter

Copy link
Copy Markdown
Contributor

Can you rebase the PR?

hutiefang and others added 4 commits August 27, 2026 01:59
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>
@hutiefang76
hutiefang76 force-pushed the codex/openfeign-1210-circuitbreaker-builder branch from 34f202a to 1129178 Compare August 26, 2026 18:11
@hutiefang76

Copy link
Copy Markdown
Author

@ryanjbaxter Rebased onto the current main and addressed the requested configuration cleanup.

  • Added FeignCircuitBreakerProperties for the existing group.enabled and alphanumeric-ids.enabled settings, shared by the targeter and builder.
  • Moved the prototype builder bean annotations to the injected overload; the existing public no-arg method remains available.
  • Removed field injection from the circuit-breaker builder configuration.
  • Extended the tests to assert the actual injected bean method and both resolver behaviors.

Validation: ./mvnw -pl spring-cloud-openfeign-core -Dtest=FeignAutoConfigurationTests test passes (11 tests). I also ran the full core module; its two remaining failures are in FeignClientCacheTests, which expects UnknownHostException for the external fake host foo, while this local network resolves it and returns NoHttpResponseException.

@@ -189,10 +188,10 @@ public CircuitBreakerNameResolver alphanumericCircuitBreakerNameResolver() {
@ConditionalOnMissingBean
@ConditionalOnBean(CircuitBreakerFactory.class)
public Targeter circuitBreakerFeignTargeter(CircuitBreakerFactory circuitBreakerFactory,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same overload pattern needs to be applied here

@ryanjbaxter ryanjbaxter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR build passes, one small change

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No CircuitBreakerNameResolver set in circuitBreakerFeignBuilder Bean

4 participants