Skip to content

Commit f8494c1

Browse files
chernserchernser
committed
Merge branch 'main' into fix_statement_impl
2 parents 1451bc7 + 2f3b741 commit f8494c1

File tree

7 files changed

+90
-20
lines changed

7 files changed

+90
-20
lines changed

‎client-v2/src/main/java/com/clickhouse/client/api/Client.java‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
import java.io.InputStream;
5656
import java.io.OutputStream;
5757
import java.lang.reflect.InvocationTargetException;
58+
import java.net.MalformedURLException;
5859
import java.net.URL;
5960
import java.nio.charset.StandardCharsets;
6061
import java.time.Duration;
@@ -121,7 +122,7 @@ public class Client implements AutoCloseable {
121122
private final Map<String, Object> configuration;
122123

123124
private final Map<String, String> readOnlyConfig;
124-
125+
125126
private final POJOSerDe pojoSerDe;
126127

127128
private final ExecutorService sharedOperationExecutor;
@@ -291,7 +292,7 @@ public Builder() {
291292
*/
292293
public Builder addEndpoint(String endpoint) {
293294
try {
294-
URL endpointURL = new java.net.URL(endpoint);
295+
URL endpointURL = new URL(endpoint);
295296

296297
if (endpointURL.getProtocol().equalsIgnoreCase("https")) {
297298
addEndpoint(Protocol.HTTP, endpointURL.getHost(), endpointURL.getPort(), true);
@@ -300,7 +301,7 @@ public Builder addEndpoint(String endpoint) {
300301
} else {
301302
throw new IllegalArgumentException("Only HTTP and HTTPS protocols are supported");
302303
}
303-
} catch (java.net.MalformedURLException e) {
304+
} catch (MalformedURLException e) {
304305
throw new IllegalArgumentException("Endpoint should be a valid URL string, but was " + endpoint, e);
305306
}
306307
return this;
@@ -380,7 +381,7 @@ public Builder setAccessToken(String accessToken) {
380381

381382
/**
382383
* Makes client to use SSL Client Certificate to authenticate with server.
383-
* Client certificate should be set as well. {@link Client.Builder#setClientCertificate(String)}
384+
* Client certificate should be set as well. {@link Builder#setClientCertificate(String)}
384385
* @param useSSLAuthentication
385386
* @return
386387
*/
@@ -1583,8 +1584,9 @@ public CompletableFuture<QueryResponse> query(String sqlQuery, Map<String, Objec
15831584
Endpoint selectedEndpoint = getNextAliveNode();
15841585
RuntimeException lastException = null;
15851586
for (int i = 0; i <= retries; i++) {
1587+
ClassicHttpResponse httpResponse = null;
15861588
try {
1587-
ClassicHttpResponse httpResponse =
1589+
httpResponse =
15881590
httpClientHelper.executeRequest(selectedEndpoint, finalSettings.getAllSettings(), lz4Factory, output -> {
15891591
output.write(sqlQuery.getBytes(StandardCharsets.UTF_8));
15901592
output.close();
@@ -1614,6 +1616,7 @@ public CompletableFuture<QueryResponse> query(String sqlQuery, Map<String, Objec
16141616
return new QueryResponse(httpResponse, responseFormat, finalSettings, metrics);
16151617

16161618
} catch (Exception e) {
1619+
httpClientHelper.closeQuietly(httpResponse);
16171620
lastException = httpClientHelper.wrapException(String.format("Query request failed (Attempt: %s/%s - Duration: %s)",
16181621
(i + 1), (retries + 1), System.nanoTime() - startTime), e);
16191622
if (httpClientHelper.shouldRetry(e, finalSettings.getAllSettings())) {

‎client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -428,8 +428,9 @@ public ClassicHttpResponse executeRequest(Endpoint server, Map<String, Object> r
428428

429429
HttpClientContext context = HttpClientContext.create();
430430

431+
ClassicHttpResponse httpResponse = null;
431432
try {
432-
ClassicHttpResponse httpResponse = httpClient.executeOpen(null, req, context);
433+
httpResponse = httpClient.executeOpen(null, req, context);
433434
boolean serverCompression = ClientConfigProperties.COMPRESS_SERVER_RESPONSE.getOrDefault(requestConfig);
434435
httpResponse.setEntity(wrapResponseEntity(httpResponse.getEntity(), httpResponse.getCode(), serverCompression, useHttpCompression, lz4Factory, requestConfig));
435436

@@ -448,14 +449,26 @@ public ClassicHttpResponse executeRequest(Endpoint server, Map<String, Object> r
448449
return httpResponse;
449450

450451
} catch (UnknownHostException e) {
452+
closeQuietly(httpResponse);
451453
LOG.warn("Host '{}' unknown", server.getBaseURL());
452454
throw e;
453455
} catch (ConnectException | NoRouteToHostException e) {
456+
closeQuietly(httpResponse);
454457
LOG.warn("Failed to connect to '{}': {}", server.getBaseURL(), e.getMessage());
455458
throw e;
456459
}
457460
}
458461

462+
public void closeQuietly(ClassicHttpResponse httpResponse) {
463+
if (httpResponse != null) {
464+
try {
465+
httpResponse.close();
466+
} catch (IOException e) {
467+
LOG.warn("Failed to close response");
468+
}
469+
}
470+
}
471+
459472
private static final ContentType CONTENT_TYPE = ContentType.create(ContentType.TEXT_PLAIN.getMimeType(), "UTF-8");
460473

461474
private void addHeaders(HttpPost req, Map<String, Object> requestConfig) {

‎jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import com.clickhouse.jdbc.internal.ParsedPreparedStatement;
1616
import com.clickhouse.jdbc.internal.SqlParser;
1717
import com.clickhouse.jdbc.metadata.DatabaseMetaDataImpl;
18+
1819
import org.slf4j.Logger;
1920
import org.slf4j.LoggerFactory;
2021

@@ -35,6 +36,7 @@
3536
import java.sql.Savepoint;
3637
import java.sql.Statement;
3738
import java.sql.Struct;
39+
import java.time.Duration;
3840
import java.util.Arrays;
3941
import java.util.Calendar;
4042
import java.util.Collections;
@@ -487,13 +489,15 @@ public SQLXML createSQLXML() throws SQLException {
487489

488490
@Override
489491
public boolean isValid(int timeout) throws SQLException {
490-
checkOpen();
491492
if (timeout < 0) {
492493
throw new SQLException("Timeout must be >= 0", ExceptionUtils.SQL_STATE_CLIENT_ERROR);
493494
}
494-
495-
//TODO: This is a placeholder implementation
496-
return true;
495+
if (isClosed()) {
496+
return false;
497+
}
498+
return timeout == 0
499+
? client.ping()
500+
: client.ping(Duration.ofSeconds(timeout).toMillis());
497501
}
498502

499503
@Override

‎jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import com.clickhouse.client.api.Client;
44
import com.clickhouse.client.api.ClientConfigProperties;
5+
import com.clickhouse.client.api.http.ClickHouseHttpProto;
56
import com.clickhouse.data.ClickHouseDataType;
67
import com.clickhouse.jdbc.Driver;
78
import com.google.common.collect.ImmutableMap;
@@ -129,7 +130,14 @@ private static String createConnectionURL(String url, boolean ssl) throws SQLExc
129130
: url;
130131
try {
131132
URI tmp = URI.create(adjustedURL);
132-
return tmp.toASCIIString();
133+
String asciiString = tmp.toASCIIString();
134+
if (tmp.getPort() < 0) {
135+
String port = ssl || adjustedURL.startsWith("https")
136+
? ":" + ClickHouseHttpProto.DEFAULT_HTTPS_PORT
137+
: ":" + ClickHouseHttpProto.DEFAULT_HTTP_PORT;
138+
asciiString += port;
139+
}
140+
return asciiString;
133141
} catch (IllegalArgumentException iae) {
134142
throw new SQLException("Failed to parse URL '" + url + "'", iae);
135143
}
@@ -171,6 +179,9 @@ private Map<String, String> parseUrl(String url) throws SQLException {
171179
throw new SQLException(
172180
"Invalid authority part JDBC URL '" + url + "'");
173181
}
182+
if (uri.getAuthority().contains(",")) {
183+
throw new SQLException("Multiple endpoints not supported");
184+
}
174185
properties.put(PARSE_URL_CONN_URL_PROP, uri.getScheme() + "://"
175186
+ uri.getRawAuthority()); // will be parsed again later
176187
if (uri.getPath() != null

‎jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import com.github.tomakehurst.wiremock.client.WireMock;
1414
import com.github.tomakehurst.wiremock.common.ConsoleNotifier;
1515
import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
16+
1617
import org.testng.Assert;
1718
import org.testng.annotations.DataProvider;
1819
import org.testng.annotations.Test;
@@ -34,7 +35,6 @@
3435
import static org.testng.Assert.assertThrows;
3536
import static org.testng.Assert.fail;
3637

37-
3838
public class ConnectionTest extends JdbcIntegrationTest {
3939

4040
@Test(groups = { "integration" }, enabled = false)
@@ -569,8 +569,32 @@ public void testConnectionWithValidDatabaseName(String dbName) throws Exception
569569
connCheck.close();
570570
}
571571

572+
@Test(groups = { "integration" })
573+
public void closedConnectionIsInvalid() throws Exception {
574+
if (isCloud()) {
575+
return;
576+
}
577+
Connection connection = this.getJdbcConnection();
578+
Assert.assertTrue(connection.isValid(3));
579+
connection.close();
580+
Assert.assertFalse(connection.isValid(3));
581+
}
582+
583+
@Test(groups = { "integration" })
584+
public void connectionWithWrongCredentialsIsInvalid() throws Exception {
585+
if (isCloud()) {
586+
return;
587+
}
588+
Connection connection = this.getJdbcConnection();
589+
Assert.assertTrue(connection.isValid(3));
590+
Properties properties = new Properties();
591+
properties.put("password", "invalid");
592+
connection = this.getJdbcConnection(properties);
593+
Assert.assertFalse(connection.isValid(3));
594+
}
595+
572596
@DataProvider(name = "validDatabaseNames")
573-
public Object[][] createValidDatabaseNames() {
597+
private static Object[][] createValidDatabaseNames() {
574598
return new Object[][] {
575599
{ "foo" },
576600
{ "with-dashes" },

‎jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcConfigurationTest.java‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package com.clickhouse.jdbc.internal;
22

3+
import com.clickhouse.client.api.Client;
34
import com.clickhouse.client.api.ClientConfigProperties;
5+
46
import org.testng.Assert;
57
import org.testng.annotations.DataProvider;
68
import org.testng.annotations.Test;
@@ -23,15 +25,17 @@ public class JdbcConfigurationTest {
2325
new JdbcConfigurationTestData("jdbc:clickhouse://localhost"),
2426
new JdbcConfigurationTestData("jdbc:clickhouse:http://localhost"),
2527
new JdbcConfigurationTestData("jdbc:clickhouse:https://localhost")
26-
.withExpectedConnectionURL("https://localhost"),
28+
.withExpectedConnectionURL("https://localhost:8443"),
29+
new JdbcConfigurationTestData("jdbc:clickhouse:https://localhost:8123")
30+
.withExpectedConnectionURL("https://localhost:8123"),
2731
new JdbcConfigurationTestData("jdbc:clickhouse://localhost")
2832
.withAdditionalConnectionParameters(
2933
Map.of(JdbcConfiguration.USE_SSL_PROP, "true"))
30-
.withExpectedConnectionURL("https://localhost")
34+
.withExpectedConnectionURL("https://localhost:8443")
3135
.withAdditionalExpectedClientProperties(
3236
Map.of("ssl", "true")),
3337
new JdbcConfigurationTestData("jdbc:clickhouse://[::1]")
34-
.withExpectedConnectionURL("http://[::1]"),
38+
.withExpectedConnectionURL("http://[::1]:8123"),
3539
new JdbcConfigurationTestData("jdbc:clickhouse://[::1]:8123")
3640
.withExpectedConnectionURL("http://[::1]:8123"),
3741
new JdbcConfigurationTestData("jdbc:clickhouse://localhost:8443")
@@ -111,14 +115,22 @@ public class JdbcConfigurationTest {
111115
))
112116
};
113117

118+
@SuppressWarnings("deprecation")
114119
@Test(dataProvider = "validURLTestData")
115120
public void testParseURLValid(String jdbcURL, Properties properties,
116-
String connectionUrl, Map<String, String> expectedClientProps)
121+
String connectionURL, Map<String, String> expectedClientProps)
117122
throws Exception
118123
{
119124
JdbcConfiguration configuration = new JdbcConfiguration(jdbcURL, properties);
120-
assertEquals(configuration.getConnectionUrl(), connectionUrl);
125+
assertEquals(configuration.getConnectionUrl(), connectionURL);
121126
assertEquals(configuration.clientProperties, expectedClientProps);
127+
Client.Builder bob = new Client.Builder();
128+
configuration.applyClientProperties(bob);
129+
Client client = bob.build();
130+
assertEquals(client.getEndpoints().size(), 1);
131+
assertEquals(
132+
client.getEndpoints().iterator().next(),
133+
connectionURL);
122134
}
123135

124136
@Test(dataProvider = "invalidURLs")
@@ -198,6 +210,9 @@ public Object[][] createInvalidConnectionURLs() {
198210
{ "jdbc:clickhouse://foo.bar?x&y=z" },
199211
{ "jdbc:clickhouse://foo.bar?x==&y=z" },
200212
{ "jdbc:clickhouse://localhost?☺=value1" },
213+
// multiple endpoints are invalid
214+
{ "jdbc:clickhouse://foo,bar" },
215+
{ "jdbc:clickhouse://foo,bar.com:8123" },
201216
};
202217
}
203218

@@ -210,7 +225,7 @@ private static final class JdbcConfigurationTestData {
210225
Map.of( "user", "default", "password", "");
211226

212227
private static final String DEFAULT_EXPECTED_CONNECTION_URL =
213-
"http://localhost";
228+
"http://localhost:8123";
214229

215230
private final String url;
216231
private final Properties connectionParameters;

‎pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@
8383
<brotli4j.version>1.12.0</brotli4j.version>
8484
<caffeine.version>3.1.7</caffeine.version>
8585
<compress.version>1.27.1</compress.version>
86-
<commons-lang3.version>3.16.0</commons-lang3.version>
86+
<commons-lang3.version>3.18.0</commons-lang3.version>
8787
<dnsjava.version>3.6.0</dnsjava.version>
8888
<fastutil.version>8.5.12</fastutil.version>
8989
<gson.version>2.10.1</gson.version>

0 commit comments

Comments
 (0)