You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
DefaultHTTPClientConfig accepts negative timeouts that fail or misbehave in HC5 #14
Split out of the #11 review so it is tracked rather than lost.
Finding
Originally raised by CodeRabbit on core-api/src/test/java/com/absmartly/sdk/DefaultHTTPClientConfigTest.java.
DefaultHTTPClientConfig accepts negative timeout values without validation, but DefaultHTTPClient passes them straight to Apache HC5:
Timeout.ofMilliseconds(config.getConnectTimeout()) and Timeout.ofMilliseconds(config.getConnectionRequestTimeout())throw IllegalArgumentException on a negative value — Timeout enforces Args.notNegative(). The failure surfaces at client construction, not at config time, so the stack trace points away from the actual mistake.
TimeValue.ofMilliseconds(config.getConnectionKeepAlive())accepts-1 and interprets it as an indefinite keep-alive, which is unlikely to be what a caller passing -1 intended.
Verified against the current code: the factual claims hold.
DefaultHTTPClientConfig and DefaultHTTPClient are untouched by that branch — git diff origin/main...HEAD over both files is empty. The permissive setters are pre-existing behaviour on main, and rejecting values they currently accept is a breaking API change for anyone relying on them today. That belongs in its own PR with a release note, not smuggled into a test-coverage branch.
DefaultHTTPClientConfigTest (added by #11) pins only the POJO round-trip: it asserts the config stores what it was given. The finding is right that those cases read as if they bless negative inputs, but they do not exercise DefaultHTTPClient.
Options
Validate in the setters — reject negative values with IllegalArgumentException at the point of the mistake. Breaking for existing callers; needs a release note and a major/minor version decision.
Validate at client construction — DefaultHTTPClient normalises or rejects before handing values to HC5, with a clear message naming the config field. Narrower blast radius, still a behaviour change for anyone currently getting the raw HC5 exception.
Document only — javadoc the accepted ranges and the -1 keep-alive meaning, leave behaviour alone.
Whichever is chosen, DefaultHTTPClientConfigTest's negative-value cases should be updated to assert the decided behaviour rather than the bare round-trip.
cc @joalves — flagging as agreed in the #11 thread.
Activity