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.
Why it was not fixed in #11
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.
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.DefaultHTTPClientConfigaccepts negative timeout values without validation, butDefaultHTTPClientpasses them straight to Apache HC5:Timeout.ofMilliseconds(config.getConnectTimeout())andTimeout.ofMilliseconds(config.getConnectionRequestTimeout())throwIllegalArgumentExceptionon a negative value —TimeoutenforcesArgs.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-1and interprets it as an indefinite keep-alive, which is unlikely to be what a caller passing-1intended.Verified against the current code: the factual claims hold.
Why it was not fixed in #11
DefaultHTTPClientConfigandDefaultHTTPClientare untouched by that branch —git diff origin/main...HEADover both files is empty. The permissive setters are pre-existing behaviour onmain, 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 exerciseDefaultHTTPClient.Options
IllegalArgumentExceptionat the point of the mistake. Breaking for existing callers; needs a release note and a major/minor version decision.DefaultHTTPClientnormalises 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.-1keep-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.