[jdbc-v2, client-v2] Fix configuration handling by normalizing it. #2470
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
There was a problem hiding this comment.
Pull Request Overview
This PR streamlines client settings handling by migrating to a typed Map<String, Object> approach, enhances JSON handling in JDBC tests, and adds table-name parsing in the SQL parser.
- Added table name extraction in
ParsedPreparedStatementand logged it in the SQL parser tests. - Refactored client-side configuration to use
ClientConfigPropertiesand typed maps. - Cleaned up debug output in tests and updated expected values to match new settings behavior.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/SqlParserTest.java | Added debug println for table extraction in parser tests. |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/DataTypeTests.java | Switched from getConnection() to getJdbcConnection(), enabled JSON test, and adjusted annotations. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/ParsedPreparedStatement.java | Added enterTableExprIdentifier to capture table identifiers. |
| client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java | Removed leftover debug println. |
| client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java | Updated expected async_insert value in mock server test. |
| client-v2/src/test/java/com/clickhouse/client/ClientTests.java | Adjusted expected configuration size counts. |
| client-v2/src/main/java/com/clickhouse/client/api/query/QuerySettings.java | Made rawSettings final, added constructor, updated httpHeader usage. |
| client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java | Refactored to use typed config map, removed MapUtils, and updated HTTP client setup. |
| client-v2/src/main/java/com/clickhouse/client/api/query/QuerySettings.java | Ensured consistent builder chaining and default initialization. |
| client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java | Expanded property definitions, added parsing logic for typed values. |
| client-v2/src/main/java/com/clickhouse/client/api/Client.java | Migrated to Map<String, Object> config, merged client and operation settings. |
| clickhouse-client/src/test/java/com/clickhouse/client/ClickHouseServerForTest.java | Removed unreachable debug exception block. |
Comments suppressed due to low confidence (2)
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/SqlParserTest.java:278
- [nitpick] Remove or replace this debug
println. Consider asserting on the extracted table name instead of printing to stdout.
System.out.println(stmt.getTable());
client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java:500
- This method no longer handles server settings (keys starting with
ClientConfigProperties.SERVER_SETTING_PREFIX), so server-side settings inrequestConfigwon't be added as query parameters. Reintroduce iteration overrequestConfigfor keys with the server setting prefix and add them viareq.addParameter(...).
private void addQueryParams(URIBuilder req, Map<String, Object> requestConfig) {
client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Other comments (3)
- client-v2/src/main/java/com/clickhouse/client/api/Client.java (1201-1201) Creating a new InsertSettings object with just the merged settings map might lose any custom behavior or state in the original settings object. Consider adding a copy constructor to InsertSettings or a method to apply settings from one object to another while preserving other state.
- jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/SqlParserTest.java (278-278) This appears to be a debug print statement that was likely added during development. Consider removing it before merging.
- client-v2/src/main/java/com/clickhouse/client/api/query/QuerySettings.java (25-29) The constructor ordering might cause issues. The default constructor is calling a constructor that's defined after it. Consider reordering the constructors to have the parameterized constructor first, followed by the default constructor.
💡 To request another review, post a new comment with "/windsurf-review".
Comment on lines
+1090
to
+1094
| try (Connection conn = getJdbcConnection(props)) { | ||
| try (Statement stmt = conn.createStatement()) { | ||
| try (ResultSet rs = stmt.executeQuery("SELECT * FROM test_json ORDER BY order")) { | ||
| assertTrue(rs.next()); | ||
| assertEquals(rs.getString("json"), json); | ||
|
|
||
| assertEquals(rs.getString("json"), serverJson); |
Contributor
There was a problem hiding this comment.
The JSON test now correctly configures server settings for JSON handling, but it would be more comprehensive to also test retrieving the JSON as an object (using getObject("json")) and verify its structure, not just the string representation.
Client V2 CoverageCoverage Report
Class Coverage
|
mzitnik
requested changes
Jun 29, 2025
| boolean hasPassword = this.configuration.containsKey(ClientConfigProperties.PASSWORD.getKey()); | ||
| boolean customHttpHeaders = this.configuration.containsKey(ClientConfigProperties.httpHeader(HttpHeaders.AUTHORIZATION)); | ||
|
|
||
| if (!(useSslAuth || hasAccessToken || hasUser || hasPassword || customHttpHeaders)) { |
Contributor
There was a problem hiding this comment.
Do we want to mention what exactly is missing
Contributor
Author
There was a problem hiding this comment.
This is when nothing is specified. That we say in the exception message
| boolean customHttpHeaders = this.configuration.containsKey(ClientConfigProperties.httpHeader(HttpHeaders.AUTHORIZATION)); | ||
|
|
||
| if (!(useSslAuth || hasAccessToken || hasUser || hasPassword || customHttpHeaders)) { | ||
| throw new IllegalArgumentException("Username and password (or access token or SSL authentication or pre-define Authorization header) are required"); |
Contributor
There was a problem hiding this comment.
Do we want to wrap it as a client exception?
Contributor
Author
There was a problem hiding this comment.
I think this is fine - it is not even a client yet.
Contributor
|
The PR message is misleading |
mzitnik
approved these changes
Jun 30, 2025
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.
Summary
Closes #2359
Checklist
Delete items not relevant to your PR: