✨ [client-v2] Statement parameters sent via multipart data request. #2693
Conversation
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
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
Comment on lines
451
to
456
| if (writeCallback != null) { | ||
| // Write callback output to buffer, then add as binary body | ||
| ByteArrayOutputStream buffer = new ByteArrayOutputStream(); | ||
| writeCallback.execute(buffer); | ||
| byte[] queryData = buffer.toByteArray(); | ||
| multipartBuilder.addBinaryBody("query", queryData, CONTENT_TYPE, null); |
Contributor
There was a problem hiding this comment.
This implementation buffers the entire query data in memory before sending it as part of the multipart request. For large queries, this could lead to OutOfMemoryError. Consider using a streaming approach for the query part of the multipart request to avoid memory issues with large queries.
chernser
changed the title
There was a problem hiding this comment.
Pull request overview
This PR modifies the HTTP client to send statement parameters as multipart form data instead of query parameters, addressing issue #2324. The change improves how parameterized queries are transmitted to the ClickHouse server.
Key changes:
- Migrated statement parameter transmission from URL query parameters to multipart form data
- Added integration test with WireMock to verify multipart parameter handling
- Removed legacy query parameter logic for statement parameters
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java | Implements multipart form data construction for statement parameters and removes old query parameter logic |
| client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java | Adds integration test to verify statement parameters are correctly sent as multipart data |
| client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java | Minor whitespace addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
453
to
456
| ByteArrayOutputStream buffer = new ByteArrayOutputStream(); | ||
| writeCallback.execute(buffer); | ||
| byte[] queryData = buffer.toByteArray(); | ||
| multipartBuilder.addBinaryBody("query", queryData, CONTENT_TYPE, null); |
There was a problem hiding this comment.
The ByteArrayOutputStream is not closed after use. While it doesn't hold system resources like file streams, it's good practice to use try-with-resources or explicitly close it to ensure proper resource management and prevent potential memory issues.
Suggested change
| ByteArrayOutputStream buffer = new ByteArrayOutputStream(); | |
| writeCallback.execute(buffer); | |
| byte[] queryData = buffer.toByteArray(); | |
| multipartBuilder.addBinaryBody("query", queryData, CONTENT_TYPE, null); | |
| try (ByteArrayOutputStream buffer = new ByteArrayOutputStream()) { | |
| writeCallback.execute(buffer); | |
| byte[] queryData = buffer.toByteArray(); | |
| multipartBuilder.addBinaryBody("query", queryData, CONTENT_TYPE, null); | |
| } |
|
|
||
| requestEntity = multipartBuilder.build(); | ||
| // Update content type header for multipart | ||
| req.setHeader(HttpHeaders.CONTENT_TYPE, requestEntity.getContentType()); |
There was a problem hiding this comment.
Overwriting the Content-Type header may discard the content encoding that was previously set. If compression is enabled and a Content-Encoding header was added earlier, this replacement could cause issues with the multipart boundary. Consider preserving or merging headers appropriately.
Suggested change
| req.setHeader(HttpHeaders.CONTENT_TYPE, requestEntity.getContentType()); | |
| req.setHeader(HttpHeaders.CONTENT_TYPE, requestEntity.getContentType()); | |
| // Restore Content-Encoding header if it was present before | |
| if (contentEncoding != null) { | |
| req.setHeader(HttpHeaders.CONTENT_ENCODING, contentEncoding); | |
| } |
| // Write callback output to buffer, then add as binary body | ||
| ByteArrayOutputStream buffer = new ByteArrayOutputStream(); | ||
| writeCallback.execute(buffer); | ||
| byte[] queryData = buffer.toByteArray(); |
There was a problem hiding this comment.
The 'query' part name in the multipart body should be documented. It's unclear if this is a ClickHouse protocol requirement or an implementation detail. Adding a comment explaining the expected part name would improve maintainability.
Suggested change
| byte[] queryData = buffer.toByteArray(); | |
| byte[] queryData = buffer.toByteArray(); | |
| // The part name "query" is required by the ClickHouse HTTP protocol for the main query body in multipart requests. | |
| // Do not change this unless the protocol changes, as the server expects this exact part name. |
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank line added. This appears to be unintentional whitespace that doesn't add value to the code.
Suggested change
| } | ||
|
|
||
| WireMockServer mockServer = new WireMockServer(WireMockConfiguration | ||
| .options().port(9090).notifier(new ConsoleNotifier(false))); |
There was a problem hiding this comment.
Hardcoded port 9090 may cause test failures if the port is already in use. Consider using dynamic port allocation (port 0) and retrieving the actual port via mockServer.port() to avoid port conflicts in CI/CD environments.
Suggested change
| .options().port(9090).notifier(new ConsoleNotifier(false))); | |
| .options().port(0).notifier(new ConsoleNotifier(false))); |
|
|
||
| QueryResponse response = client.query( | ||
| "SELECT {testParam:String} as col1, {anotherParam:Int32} as col2", | ||
| queryParams).get(10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
The timeout value of 10 seconds is a magic number. Consider extracting this to a named constant (e.g., TEST_TIMEOUT_SECONDS) for better maintainability and consistency across tests.
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…dy to stream instead of copy
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1029
to
+1030
| if (mimeType != null && mimeType.contains("/")) { | ||
| return mimeType.substring(0, mimeType.indexOf('/')); |
There was a problem hiding this comment.
The getMediaType() method uses contains('/') check before indexOf('/'), but both checks could fail for the same input. If contains('/') returns true, indexOf('/') is guaranteed to return a valid index >= 0, but using indexOf directly would be more efficient. Consider checking indexOf('/') >= 0 instead to avoid two string traversals.
Suggested change
| if (mimeType != null && mimeType.contains("/")) { | |
| return mimeType.substring(0, mimeType.indexOf('/')); | |
| if (mimeType != null) { | |
| int idx = mimeType.indexOf('/'); | |
| if (idx >= 0) { | |
| return mimeType.substring(0, idx); | |
| } |
Comment on lines
+1038
to
+1039
| if (mimeType != null && mimeType.contains("/")) { | ||
| return mimeType.substring(mimeType.indexOf('/') + 1); |
There was a problem hiding this comment.
The getSubType() method uses contains('/') check before indexOf('/'), but both checks could fail for the same input. If contains('/') returns true, indexOf('/') is guaranteed to return a valid index >= 0, but using indexOf directly would be more efficient. Consider checking indexOf('/') >= 0 instead to avoid two string traversals.
Suggested change
| if (mimeType != null && mimeType.contains("/")) { | |
| return mimeType.substring(mimeType.indexOf('/') + 1); | |
| if (mimeType != null) { | |
| int idx = mimeType.indexOf('/'); | |
| if (idx >= 0) { | |
| return mimeType.substring(idx + 1); | |
| } |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request updates the way statement parameters are sent in HTTP requests by switching from query parameters to multipart form data, and adds a new integration test to verify this behavior. The changes also include necessary import updates and cleanup of the old parameter handling logic.
HTTP request handling improvements:
KEY_STATEMENT_PARAMS) are now sent as multipart form data instead of as query parameters. The request body is constructed usingMultipartEntityBuilderto include both the query and the statement parameters as separate parts. (client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java)client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java)client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java)Testing enhancements:
testStatementParamsSentAsMultipartto verify that statement parameters are correctly sent as multipart form data and received by the server. This test uses WireMock to assert the presence of parameter parts in the request. (client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java)client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java)Miscellaneous:
client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java)## SummaryCloses #2324
Checklist
Delete items not relevant to your PR: