refactor(java)!: switch idioms, deprecations, Lombok, warning gate - #198
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, your pull request is larger than the review limit of 150000 diff characters
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change replaces string authentication requirements with the public ChangesGateway typing and cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java (1)
361-365: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the shared
WebSocketClienton shutdown.
createWebSocketClient()creates one long-lived client for the edge, butonShutdown()only drains in-flight requests. Add an explicit shutdown/close path forwebSocketRelayStage’sWebSocketClientto release pooled connections and avoid leaving resources active after the edge shuts down.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a12040d0-e56d-4574-9883-66d984a9216c
📒 Files selected for processing (35)
.claude/skills/run-integration-tests/SKILL.mdCLAUDE.mdapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/auth/AuthenticationStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/RouteTableBuilder.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/load/ConfigLoader.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/AuthConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/Require.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigModelReflection.javaapi-sheriff/src/main/resources/application.propertiesapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/runtime/SessionAuthenticationStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/DocumentedSetsContractTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/RouteTableBuilderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/load/ConfigLoaderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/ConfigModelContractTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/topology/TopologyResolverTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorRouteDisjointnessTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgePipelineTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ShippedApplicationPropertiesTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/routing/RouteRuntimeTest.javadoc/development/README.adocintegration-tests/docker-compose.ymlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/ItProfileConfigBindingWiringTest.javapom.xml
Collaborator
Author
Triage dispositionsIn reply to comment_id:
|
cuioss-oliver
added a commit
that referenced
this pull request
Aug 9, 2026
Two valid review findings from the PR #198 automated review: - run-integration-tests SKILL.md claimed ItProfileConfigBindingWiringTest "guards that pairing" for QUARKUS_LOG_FILE_ENABLED and LOG_FILE_PATH. Reading the whole class shows it asserts only the enable flag -- LOG_FILE_PATH and the /logs mount appear nowhere in it. Reworded to claim only the coverage the test actually provides. - AuthConfig was missing the thread-safety note the project's Javadoc standard requires. Added; the require/override prose is untouched. Three further comments were declined with rationale on the threads: a MemorySize.of(long) compile claim refuted by green CI on this head, a request to drop the documented warnings[]/errors[] payload shape that contradicts both the build-execution standard and this plan's own deliverable-9 negative control, and a nitpick to close the shared WebSocketClient on shutdown (Vert.x close hooks already release it, and onShutdown is the drain observer -- closing there would tear down the in-flight relays the drain exists to let finish). Refs #178
Add <showDeprecation>true</showDeprecation> beside <release>25</release> in the pluginManagement maven-compiler-plugin entry, so javac reports deprecated-API use across the whole reactor instead of collapsing it to a summary note. Every compile/testCompile line now runs javac [debug deprecation release 25] and the five known deprecation sites surface as warnings. failOnWarning is deliberately NOT set here - that is deliverable 9, and turning it on now would fail the build on the sites deliverable 6 has not yet retired. api-sheriff/pom.xml is left alone: it configures annotationProcessorPaths only, and the parent's pluginManagement entry already reaches it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBJYx6sqpfKg5tV1fJ1v4E
…e.enabled Quarkus 3.26 deprecated quarkus.log.file.enable for removal and replaced it with quarkus.log.file.enabled. Verified against the shipped artifact rather than the docs: io.quarkus.runtime.logging.LogRuntimeConfig.FileConfig in quarkus-core-3.38.1.jar declares a deprecated Optional<Boolean> enable() annotated @deprecated(since="3.26", forRemoval=true) alongside the replacement boolean enabled() annotated @WithDefault("false"). Migrated every occurrence across all six carriers - application.properties, integration-tests/docker-compose.yml (all seven gateway services move together, since the enable flag and LOG_FILE_PATH are one decision), doc/development/README.adoc, .claude/skills/run-integration-tests/SKILL.md, and the two guards that assert on the key (ShippedApplicationPropertiesTest, ItProfileConfigBindingWiringTest). A repo-wide sweep now reports zero remaining occurrences of the deprecated spelling. The shipped OFF-by-default posture (ADR-0032) is preserved and in fact expressed more strongly: the replacement carries @WithDefault("false") where the deprecated accessor had no default at all. The env-var spelling follows the same mechanical UPPER_SNAKE transform, so QUARKUS_LOG_FILE_ENABLE becomes QUARKUS_LOG_FILE_ENABLED. Refs #178 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBJYx6sqpfKg5tV1fJ1v4E
Replaces the three duplicated REQUIRE_* string-constant sets in ConfigValidator, AuthenticationStage and GatewayEdgeRoute with one shared enum, making the posture dispatch an exhaustive compiler-checked switch. Require.toString() keeps the lowercase config spelling so operator-facing messages and the ROUTE_POSTURE log line stay byte-identical. DocumentedSetsContractTest now binds the enum to the require arrays of both bundled schemas.
Converts ConfigLoader.substitute (ObjectNode/ArrayNode type dispatch) and the two GatewayEdgeRoute chains (protocol dispatch, rejection-WARN dispatch) to switches with identical branch selection. The other 16 enumerated files carry no chain a switch expresses without changing selection; per-file verdicts are recorded in the plan's work/d5-switch-survey.md for the PR body.
Replaces the two deprecated HttpServerRequest.host() fallback arms in GatewayEdgeRoute with one shared authorityHost() seam, migrates the WebSocket dial from the deprecated HttpClient.webSocket(WebSocketConnectOptions) to an edge-wide Vert.x WebSocketClient, and swaps the two deprecated-for-removal MemorySize(BigInteger) test constructions for MemorySize.of(long). No dependency was added and pom.xml is untouched; no in-code deprecation suppression was introduced. A clean whole-reactor test-compile now carries zero deprecation warnings.
Adds failOnWarning to the pluginManagement compiler-plugin entry beside showDeprecation, so javac runs with -Werror across all six modules and a deprecation stops scrolling past in the log. Affordable only because every site was retired by migration rather than suppression: a clean whole-reactor test-compile is deprecation-free and this plan added no @SuppressWarnings. Negative control: re-introducing HttpServerRequest.host() failed the build naming GatewayEdgeRoute.java:1169, and the scratch edit was reverted. CLAUDE.md's Pre-Commit Process now describes what the gate actually enforces, including that the fix is migration and not suppression.
The pre-submission self-review found that AuthenticationStage.process relied on a compile-time guarantee that does not exist. A switch statement whose labels are all enum constants is a legacy switch: javac neither requires it to be exhaustive nor warns about it. Verified under this project's exact flags (--release 25 -Xlint:all -Werror), a switch missing a constant compiles clean and falls through silently. That matters here because the plan had removed the pre-existing fail-closed backstop on the strength of that guarantee. A future fourth Require constant would have made process() a silent no-op for that posture while RouteTableBuilder.effectiveAccessLevel still reported the route AUTHENTICATED, so anchor-floor checks would pass -- a fail-open auth bypass with no test and no gate behind it. Adding a `case null` arm makes it an enhanced switch, which javac IS required to check: a missing constant is now a compile error. The arm is unreachable at runtime (AuthConfig's canonical constructor rejects null) and doubles as the restored fail-closed backstop. Also corrects two Javadoc claims the review refuted: - Require's class doc no longer asserts automatic compiler-checked exhaustiveness, and explains why the case-null arm is load-bearing. - GatewayEdgeRoute.authorityHost no longer claims host() and authority().host() agree. host() returns the raw Host header including the port, so they differ whenever a port is present; substituting it back would leak the port into the reserved-path match and the security-validated request host. The real reason dropping the fallback is behaviour-preserving is stated instead. Refs #178
Two valid review findings from the PR #198 automated review: - run-integration-tests SKILL.md claimed ItProfileConfigBindingWiringTest "guards that pairing" for QUARKUS_LOG_FILE_ENABLED and LOG_FILE_PATH. Reading the whole class shows it asserts only the enable flag -- LOG_FILE_PATH and the /logs mount appear nowhere in it. Reworded to claim only the coverage the test actually provides. - AuthConfig was missing the thread-safety note the project's Javadoc standard requires. Added; the require/override prose is untouched. Three further comments were declined with rationale on the threads: a MemorySize.of(long) compile claim refuted by green CI on this head, a request to drop the documented warnings[]/errors[] payload shape that contradicts both the build-execution standard and this plan's own deliverable-9 negative control, and a nitpick to close the shared WebSocketClient on shutdown (Vert.x close hooks already release it, and onShutdown is the drain observer -- closing there would tear down the in-flight relays the drain exists to let finish). Refs #178
cuioss-oliver
force-pushed
the
feature/plan-v02-02-java-idiom-sweep
branch
from
August 9, 2026 03:32
d07800a to
be0eeba
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One idiom sweep over the
api-sheriffcorpus, bracketed between the two halves of the buildmechanism that keeps it from silently regressing:
<showDeprecation>goes on first so thecompiler — not grep — enumerates the deprecation set, and
<failOnWarning>goes on last, oncethe corpus is clean.
This is a deliberate BREAKING change (
compatibility: breaking). Per the repository's Pre-1.0Rules there are no deprecation markers, no transitional shims and no backward-compatibility path:
AuthConfig.requirechanges type fromStringto a boundRequireenum, the deprecatedquarkus.log.file.enableproperty is replaced outright byquarkus.log.file.enabled, andfailOnWarningnow makes a compiler warning a build failure across all six reactor modules.Anything downstream that constructs an
AuthConfigwith a string, sets the old property, orcompiles with a warning present will break — intentionally.
Changes
The bracket
pom.xml—<showDeprecation>true</showDeprecation>added to thepluginManagementmaven-compiler-pluginentry (opens the bracket), then<failOnWarning>true</failOnWarning>(closes it). Reactor-wide, all six modules; javac now runs with
-Werror.CLAUDE.md— the Pre-Commit Process prose reconciled with what the gate now actuallyenforces, so the mechanism and its description agree.
AuthConfig.require:String→Requireenumapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/Require.java; consumersmoved in lockstep across
AuthConfig.java,ConfigValidator.java,AuthenticationStage.java,GatewayEdgeRoute.javaandRouteTableBuilder.java; the enum registered inConfigModelReflection.javafor the native image. Three duplicatedREQUIRE_*constant setsretired. No YAML/doc value moves —
ConfigLoaderenablesACCEPT_CASE_INSENSITIVE_ENUMS, sonone/bearer/sessionkeep their lowercase spelling on the config surface.Deprecated
quarkus.log.file.enablemigrationapi-sheriff/src/main/resources/application.properties,integration-tests/docker-compose.yml(7, env spelling),doc/development/README.adoc,.claude/skills/run-integration-tests/SKILL.md,ShippedApplicationPropertiesTest.java,ItProfileConfigBindingWiringTest.java.io.quarkus.runtime.logging.LogRuntimeConfig.FileConfiginquarkus-core-3.38.1declares boththe deprecated
Optional<Boolean> enable()(@Deprecated(since = "3.26", forRemoval = true)) andthe replacement
boolean enabled()(@WithDefault(false)). Key →quarkus.log.file.enabled,env →
QUARKUS_LOG_FILE_ENABLED. The shipped OFF-by-default posture (ADR-0032) is preservedand expressed more strongly: the replacement carries a framework-level
falsedefault where thedeprecated accessor had none.
Java deprecation retirement (enumerated by the compiler, not by grep)
WebSocketRelayStage.java, migrated offthe deprecated
HttpClientWebSocket path onto the Vert.xWebSocketClient; the twoHttpServerRequest.host()fallbacks inGatewayEdgeRoute.javaare retired againstauthority().Surveys (published enumeration is the deliverable)
switch: 19 files surveyed, 3 chains converted inConfigLoader.javaandGatewayEdgeRoute.java.TokenValidatorProducer.applyJwksqualifies but sits outside thedeliverable's declared footprint and was deliberately deferred rather than smuggled in.
java-lombokdecision table: 30 files surveyed, zero mutations. 23@Builder-on-record and 6@UtilityClasssites match the table verbatim;RouteRuntime.javaiskept as a
@Builder @Getterfinal class because its four@Builder.Defaultfields cannot live ona record — a conversion would trade declarative fail-closed defaults for a hand-written partial
builder.
Every OpenRewrite jar in the resolved repository was swept (
UseUnnamed/UnnamedVariable/UnnamedPattern) with zero hits; the activeUpgradeToJava25recipe list(
rewrite-migrate-java3.40.0) andrewrite-static-analysis2.39.0 declare none. No recipe wasappended and
pom.xmlcarries no change from this deliverable. Residual, deliberately left open:a module the build does not resolve might carry one — adding a module is a new dependency and
out of scope.
Operator decisions (recorded verbatim, and load-bearing on this review)
Two questions were put to the operator during outline authoring. Both answers are reproduced here
because the authority behind them should be auditable rather than inferred.
q1 — the enum-conversion rationale. Asked because the outline's own first-party verification
had REFUTED both justifications the request gave for the conversion, leaving the change without the
rationale it was requested on.
The operator re-confirmed the full conversion after reading the refutation. Deliverable 4
therefore proceeds on a narrowed, type-safety-only rationale, and a reviewer should hold it to
exactly that and no more:
duplicated
REQUIRE_*constant sets (AuthenticationStage,ConfigValidator,GatewayEdgeRoute).and no security improvement. The claim that "nothing rejects an unknown
requirevalue today"is false:
gateway.schema.jsonandendpoint.schema.jsonalready declare"require": {"type": "string", "enum": ["none", "bearer", "session"]}, andConfigLoaderrunsschema validation before bind, so
require: bearerris already refused at load time as acollected
ConfigError. It never reachesAuthenticationStage. The startup-rejection behaviourthis change was originally pitched on already existed.
q2 — the WebSocket client dependency. Asked because the epic carries a "never add dependencies
or plugins without explicit user approval" clause and the
WebSocketRelayStagefix appeared to needa new artifact.
That is the explicit operator approval the clause requires — and the approval went unused. It
was treated as a permission, not an instruction: the outline first verified whether an artifact
was genuinely required, and it is not.
io.vertx:vertx-core:4.5.30is already on theapi-sheriffcompile classpath and already contains
io/vertx/core/http/WebSocketClient.class, withVertx.createWebSocketClient()declared onio.vertx.core.Vertxin the same jar.No new Maven artifact is added by this PR. Recording the approval anyway matters: a reviewer
seeing "no dependency added" should be able to tell the difference between a constraint that
blocked a fix and a fix that turned out not to need the allowance it was granted. This is the
second.
Pre-submission self-review: two contract-drift defects caught and fixed
The self-review pass before submission found and fixed two defects, both landed in
b94f17b. Theyare named here rather than buried in the diff, because both are the kind a green suite does not
catch:
Requiredispatch was not compiler-checked for exhaustiveness. A constant-onlyswitchstatement is a legacy switch, so it carries no exhaustiveness requirement — a future fourth
Requireconstant would have fallen through silently while the route still reported itselfAUTHENTICATED. That is a fail-open auth path. Converted to a form the compiler must proveexhaustive.
authorityHostJavadoc asserted a false equivalence betweenhost()andauthority().host(). The two are not interchangeable in the case the fallback exists to handle;the doc now describes what the code actually does.
Test Plan
verify -Ppre-commit)verify)WebSocketProxyIT8/8 green against a freshly built native image (111 integration teststotal) — the WebSocket client swap is the only framework-boundary change here and the native
image is where a Vert.x client swap historically surprises, so a JVM-only pass was not
accepted as discharge
failOnWarningon, a deliberately re-introduceddeprecated
HttpServerRequest.host()call failed the build (status: error,exit_code: 1), with the offending site reported asGatewayEdgeRoute.java:1169. Worthrecording precisely: the file and line land on the
warnings[]row whileerrors[]carries the
-Werrorcause naming the file without a line — so the payload does carryfile+line, just not both on one
errors[N]row. Scratch edit reverted; the file's diff isempty. A gate that has never been observed to fail is not known to gate.
Related Issues
Refs #178 — the deprecated
quarkus.log.file.enablemigration (6 carriers / 20 occurrences) is thework that issue asks for. It is deliberately not auto-closed by this PR: the issue gets a
comment naming this PR and the merge commit, and is closed post-merge.
Generated by plan-finalize skill
Intent
The problem. Compiler warnings were invisible to this project's agent-facing build surface. The
reactor configured
maven-compiler-pluginwith<release>25</release>only — noshowDeprecation,no
-Xlint, nofailOnWarning— so deprecated-API usage accumulated in the corpus unseen, and theMaven executor's structured payload has no warnings channel to surface it through even when javac
does emit it. Alongside that, four idiom-level defects had accrued: a deprecated Quarkus logging
property, string-typed auth posture triplicated across three classes, if/else-if chains where a
switch belongs, and unaudited Lombok usage.
The approach. Bracket the corpus work between the two halves of the mechanism.
showDeprecationgoes on FIRST, so the deprecation set is enumerated by running the compiler across the whole
reactor rather than by grepping for a guess — that is how five sites in four files were found where
the request had named two in one.
failOnWarninggoes on LAST, once the corpus is clean, because awarning only reaches the executor's
errors[]payload once it is a build failure; lint alone doesnot make it visible to an agent. The order is load-bearing, not stylistic. Every deprecation site is
retired by migrating off the warned construct, never by suppressing it — a
@SuppressWarningsadded to reach green would hollow out the gate while leaving it
[Intent truncated — 1393 of 2806 characters shown; full outline in the plan workspace]
Summary by CodeRabbit
New Features
Bug Fixes
Documentation