Skip to content

Refactor Logger level checks and encapsulate Response.Builder fields - #3553

Open
saikat709 wants to merge 1 commit into
OpenFeign:masterfrom
saikat709:refactor/logger-response
Open

Refactor Logger level checks and encapsulate Response.Builder fields#3553
saikat709 wants to merge 1 commit into
OpenFeign:masterfrom
saikat709:refactor/logger-response

Conversation

@saikat709

Copy link
Copy Markdown

Refactors logging level comparisons and encapsulates Response.Builder fields for better API hygiene.

Summary of Changes

  • Logger.Level#atLeast: Adds atLeast(Level other) helper to Logger.Level and replaces ordinal() comparisons across Logger.java.
  • Logger helper extraction: Splits logAndRebufferResponse into logResponseHeaders and rebufferBody private helpers.
  • Response.Builder encapsulation: Makes Builder fields private (status, reason, headers, body, request).

Tests Added

  • LoggerMethodsTest: Adds tests for Logger.Level.atLeast() covering equal, more verbose, and less verbose level comparisons.

$ ./mvnw test -pl core -Dtest=LoggerMethodsTest,LoggerTest,LoggerRebufferTest -Dtoolchain.skip=true
Tests run: 11, Failures: 0, Errors: 0, Skipped: 0

- Logger.Level gains atLeast(Level) replacing ordinal comparisons
- logAndRebufferResponse split into logResponseHeaders/rebufferBody helpers
- Response.Builder fields made private; access is within the same file

Tests: LoggerMethodsTest#atLeast* (new)

@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.

The Level.atLeast() extraction is a good, well-named improvement over the scattered ordinal() comparisons, and shortening logAndRebufferResponse by pulling out logResponseHeaders/rebufferBody is a reasonable move. A few things need fixing before merge:

  1. The six new Response.Builder getters aren't needed. Builder is a static nested class of Response, so the enclosing class already has access to its private fields directly — I verified this compiles fine (mvn -pl core compile) with the getters removed and the constructor reading builder.status/builder.request/etc. directly. The getters add a second, wider access surface (package-visible, no benefit) and collide in name with the existing fluent setters (status() getter vs status(int) setter). Please drop them and keep direct field access from the constructor — the private modifier change alone already achieves the stated goal.

  2. logResponseHeaders(String, Level, Response) takes a logLevel parameter that's never used — the caller already gates the call behind logLevel.atLeast(Level.HEADERS). Please drop the unused parameter.

  3. One spot wasn't converted to the new abstraction: logLevel.compareTo(Level.NONE) > 0 should be logLevel.atLeast(Level.BASIC), consistent with the rest of the refactor.

  4. LoggerMethodsTest.java currently fails the project's own formatting check (git-code-format-maven-plugin / verify-formatting) — there's a stray trailing-whitespace-only blank line before one of the new @test methods. This will fail CI as-is; please run the formatter.

Happy to merge once these are addressed.

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