Skip to content

Refactor: Tighten Logger, encapsulate Response.Builder, add HttpStatus, fix races - #3469

Open
saikat709 wants to merge 5 commits into
OpenFeign:masterfrom
saikat709:refactor/feign-cleanup
Open

Refactor: Tighten Logger, encapsulate Response.Builder, add HttpStatus, fix races#3469
saikat709 wants to merge 5 commits into
OpenFeign:masterfrom
saikat709:refactor/feign-cleanup

Conversation

@saikat709

Copy link
Copy Markdown

Adds a new top-level HttpStatus enum covering the 19 status codes that s 0
FeignException recognises and uses it to replace the literal 400,
401, 403, …, 504 magic numbers in the fourteen FeignClientException /
FeignServerException subclasses and in the clientErrorStatus /
serverErrorStatus switch statements. Also replaces the 204 / 205
literals in Logger.logAndRebufferResponse. Switches now switch on
HttpStatus constants, so the status-to-exception mapping reads off the
constant names directly.

Four follow-ups in the same files:

  • Logger.Level gets an atLeast(Level) helper. The four
    logLevel.ordinal() >= Level.X.ordinal() comparisons in logRequest,
    logAndRebufferResponse, and logIOException become
    logLevel.atLeast(Level.X). logAndRebufferResponse also loses its
    header loop and body rebuffering into logResponseHeaders(...) and
    rebufferBody(...) private helpers.
  • Response.Builder's seven fields are now private. The
    Response(Builder) constructor in the same file routes through
    package-private getters, restoring the immutability contract the ╮
    class javadoc already advertises.
  • Request.Options.getMethodOptions / setMethodOptions previously
    had a check-then-act race on the ConcurrentHashMap<String, Map<String, Options>> and used a plain HashMap for the inner map. They now use
    computeIfAbsent and a ConcurrentHashMap inner map.
    AsynchronousMethodHandler.CancellableFuture.inner is now an
    AtomicReference<CompletableFuture<T>> so retry callbacks no longer
    race with cancel(). Request.Body.data becomes final; the no-arg
    Body() constructor delegates to this(null). ╮
  • The deprecated RequestTemplate.resolve(Map, Map) overload
    (alreadyEncoded was ignored) is removed. All in-repo callers use
    the single-arg resolve(Map).

No public-API additions beyond HttpStatus and Logger.Level#atLeast.
japicmp is unchanged.

$ ./mvnw -pl core test ╮
... Tests run: 634, Failures: 0, Errors: 0, Skipped: 3

@velo

velo commented Jul 28, 2026

Copy link
Copy Markdown
Member

Thanks for the PR — the Builder encapsulation, deprecated-cleanup, and concurrency fixes are welcome. A few things are blocking merge as-is:

Tests. The PR adds public API (HttpStatus.from()/isClientError()/isServerError(), Logger.Level.atLeast()) and claims two race fixes (CancellableFuture, Options.setMethodOptions), but no test files are added or modified. Each of those needs coverage — especially the race fixes, which should have a test demonstrating the scenario.

CancellableFuture is a partial fix. If cancel() wins the race ahead of setInner(), the inner future is still left running — there's no isCancelled() re-check after inner.set(...). Worth closing that window while you're in there.

HttpStatus.from() cost. It linearly scans a fresh values() clone on every 4xx/5xx exception creation, replacing a branch-free int switch. Consider a static lookup array/map indexed by code.

On HttpStatus itself: committing feign-core to a public status enum is a real API decision, so let's make it pull its weight rather than being an internal lookup detour. One concrete way: overload FeignException.FeignClientException/FeignServerException (and the errorStatus path) to accept HttpStatus alongside the existing int constructors, so users can actually consume the type.

Alternatively, splitting the Builder/race/deprecated-removal half into its own PR (with tests) would let that land quickly while the HttpStatus design settles.

* such and skipped.
*/
@SuppressWarnings("unused")
@Deprecated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please undo this

@velo velo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution. There's real value in here, but I can't merge it in this shape — please split it up and add tests.

Please split

The PR bundles five unrelated changes:

  1. CancellableFuture.innerAtomicReference (race fix)
  2. Request.Options thread-to-method-options map (race fix)
  3. Response.Builder field encapsulation
  4. Removal of the deprecated RequestTemplate.resolve(Map, Map)
  5. A brand-new public feign.HttpStatus enum + Logger.Level#atLeast

Each has a different risk profile and a different reviewer question. Bundled together they have to be accepted or rejected as a unit, which isn't going to happen.

No tests

207 added lines, zero test files. The two concurrency fixes (1 and 2) are the most valuable part of this PR and they are also the most testable — setMethodOptions/getMethodOptions under concurrent access, and CancellableFuture#cancel racing setInner. Please land those first, with tests that fail against the current code.

On HttpStatus

This adds a new public type to feign core, which is a permanent API commitment. As written it enumerates only the 19 codes the exception hierarchy happens to use, and from() returns null for everything else — so users who reach for it will find it half-complete and null-returning. If we want an HTTP status enum in core it needs to be a deliberate design decision (complete set, Optional or a throwing lookup, isRedirect/isSuccess, etc.), not a by-product of removing magic numbers from a switch. Separate PR, please.

Note that the FeignException switch also got slower along the way: HttpStatus.from(status) is a linear scan over values() (which allocates a fresh array each call) on every error response, replacing a constant-time tableswitch on the int.

On the deprecated RequestTemplate.resolve(Map, Map) removal

Deprecated-API removal is being handled as its own batch on the 14.x line. Please drop it from this PR.

Smaller notes

  • Level#atLeast is a nice readability win and I'd take it — but it's public API on a public enum, so it wants a line of test coverage.
  • The logResponseHeaders / rebufferBody extractions in Logger are fine and behaviour-preserving.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants