-
Notifications
You must be signed in to change notification settings - Fork 616
[client-v2] Send parameters in request body #2713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ion stream to proper handle resources.
…e flag - false by default
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
kavirajk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (given my limited Java knowledge 😆 )
Left few minor comments. Approving to unblock
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java
Outdated
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/internal/CompressedEntity.java
Outdated
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java
Outdated
Show resolved
Hide resolved
client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java
Outdated
Show resolved
Hide resolved
|
Thank you, @kavirajk ! I appreciate your review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java
Show resolved
Hide resolved
|
mzitnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM



Summary
USE_HTTP_FORM_REQUEST_FOR_QUERYCloses #2324
Checklist
Delete items not relevant to your PR:
Note
Introduces optional multipart form requests for statement parameters and aligns HTTP/compression handling.
client.http.use_form_request_for_queryand builderuseHttpFormDataForQuery(...)Client.query(...): when enabled and params present, sends multipart; otherwise uses standard POST; inserts/query paths updated to new helper APIsHttpAPIClientHelper: newexecuteRequest(..., String)andexecuteMultiPartRequest(...); refactors URI/context/header builders; wraps entities without passingLZ4Factoryper-call; disables client compression for multipartCompressedEntity/LZ4Entity: correct unknown content length for compressed requests and improve stream lifecycletestMultiPartRequestintegration testWritten by Cursor Bugbot for commit b7eba36. This will update automatically on new commits. Configure here.