Merged
Conversation
chernser
requested review from
alex-clickhouse and
Copilot
and removed request for
mzitnik
There was a problem hiding this comment.
Pull request overview
Updates how the JDBC v2 driver composes the “default” client name used for the HTTP User-Agent, especially when an application name is provided via config properties (linked to clickhouse-java issue #2775).
Changes:
- Switch default JDBC client name construction to use
Driver.DRIVER_CLIENT_NAME + Driver.getLibraryVersion()(instead of a hardcoded string). - Extend the integration test to validate User-Agent behavior both when client name is set via config and when set at runtime.
- Refactor repeated User-Agent verification logic into a helper method.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java |
Aligns default client name construction with Driver.DRIVER_CLIENT_NAME/version. |
jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java |
Adds/extends integration coverage for User-Agent when client name is provided via config and refactors verification into a helper. |
Comments suppressed due to low confidence (1)
jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java:88
- When CLIENT_NAME is provided but blank (e.g., "" or whitespace), this code path still prefixes the default driver client name with
appName + " ", producing a client name starting with a leading space (" jdbc-v2/"). The HTTP client treats that as a non-empty client name and will emit a User-Agent header that begins with a space, which is undesirable and can break downstream parsing. Consider treating blank/whitespace-only CLIENT_NAME/PRODUCT_NAME as "not set" (e.g., checktrim().isEmpty()before prefixing) so the default client name is used without a leading separator.
String clientName = Driver.DRIVER_CLIENT_NAME + Driver.getLibraryVersion();
Map<String, String> clientProperties = config.getClientProperties();
if (clientProperties.get(ClientConfigProperties.CLIENT_NAME.getKey()) != null) {
this.appName = clientProperties.get(ClientConfigProperties.CLIENT_NAME.getKey()).trim();
clientName = this.appName + " " + clientName; // Use the application name as client name
} else if (clientProperties.get(ClientConfigProperties.PRODUCT_NAME.getKey()) != null) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
mshustov
reviewed
Mar 18, 2026
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
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 #2775
Checklist
Delete items not relevant to your PR: