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

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