Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions client-v2/src/main/java/com/clickhouse/client/api/Client.java
Original file line number Diff line number Diff line change
Expand Up @@ -288,21 +288,33 @@ public Builder() {
* <ul>
* <li>{@code http://localhost:8123}</li>
* <li>{@code https://localhost:8443}</li>
* <li>{@code http://localhost:8123/clickhouse} (with path for reverse proxy scenarios)</li>
* </ul>
*
* @param endpoint - URL formatted string with protocol, host and port.
* @param endpoint - URL formatted string with protocol, host, port, and optional path.
*/
public Builder addEndpoint(String endpoint) {
try {
URL endpointURL = new URL(endpoint);

if (endpointURL.getProtocol().equalsIgnoreCase("https")) {
addEndpoint(Protocol.HTTP, endpointURL.getHost(), endpointURL.getPort(), true);
} else if (endpointURL.getProtocol().equalsIgnoreCase("http")) {
addEndpoint(Protocol.HTTP, endpointURL.getHost(), endpointURL.getPort(), false);
} else {
if (!endpointURL.getProtocol().equalsIgnoreCase("https") &&
!endpointURL.getProtocol().equalsIgnoreCase("http")) {
throw new IllegalArgumentException("Only HTTP and HTTPS protocols are supported");
}

// Build endpoint URL preserving the path but ignoring query parameters
StringBuilder sb = new StringBuilder();
sb.append(endpointURL.getProtocol().toLowerCase());
sb.append("://");
sb.append(endpointURL.getHost());
if (endpointURL.getPort() > 0) {
sb.append(":").append(endpointURL.getPort());
}
String path = endpointURL.getPath();
if (path != null && !path.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sb.append(path);
}
this.endpoints.add(sb.toString());
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
} catch (MalformedURLException e) {
throw new IllegalArgumentException("Endpoint should be a valid URL string, but was " + endpoint, e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1120,6 +1120,71 @@ public void testSNIWithCloud() throws Exception {
}
}

@Test(groups = {"integration"})
public void testEndpointUrlPathIsPreserved() throws Exception {
if (isCloud()) {
return; // mocked server
}

int serverPort = new Random().nextInt(1000) + 10000;
WireMockServer mockServer = new WireMockServer(WireMockConfiguration
.options().port(serverPort)
.notifier(new Slf4jNotifier(true)));
mockServer.start();

try {
// Setup stubs for two virtual ClickHouse instances behind a reverse proxy
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());
Comment on lines +1137 to +1141
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid test. Update description?


mockServer.addStubMapping(WireMock.post(WireMock.urlPathEqualTo("/billing/db"))
.willReturn(WireMock.aResponse()
.withStatus(HttpStatus.SC_OK)
.withHeader("X-ClickHouse-Summary",
"{ \"read_bytes\": \"200\", \"read_rows\": \"20\"}")).build());

// Test sales virtual instance
try (Client salesClient = new Client.Builder()
.addEndpoint("http://localhost:" + serverPort + "/sales/db")
.setUsername("default")
.setPassword(ClickHouseServerForTest.getPassword())
.compressServerResponse(false)
.build()) {

try (QueryResponse response = salesClient.query("SELECT 1").get(10, TimeUnit.SECONDS)) {
Assert.assertEquals(response.getReadBytes(), 100);
}
}

// Test billing virtual instance - also verify query parameters in URL are ignored
try (Client billingClient = new Client.Builder()
.addEndpoint("http://localhost:" + serverPort + "/billing/db?ignored_param=value")
.setUsername("default")
.setPassword(ClickHouseServerForTest.getPassword())
.compressServerResponse(false)
.build()) {

try (QueryResponse response = billingClient.query("SELECT 1").get(10, TimeUnit.SECONDS)) {
Assert.assertEquals(response.getReadBytes(), 200);
}

// Verify that ignored_param is not in the request URL
mockServer.verify(WireMock.postRequestedFor(WireMock.urlPathEqualTo("/billing/db"))
.withoutQueryParam("ignored_param"));
}

// Verify requests were made to the correct paths
mockServer.verify(WireMock.postRequestedFor(WireMock.urlPathEqualTo("/sales/db")));
mockServer.verify(WireMock.postRequestedFor(WireMock.urlPathEqualTo("/billing/db")));

} finally {
mockServer.stop();
}
}

protected Client.Builder newClient() {
ClickHouseNode node = getServer(ClickHouseProtocol.HTTP);
boolean isSecure = isCloud();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@

private List<DriverPropertyInfo> listOfProperties;

private Map<String, String> parseUrl(String url) throws SQLException {

Check warning on line 165 in jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

A "Brain Method" was detected. Refactor it to reduce at least one of the following metrics: LOC from 71 to 64, Complexity from 24 to 14, Nesting Level from 4 to 2, Number of Variables from 17 to 6.

See more on https://sonarcloud.io/project/issues?id=ClickHouse_clickhouse-java&issues=AZsQvtXaNjNj9hSOyS3a&open=AZsQvtXaNjNj9hSOyS3a&pullRequest=2691
Map<String, String> properties = new HashMap<>();
String myURL = null;
try {
Expand All @@ -188,17 +188,47 @@
if (uri.getAuthority().contains(",")) {
throw new SQLException("Multiple endpoints not supported");
}
properties.put(PARSE_URL_CONN_URL_PROP, uri.getScheme() + "://"
+ uri.getRawAuthority()); // will be parsed again later

if (uri.getPath() != null
&& !uri.getPath().trim().isEmpty()
&& !"/".equals(uri.getPath()))
{
properties.put(
ClientConfigProperties.DATABASE.getKey(),
uri.getPath().substring(1));
// Parse path: last segment is database name, everything before is HTTP path
// Example: /proxy/path/mydb -> httpPath=/proxy/path, database=mydb
// Example: /mydb -> httpPath=empty, database=mydb
// Example: /sales/db -> httpPath=/sales, database=db
// Use raw path for splitting to avoid issues with URL-encoded slashes (e.g., %2F)
String rawPath = uri.getRawPath();
String httpPath = "";
String database = null;

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('/');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (lastSlashIndex > 0) {
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
// Path contains a slash (not at position 0), so it has at least two segments.
// Everything before the last slash becomes HTTP path, the last segment is the database.
// Example: "sales/db" -> httpPath="/sales", database="db"
// Example: "api/v1/clickhouse/mydb" -> httpPath="/api/v1/clickhouse", database="mydb"
httpPath = "/" + pathWithoutLeadingSlash.substring(0, lastSlashIndex);

Check warning on line 211 in jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this hard-coded path-delimiter.

See more on https://sonarcloud.io/project/issues?id=ClickHouse_clickhouse-java&issues=AZsQvtXaNjNj9hSOyS3Y&open=AZsQvtXaNjNj9hSOyS3Y&pullRequest=2691
database = URLDecoder.decode(pathWithoutLeadingSlash.substring(lastSlashIndex + 1), StandardCharsets.UTF_8);
} else {
// No slash found (lastSlashIndex == -1), so it's a single segment representing the database name.
// Example: "mydb" -> httpPath="", database="mydb"
database = URLDecoder.decode(pathWithoutLeadingSlash, StandardCharsets.UTF_8);
}
}

// Build connection URL with HTTP path preserved
StringBuilder connectionUrl = new StringBuilder();

Check warning on line 221 in jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Rename "connectionUrl" which hides the field declared at line 48.

See more on https://sonarcloud.io/project/issues?id=ClickHouse_clickhouse-java&issues=AZsQvtXaNjNj9hSOyS3Z&open=AZsQvtXaNjNj9hSOyS3Z&pullRequest=2691
connectionUrl.append(uri.getScheme()).append("://").append(uri.getRawAuthority());
if (!httpPath.isEmpty()) {
connectionUrl.append(httpPath);
}
properties.put(PARSE_URL_CONN_URL_PROP, connectionUrl.toString());

if (database != null && !database.trim().isEmpty()) {
properties.put(ClientConfigProperties.DATABASE.getKey(), database);
}

if (uri.getQuery() != null && !uri.getQuery().trim().isEmpty()) {
for (String pair : uri.getRawQuery().split("&")) {
try {
Expand Down
86 changes: 86 additions & 0 deletions jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -967,4 +967,90 @@
}

}

@Test(groups = {"integration"})
public void testEndpointUrlPathIsPreserved() throws Exception {
if (isCloud()) {
return; // mocked server
}

WireMockServer mockServer = new WireMockServer(WireMockConfiguration
.options().port(9090).notifier(new ConsoleNotifier(false)));
mockServer.start();

try {
// From wireshark dump as C Array - response for SELECT currentUser() AS user, timezone() AS timezone, version() AS version LIMIT 1
char selectServerInfo[] = {

Check warning on line 983 in jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Move the array designators [] to the type.

See more on https://sonarcloud.io/project/issues?id=ClickHouse_clickhouse-java&issues=AZsQvtWVNjNj9hSOyS3W&open=AZsQvtWVNjNj9hSOyS3W&pullRequest=2691
0x03, 0x04, 0x75, 0x73, 0x65, 0x72, 0x08, 0x74,
0x69, 0x6d, 0x65, 0x7a, 0x6f, 0x6e, 0x65, 0x07,
0x76, 0x65, 0x72, 0x73, 0x69, 0x6f, 0x6e, 0x06,
0x53, 0x74, 0x72, 0x69, 0x6e, 0x67, 0x06, 0x53,
0x74, 0x72, 0x69, 0x6e, 0x67, 0x06, 0x53, 0x74,
0x72, 0x69, 0x6e, 0x67, 0x07, 0x64, 0x65, 0x66,
0x61, 0x75, 0x6c, 0x74, 0x03, 0x55, 0x54, 0x43,
0x0b, 0x32, 0x34, 0x2e, 0x33, 0x2e, 0x31, 0x2e,
0x32, 0x36, 0x37, 0x32};

char select1Res[] = {

Check warning on line 994 in jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Move the array designators [] to the type.

See more on https://sonarcloud.io/project/issues?id=ClickHouse_clickhouse-java&issues=AZsQvtWVNjNj9hSOyS3X&open=AZsQvtWVNjNj9hSOyS3X&pullRequest=2691
0x01, 0x01, 0x31, 0x05, 0x55, 0x49, 0x6e, 0x74,
0x38, 0x01};

// URL format: jdbc:clickhouse://host:port/http_path/database
// For /sales/db: http_path=/sales, database=db
// For /billing/db: http_path=/billing, database=db

// Setup stubs for sales virtual instance (path: /sales)
mockServer.addStubMapping(WireMock.post(WireMock.urlPathEqualTo("/sales"))
.withRequestBody(WireMock.matching(".*SELECT 1.*"))
.willReturn(WireMock.ok(new String(select1Res))
.withHeader("X-ClickHouse-Summary",
"{ \"read_bytes\": \"100\", \"read_rows\": \"10\"}")).build());

mockServer.addStubMapping(WireMock.post(WireMock.urlPathEqualTo("/sales"))
.withRequestBody(WireMock.equalTo("SELECT currentUser() AS user, timezone() AS timezone, version() AS version LIMIT 1"))
.willReturn(WireMock.ok(new String(selectServerInfo))
.withHeader("X-ClickHouse-Summary",
"{ \"read_bytes\": \"10\", \"read_rows\": \"1\"}")).build());

// Setup stubs for billing virtual instance (path: /billing)
mockServer.addStubMapping(WireMock.post(WireMock.urlPathEqualTo("/billing"))
.withRequestBody(WireMock.matching(".*SELECT 2.*"))
.willReturn(WireMock.ok(new String(select1Res))
.withHeader("X-ClickHouse-Summary",
"{ \"read_bytes\": \"200\", \"read_rows\": \"20\"}")).build());

mockServer.addStubMapping(WireMock.post(WireMock.urlPathEqualTo("/billing"))
.withRequestBody(WireMock.equalTo("SELECT currentUser() AS user, timezone() AS timezone, version() AS version LIMIT 1"))
.willReturn(WireMock.ok(new String(selectServerInfo))
.withHeader("X-ClickHouse-Summary",
"{ \"read_bytes\": \"10\", \"read_rows\": \"1\"}")).build());

Properties properties = new Properties();
properties.put("compress", "false");

// Test sales virtual instance: /sales/db means http_path=/sales, database=db
String salesJdbcUrl = "jdbc:clickhouse://localhost:" + mockServer.port() + "/sales/db";
try (Connection conn = new ConnectionImpl(salesJdbcUrl, properties);
Statement stmt = conn.createStatement();
ResultSet rs = stmt.executeQuery("SELECT 1")) {
Assert.assertTrue(rs.next());
Assert.assertEquals(rs.getInt(1), 1);
}

// Test billing virtual instance: /billing/db means http_path=/billing, database=db
String billingJdbcUrl = "jdbc:clickhouse://localhost:" + mockServer.port() + "/billing/db";
try (Connection conn = new ConnectionImpl(billingJdbcUrl, properties);
Statement stmt = conn.createStatement();
ResultSet rs = stmt.executeQuery("SELECT 2")) {
Assert.assertTrue(rs.next());
}

// 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")));
Comment on lines +1048 to +1050
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid test


} finally {
mockServer.stop();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ public void testParseURLValid(String jdbcURL, Properties properties,
throws Exception
{
JdbcConfiguration configuration = new JdbcConfiguration(jdbcURL, properties);
assertEquals(configuration.getConnectionUrl(), connectionURL);
assertEquals(configuration.clientProperties, expectedClientProps);
assertEquals(configuration.getConnectionUrl(), connectionURL, "URL: " + jdbcURL);
assertEquals(configuration.clientProperties, expectedClientProps, "URL: " + jdbcURL);
Client.Builder bob = new Client.Builder();
configuration.applyClientProperties(bob);
Client client = bob.build();
Expand All @@ -144,7 +144,7 @@ public void testParseURLInvalid(String jdbcURL) {

@Test(dataProvider = "validURLs")
public void testAcceptsURLValid(String url) throws Exception {
Assert.assertTrue(JdbcConfiguration.acceptsURL(url));
Assert.assertTrue(JdbcConfiguration.acceptsURL(url), "URL: " + url);
}

@Test
Expand Down
Loading