[client-v2, jdbc-v2] Handling of unknown parameters #2688
+1,492
−950
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
…ltering out JDBC properties from client ones
…stom_ properties to server
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Other comments (2)
- jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java (65-65) There's a typo in the variable name 'driverPropertiesMapBuidler' which should be 'driverPropertiesMapBuilder'.
-
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcConfigurationTest.java (125-125)
The assertion message uses direct concatenation which could lead to confusing output. Consider using a formatted string instead:
assertEquals(configuration.clientProperties, expectedClientProps, String.format("clientProperties mismatch: %s vs %s", configuration.clientProperties, expectedClientProps));
💡 To request another review, post a new comment with "/windsurf-review".
Comment on lines
344
to
345
| } else if (key.startsWith(CUSTOM_SETTING_PREFIX)) { | ||
| parsedConfig.put(serverSetting(key), tmpMap.remove(key)); |
Contributor
There was a problem hiding this comment.
The handling of custom settings needs to be fixed. Currently, it's adding the server setting prefix to the entire key including 'custom_'. Instead, you should strip the 'custom_' prefix and then add the server setting prefix.
Suggested change
| } else if (key.startsWith(CUSTOM_SETTING_PREFIX)) { | |
| parsedConfig.put(serverSetting(key), tmpMap.remove(key)); | |
| } else if (key.startsWith(CUSTOM_SETTING_PREFIX)) { | |
| parsedConfig.put(serverSetting(key.substring(CUSTOM_SETTING_PREFIX.length())), tmpMap.remove(key)); |
Contributor
Author
There was a problem hiding this comment.
This is correct because server should get custom_my_setting=123. That is why we wrap all with server setting prefix.
Comment on lines
186
to
194
| { | ||
| Properties properties = new Properties(); | ||
| properties.put(DriverProperties.SECURE_CONNECTION.getKey(), "true"); | ||
| properties.put(DriverProperties.SCHEMA_TERM.getKey(), "catalog"); | ||
| try (Connection connection = getJdbcConnection(properties); | ||
| Statement stmt = connection.createStatement()) { | ||
| // | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This test block creates a connection and statement but doesn't perform any assertions or operations. Consider either adding assertions to verify expected behavior or adding a comment explaining the purpose of this test case.
Contributor
Author
There was a problem hiding this comment.
nothing to assert
- we checking that ssl property doesn't cause exception
| } | ||
| } | ||
|
|
||
| driver.connect(getEndpointString() + "?unknown_setting=1&" + ClientConfigProperties.NO_THROW_ON_UNKNOWN_CONFIG + "=1", new Properties()).close(); |
Contributor
There was a problem hiding this comment.
This test connects with an unknown setting and NO_THROW_ON_UNKNOWN_CONFIG flag, but doesn't include any assertions to verify the expected behavior. Consider adding assertions to confirm that the connection was successful and no exceptions were thrown.
Contributor
Author
There was a problem hiding this comment.
if something fails - exception will be thrown at this stage.
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
There was a problem hiding this comment.
Pull request overview
This PR introduces validation for unknown configuration parameters in both the client-v2 and jdbc-v2 libraries. The main change is to throw an exception when an unknown parameter is detected during client build, with an opt-out mechanism via the no_throw_on_unknown_config property. Additionally, the PR fixes how custom parameters are passed to the server (requiring custom_ prefix) and ensures JDBC driver-specific parameters are not incorrectly forwarded to the client.
Key Changes:
- Added validation to throw
ClientMisconfigurationExceptionon unknown config properties unless bypassed - Fixed parameter handling to distinguish between driver-only and client properties
- Corrected custom settings to use proper prefixing for server settings
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java | Added validation logic to throw on unknown config properties; added CUSTOM_SETTINGS_PREFIX property and NO_THROW_ON_UNKNOWN_CONFIG constant |
| client-v2/src/main/java/com/clickhouse/client/api/ServerException.java | Added UNKNOWN_SETTING error code constant |
| client-v2/src/main/java/com/clickhouse/client/api/Client.java | Removed extra blank lines |
| client-v2/src/test/java/com/clickhouse/client/ClientTests.java | Added test coverage for unknown client settings validation |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/DriverProperties.java | Added helper methods for server settings and HTTP headers |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java | Refactored property initialization to separate driver properties from client properties |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcConfigurationTest.java | Updated test data to use custom_ prefix and verify driver properties aren't passed to client |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/DriverTest.java | Added comprehensive test for unknown settings validation; changed assertions to use static imports |
| clickhouse-r2dbc/src/test/java/com/clickhouse/r2dbc/spi/test/R2DBCTestKitImplTest.java | Fixed property name from serverTimezone to server_time_zone |
Comments suppressed due to low confidence (1)
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java:1
- Missing space after the '?' in the ternary operator. For consistency, add a space:
isCloud() ? \"SQL_\" :instead ofisCloud()? \"SQL_\" :
package com.clickhouse.client.api;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java
Outdated
Show resolved
Hide resolved
|
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
no_throw_on_unknown_configto properties. Parsing happens on.build()custom_sslas it is only for driver)Closes #2658
Checklist
Delete items not relevant to your PR: