✨[client-v2, jdbc-v2] Allow setting http path in URL #2691
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}
| if (rawPath != null && !rawPath.trim().isEmpty() && !"/".equals(rawPath)) { | ||
| // Remove leading slash for processing | ||
| String pathWithoutLeadingSlash = rawPath.startsWith("/") ? rawPath.substring(1) : rawPath; | ||
| int lastSlashIndex = pathWithoutLeadingSlash.lastIndexOf('/'); |
Contributor
There was a problem hiding this comment.
The current implementation might not handle URL-encoded slashes (%2F) correctly. When determining the database name using lastIndexOf('/'), encoded slashes will be treated as regular characters, but when they're part of a path segment they should not be treated as separators. Consider using URI path segment parsing or explicitly handling encoded slashes.
| httpPath = "/" + pathWithoutLeadingSlash.substring(0, lastSlashIndex); | ||
| // Decode the database name | ||
| try { | ||
| database = URLDecoder.decode(pathWithoutLeadingSlash.substring(lastSlashIndex + 1), StandardCharsets.UTF_8.name()); |
Contributor
There was a problem hiding this comment.
The database name is being decoded twice - once implicitly by URI.getPath() and then explicitly with URLDecoder. This could cause issues if the database name contains characters that look like percent-encoded sequences. Consider using uri.getRawPath() consistently throughout this method to avoid double-decoding issues.
Contributor
Author
There was a problem hiding this comment.
No, that statement is FALSE for the current implementation.
Looking at the current code:
// Line 197: Gets the RAW (encoded) pathString rawPath = uri.getRawPath();// Lines 211/219: Explicitly decode the database namedatabase = URLDecoder.decode(pathWithoutLeadingSlash.substring(...), StandardCharsets.UTF_8.name());
The implementation is correct:
uri.getRawPath() returns the path without decoding (e.g., /data%2Fbase stays as /data%2Fbase)
URLDecoder.decode() is called once to decode only the database name segment
There's no double decoding because:
uri.getPath() → decodes automatically (not used for database extraction)
uri.getRawPath() → keeps encoding intact (this is what we use)
Then we manually decode once with URLDecoder.decode()
If I had used uri.getPath() + URLDecoder.decode(), then yes, that would cause double decoding. But the current implementation correctly uses getRawPath().
Example:
Input: jdbc:clickhouse://localhost/data%2Fbase
rawPath = /data%2Fbase (still encoded)
After URLDecoder.decode("data%2Fbase") → data/base (decoded once)
| sb.append(":").append(endpointURL.getPort()); | ||
| } | ||
| String path = endpointURL.getPath(); | ||
| if (path != null && !path.isEmpty()) { |
Contributor
There was a problem hiding this comment.
The path null check is redundant since URL.getPath() never returns null according to Java documentation. However, if you want to keep it for defensive programming, consider using StringUtils.isEmpty() or a similar utility method for cleaner code.
There was a problem hiding this comment.
Pull request overview
This pull request enhances HTTP endpoint URL handling to support custom paths, enabling reverse proxy scenarios and virtual ClickHouse instances. The changes ensure that path components in endpoint URLs are correctly preserved and parsed throughout the client and JDBC layers.
Key changes:
- Modified
Client.Builder.addEndpointto preserve endpoint paths while filtering out query parameters - Updated
JdbcConfiguration.parseUrlto split paths into HTTP path and database name components - Added comprehensive integration tests to verify correct path handling in reverse proxy scenarios
Reviewed changes
Copilot reviewed 5 out of 5 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/Client.java | Refactored endpoint URL building to preserve paths and ignore query parameters |
| client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java | Added integration test for endpoint path preservation with mock reverse proxy scenarios |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java | Enhanced URL parsing to extract HTTP path and database name from path segments |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java | Added integration test verifying correct HTTP path routing in JDBC layer |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcConfigurationTest.java | Improved test assertions with URL context in error messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (path != null && !path.isEmpty()) { | ||
| sb.append(path); | ||
| } | ||
| this.endpoints.add(sb.toString()); |
There was a problem hiding this comment.
The endpoint is added using the reconstructed URL string, but the original protocol-specific logic that called addEndpoint(Protocol.HTTP, endpointURL.getHost(), endpointURL.getPort(), isSecure) has been removed. This change bypasses the existing endpoint addition logic that may perform additional validation or configuration. The endpoints list appears to expect properly configured endpoint objects, not raw URL strings.
| String pathWithoutLeadingSlash = rawPath.startsWith("/") ? rawPath.substring(1) : rawPath; | ||
| int lastSlashIndex = pathWithoutLeadingSlash.lastIndexOf('/'); | ||
|
|
||
| if (lastSlashIndex > 0) { |
There was a problem hiding this comment.
The condition lastSlashIndex > 0 should be lastSlashIndex >= 0 to handle cases where the path has exactly one segment after the leading slash (e.g., /sales/db where lastSlashIndex would be 5, which is > 0, but /a/b where lastSlashIndex would be 1). The current logic is correct for paths with multiple segments, but the comment on line 207 says 'multiple segments' which may be misleading. However, if the intent is to only treat paths with at least two segments (one for HTTP path, one for database) as having an HTTP path component, then the condition should be lastSlashIndex > 0 with a clearer comment.
Comment on lines
+1137
to
+1141
| mockServer.addStubMapping(WireMock.post(WireMock.urlPathEqualTo("/sales/db")) | ||
| .willReturn(WireMock.aResponse() | ||
| .withStatus(HttpStatus.SC_OK) | ||
| .withHeader("X-ClickHouse-Summary", | ||
| "{ \"read_bytes\": \"100\", \"read_rows\": \"10\"}")).build()); |
There was a problem hiding this comment.
The test stubs expect requests to /sales/db and /billing/db (full paths including database name), but the PR description states that the HTTP path should be /sales and /billing with the database name being db. This inconsistency suggests the test may not be validating the intended behavior of separating HTTP path from database name.
Contributor
Author
There was a problem hiding this comment.
This is valid test. Update description?
Comment on lines
+1048
to
+1050
| // Verify requests were made to the correct HTTP paths (/sales and /billing, not /sales/db) | ||
| mockServer.verify(WireMock.postRequestedFor(WireMock.urlPathEqualTo("/sales"))); | ||
| mockServer.verify(WireMock.postRequestedFor(WireMock.urlPathEqualTo("/billing"))); |
There was a problem hiding this comment.
The verification expects requests to /sales and /billing, but the stubs are configured for /sales and /billing (lines 1003, 1009, 1016, 1022). However, the test configuration uses /sales/db and /billing/db as the JDBC URLs, which should result in HTTP paths of /sales and /billing with database name db. The stubs need to be verified against the correct paths. If the implementation correctly strips the database name, the verifications are correct but the stub setup should also match these paths.
Comment on lines
210
to
222
| try { | ||
| database = URLDecoder.decode(pathWithoutLeadingSlash.substring(lastSlashIndex + 1), StandardCharsets.UTF_8.name()); | ||
| } catch (UnsupportedEncodingException e) { | ||
| throw new SQLException("Failed to decode database name", e); | ||
| } | ||
| } else { | ||
| // Single segment: it's the database name, no HTTP path | ||
| // Decode the database name | ||
| try { | ||
| database = URLDecoder.decode(pathWithoutLeadingSlash, StandardCharsets.UTF_8.name()); | ||
| } catch (UnsupportedEncodingException e) { | ||
| throw new SQLException("Failed to decode database name", e); | ||
| } |
There was a problem hiding this comment.
The StandardCharsets.UTF_8.name() call cannot throw UnsupportedEncodingException since UTF-8 is guaranteed to be supported. Consider using StandardCharsets.UTF_8 directly with the newer URLDecoder.decode(String, Charset) method to avoid the unnecessary try-catch block, or acknowledge that this exception will never occur in practice.
Suggested change
| try { | |
| database = URLDecoder.decode(pathWithoutLeadingSlash.substring(lastSlashIndex + 1), StandardCharsets.UTF_8.name()); | |
| } catch (UnsupportedEncodingException e) { | |
| throw new SQLException("Failed to decode database name", e); | |
| } | |
| } else { | |
| // Single segment: it's the database name, no HTTP path | |
| // Decode the database name | |
| try { | |
| database = URLDecoder.decode(pathWithoutLeadingSlash, StandardCharsets.UTF_8.name()); | |
| } catch (UnsupportedEncodingException e) { | |
| throw new SQLException("Failed to decode database name", e); | |
| } | |
| database = URLDecoder.decode(pathWithoutLeadingSlash.substring(lastSlashIndex + 1), StandardCharsets.UTF_8); | |
| } else { | |
| // Single segment: it's the database name, no HTTP path | |
| // Decode the database name | |
| database = URLDecoder.decode(pathWithoutLeadingSlash, StandardCharsets.UTF_8); |
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
chernser
changed the title
chernser
changed the title
|
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
This pull request improves support for HTTP endpoints with custom paths, which is important for scenarios involving reverse proxies or virtual ClickHouse instances. It ensures that endpoint URLs with paths are correctly parsed and preserved in both the core client and JDBC layers. Comprehensive integration tests are added to verify the new behavior.
Core client improvements:
Client.Builder.addEndpointmethod now preserves the full endpoint path (excluding query parameters) when adding endpoints, enabling support for URLs likehttp://localhost:8123/clickhousefor reverse proxy scenarios.JDBC layer enhancements:
JdbcConfiguration.parseUrlmethod now splits the path into an HTTP path and a database name, ensuring the HTTP path is preserved in the connection URL and the database name is set correctly. For example,/proxy/path/mydbis parsed ashttpPath=/proxy/pathanddatabase=mydb.Testing improvements:
HttpTransportTests) and JDBC (ConnectionTest) to verify that endpoint paths are correctly preserved and requests are routed to the correct HTTP paths. These tests simulate reverse proxy scenarios and check that query parameters in the endpoint URL are ignored. [1] [2]Test assertion enhancements:
JdbcConfigurationTestto include the tested URL in error messages for better debugging. [1] [2]Closes [client-v2] Support changing path name in the URL #2344
Checklist
Delete items not relevant to your PR: