Repository navigation
Conversation
|
|
| } catch (Exception e) { | ||
| throw new ClientException("Failed to construct input stream", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Response close handling accidentally removed
High Severity
TransportResponseImpl no longer swallows ConnectionClosedException when a response or its body stream is closed after a partial read. Closing now surfaces that drain failure, so QueryResponse.close() can throw ClientException after a mid-stream server abort even when the caller already stopped reading. This is unrelated to the header-value fix and breaks the existing close-after-partial-read path.
Reviewed by Cursor Bugbot for commit e6e7dff. Configure here.
|
Thanks! |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 59f440a. Configure here.
| LOG.warn("failed to construct http client version string", e); | ||
| } | ||
| return userAgent.toString(); | ||
| } |
There was a problem hiding this comment.
User-Agent identity support removed
High Severity
correctUserAgentHeader rebuilds a local defaultUserAgent instead of using WorkloadIdentitySupport.getUserAgent. CLIENT_ID_METADATA is omitted, updateClientName no longer changes the sent header, and the format no longer matches lv/os/hc:ahc_ that tests and ClickHouse client identity expect.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 59f440a. Configure here.


Closes #3227
Summary
This PR addresses an inconsistency in HTTP header value validation within client-v2 that results in configured header values being transmitted under an alternative header name and subsequently ignored by the ClickHouse server.
The existing PATTERN_HEADER_VALUE_ASCII expression restricts whitespace between visible ASCII characters to a single space. Consequently, otherwise representable values containing consecutive spaces or internal tabs are incorrectly classified and subjected to the Header-Name* encoding mechanism, resulting in silent loss of the original header.
Changes Introduced
Verification
Compatibility Considerations
This change corrects the transmission of previously mishandled header values. Values that cannot be represented through the supported mechanism will fail explicitly rather than being transmitted under an alternative header name that the server does not recognize.
Note
Medium Risk
Header behavior changes fix silent drops but may fail requests that previously sent mis-encoded headers; User-Agent composition and response-close semantics also changed in the HTTP transport path.
Overview
Fixes client-v2 HTTP custom header handling in
HttpAPIClientHelper.setHeader:PATTERN_HEADER_VALUE_ASCIInow treats printable ASCII with multiple spaces or tabs between tokens as valid, so values go out on the real header name instead of being rewritten toHeader-Name*(which ClickHouse ignored).Invalid header values no longer get RFC 5987-style
UTF-8''…encoding; the client throws aClientExceptionwith a clear message when a value is outside the supported ASCII rules (e.g. non-ASCII), avoiding silent misconfiguration.The same class also drops
WorkloadIdentitySupportfrom the helper constructor and builds a cacheddefaultUserAgentlocally (buildDefaultUserAgent/correctUserAgentHeaderusingCLIENT_NAMEandClient.CLIENT_USER_AGENT).TransportResponseImplno longer wraps the response body inResponseBodyInputStreamor swallowsConnectionClosedExceptionon close—it uses the entity stream directly.Reviewed by Cursor Bugbot for commit 59f440a. Bugbot is set up for automated code reviews on this repo. Configure here.