Skip to content

Use binary class name to resolve per-class HTTP interface fallbacks - #1715

Open
HDPark95 wants to merge 1 commit into
spring-cloud:mainfrom
HDPark95:fix/httpservice-fallback-nested-classname
Open

Use binary class name to resolve per-class HTTP interface fallbacks#1715
HDPark95 wants to merge 1 commit into
spring-cloud:mainfrom
HDPark95:fix/httpservice-fallback-nested-classname

Conversation

@HDPark95

@HDPark95 HDPark95 commented Aug 6, 2026

Copy link
Copy Markdown

What

CircuitBreakerRequestValueProcessor records the declaring class of the invoked @HttpExchange method under getCanonicalName(), but per-class fallbacks are registered with Class#getName() (the binary name) in CircuitBreakerConfigurerUtils.addFallbackEntries. getFallback then resolves the fallback by looking up that recorded value.

For a top-level interface both forms are identical, but for a nested @HttpExchange interface they differ: com.example.Outer.Inner (canonical) vs com.example.Outer$Inner (binary). The lookup key never matches the registration key, so the per-class fallback is silently skipped and the invocation falls through to the default fallback or, when none is registered, throws NoFallbackAvailableException.

Fix

Record the declaring class under getName() so the declaring-class lookup key matches the registration key. Top-level interfaces are unaffected because their canonical and binary names are identical.

This circuit-breaking-over-@HttpExchange support is new in 5.0.0 and unreleased, so aligning the stored attribute carries no compatibility concern.

Verification

  • Added shouldResolvePerClassFallbackForNestedServiceInterface to CircuitBreakerAdapterDecoratorTests, which drives the real CircuitBreakerRequestValueProcessor with a nested service interface and asserts the per-class fallback resolves.
  • With the fix reverted the new test fails with NoFallbackAvailableException; with the fix the whole test class passes (Tests run: 6, Failures: 0, Errors: 0).
  • ./mvnw -pl spring-cloud-commons test is green, including spring-javaformat and checkstyle validation.

CircuitBreakerRequestValueProcessor stored the declaring class under its
canonical name, while fallbacks are registered per service using
Class#getName() (the binary name). For a nested @HttpExchange interface
these two forms differ (Outer.Inner vs Outer$Inner), so the per-class
fallback was never matched and the invocation fell through to the default
fallback or NoFallbackAvailableException.

Store the binary name so the declaring-class lookup key matches the
registration key. Top-level interfaces are unaffected because their
canonical and binary names are identical.

Signed-off-by: HDPark95 <qkrgusen456@gmail.com>
@kdelay

kdelay commented Aug 7, 2026

Copy link
Copy Markdown

I ran this locally against the PR head (JDK 17.0.15, ./mvnw -pl spring-cloud-commons test).

The regression guard holds. With the one-line production change reverted and the rest of the branch untouched, shouldResolvePerClassFallbackForNestedServiceInterface is the only failure across the two decorator test classes:

CircuitBreakerAdapterDecoratorTests:         Tests run: 6,  Failures: 0, Errors: 1
  shouldResolvePerClassFallbackForNestedServiceInterface
  -> NoFallbackAvailableException: No fallback available.
ReactiveCircuitBreakerAdapterDecoratorTests: Tests run: 14, Failures: 0, Errors: 0

With the change in place both classes are green (6/0 and 14/0). The reactive decorator needs no separate change: both decorators resolve through CircuitBreakerConfigurerUtils.getFallback and read the same DECLARING_CLASS_ATTRIBUTE_NAME, so the single write site in the processor covers both paths. That might be worth a sentence in the description, since the diff touching only the blocking test class makes it look narrower than it is.

Two things I noticed while checking. Neither blocks the fix.

1. The existing fixtures still build the key with getCanonicalName(). There are 17 places that put TestService.class.getCanonicalName() into the attribute map by hand (4 in CircuitBreakerAdapterDecoratorTests, 13 in ReactiveCircuitBreakerAdapterDecoratorTests), 4 registration keys built the same way, and both group configurer tests assert on decorator.getFallbackClasses().get(TestService.class.getCanonicalName()) even though addFallbackEntries registers under getName(). They pass only because TestService and UnusedTestService are top-level types, where the two forms are identical - the run above shows they are insensitive to which form the processor writes. As they stand they document a key form the processor no longer produces, and a fixture later converted to a nested interface would reintroduce this bug without any test noticing. It is a mechanical test-only change if you want it here.

2. Inherited @HttpExchange methods have the same class of mismatch, and this fix does not close it. getFallback keys on the declaring class, while addFallbackEntries keys on the class listed in @HttpServiceFallback(service = ...). When a service interface inherits its method from a super-interface those differ regardless of nesting. Driving CircuitBreakerRequestValueProcessor on this branch with ChildProbeService.class.getMethod("test", String.class, Integer.class), where ChildProbeService extends BaseProbeService, and registering the fallback under ChildProbeService.class.getName():

declaringClass  = ...ProbeInheritedTests$BaseProbeService
registrationKey = ...ProbeInheritedTests$ChildProbeService
attribute       = ...ProbeInheritedTests$BaseProbeService
-> NoFallbackAvailableException: No fallback available.

A JDK proxy over Child extends Base hands the invocation handler the same Base-declared Method, so the proxy path should behave the same way, though I did not drive a full HttpServiceProxyFactory client end to end. Closing it means carrying the service interface rather than the declaring class, which is a wider change than this one - probably its own issue rather than scope creep here.

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.

3 participants