Skip to content

Commit a0ab761

Browse files
chernserchernser
authored
Merge pull request #1879 from ClickHouse/fix_error_handling
[client-v2] Fixes handling error when successful http status by error code header in response
2 parents a6ff3e3 + 30ad400 commit a0ab761

File tree

6 files changed

+67
-11
lines changed

6 files changed

+67
-11
lines changed

‎clickhouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,11 +194,11 @@ private void setHeaders(HttpRequest request, Map<String, String> headers) {
194194
}
195195

196196
private void checkResponse(ClickHouseConfig config, CloseableHttpResponse response) throws IOException {
197-
if (response.getCode() == HttpURLConnection.HTTP_OK) {
197+
final Header errorCode = response.getFirstHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE);
198+
if (response.getCode() == HttpURLConnection.HTTP_OK && errorCode == null) {
198199
return;
199200
}
200201

201-
final Header errorCode = response.getFirstHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE);
202202
final Header serverName = response.getFirstHeader(ClickHouseHttpProto.HEADER_SRV_DISPLAY_NAME);
203203
if (response.getEntity() == null) {
204204
throw new ConnectException(

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.clickhouse.client.api;
22

33
import com.clickhouse.client.ClickHouseClient;
4+
import com.clickhouse.client.ClickHouseException;
45
import com.clickhouse.client.ClickHouseNode;
56
import com.clickhouse.client.ClickHouseRequest;
67
import com.clickhouse.client.ClickHouseResponse;
@@ -80,6 +81,7 @@
8081
import java.util.TimeZone;
8182
import java.util.UUID;
8283
import java.util.concurrent.CompletableFuture;
84+
import java.util.concurrent.CompletionException;
8385
import java.util.concurrent.ConcurrentHashMap;
8486
import java.util.concurrent.ExecutionException;
8587
import java.util.concurrent.ExecutorService;
@@ -1455,6 +1457,11 @@ public CompletableFuture<InsertResponse> insert(String tableName,
14551457
return response;
14561458
} catch (ExecutionException e) {
14571459
throw new ClientException("Failed to get insert response", e.getCause());
1460+
} catch (CompletionException e) {
1461+
if (e.getCause() instanceof ClickHouseException) {
1462+
throw new ServerException(((ClickHouseException)e.getCause()).getErrorCode(), e.getCause().getMessage().trim());
1463+
}
1464+
throw new ClientException("Failed to get query response", e.getCause());
14581465
} catch (InterruptedException | TimeoutException e) {
14591466
throw new ClientException("Operation has likely timed out.", e);
14601467
}
@@ -1575,7 +1582,7 @@ public CompletableFuture<QueryResponse> query(String sqlQuery, Map<String, Objec
15751582
} else {
15761583
throw lastException;
15771584
}
1578-
} catch (ClientException e) {
1585+
} catch (ClientException | ServerException e) {
15791586
throw e;
15801587
} catch (Exception e) {
15811588
throw new ClientException("Query request failed", e);
@@ -1609,6 +1616,11 @@ public CompletableFuture<QueryResponse> query(String sqlQuery, Map<String, Objec
16091616
return new QueryResponse(clickHouseResponse, format, clientStats, finalSettings);
16101617
} catch (ClientException e) {
16111618
throw e;
1619+
} catch (CompletionException e) {
1620+
if (e.getCause() instanceof ClickHouseException) {
1621+
throw new ServerException(((ClickHouseException)e.getCause()).getErrorCode(), e.getCause().getMessage().trim());
1622+
}
1623+
throw new ClientException("Failed to get query response", e.getCause());
16121624
} catch (Exception e) {
16131625
throw new ClientException("Failed to get query response", e);
16141626
}
@@ -1795,6 +1807,8 @@ private TableSchema getTableSchemaImpl(String describeQuery, String name, String
17951807
throw new ClientException("Operation has likely timed out after " + getOperationTimeout() + " seconds.", e);
17961808
} catch (ExecutionException e) {
17971809
throw new ClientException("Failed to get table schema", e.getCause());
1810+
} catch (ServerException e) {
1811+
throw e;
17981812
} catch (Exception e) {
17991813
throw new ClientException("Failed to get table schema", e);
18001814
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,7 @@ public ClassicHttpResponse executeRequest(ClickHouseNode server, Map<String, Obj
354354
} else if (httpResponse.getCode() == HttpStatus.SC_BAD_GATEWAY) {
355355
httpResponse.close();
356356
throw new ClientException("Server returned '502 Bad gateway'. Check network and proxy settings.");
357-
} else if (httpResponse.getCode() >= HttpStatus.SC_BAD_REQUEST) {
357+
} else if (httpResponse.getCode() >= HttpStatus.SC_BAD_REQUEST || httpResponse.containsHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE)) {
358358
try {
359359
throw readError(httpResponse);
360360
} finally {

‎client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java‎

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import org.apache.hc.core5.http.ConnectionRequestTimeoutException;
2626
import org.apache.hc.core5.http.HttpStatus;
2727
import org.apache.hc.core5.net.URIBuilder;
28+
import org.eclipse.jetty.server.Server;
2829
import org.testcontainers.utility.ThrowingFunction;
2930
import org.testng.Assert;
3031
import org.testng.annotations.DataProvider;
@@ -320,12 +321,11 @@ public void testServerErrorHandling(ClickHouseFormat format) {
320321
try (QueryResponse response =
321322
client.query("SELECT invalid;statement", querySettings).get(1, TimeUnit.SECONDS)) {
322323
Assert.fail("Expected exception");
323-
} catch (ClientException e) {
324+
} catch (ServerException e) {
324325
e.printStackTrace();
325-
ServerException serverException = (ServerException) e.getCause();
326-
Assert.assertEquals(serverException.getCode(), 62);
327-
Assert.assertTrue(serverException.getMessage().startsWith("Code: 62. DB::Exception: Syntax error (Multi-statements are not allowed): failed at position 15 (end of query)"),
328-
"Unexpected error message: " + serverException.getMessage());
326+
Assert.assertEquals(e.getCode(), 62);
327+
Assert.assertTrue(e.getMessage().startsWith("Code: 62. DB::Exception: Syntax error (Multi-statements are not allowed): failed at position 15 (end of query)"),
328+
"Unexpected error message: " + e.getMessage());
329329
}
330330
} catch (Exception e) {
331331
e.printStackTrace();
@@ -338,6 +338,43 @@ public static Object[] testServerErrorHandlingDataProvider() {
338338
return new Object[] { ClickHouseFormat.JSON, ClickHouseFormat.TabSeparated, ClickHouseFormat.RowBinary };
339339
}
340340

341+
342+
@Test(groups = { "integration" })
343+
public void testErrorWithSuccessfulResponse() {
344+
WireMockServer mockServer = new WireMockServer( WireMockConfiguration
345+
.options().port(9090).notifier(new ConsoleNotifier(false)));
346+
mockServer.start();
347+
348+
try (Client client = new Client.Builder().addEndpoint(Protocol.HTTP, "localhost", mockServer.port(), false)
349+
.setUsername("default")
350+
.setPassword("")
351+
.compressServerResponse(false)
352+
.useNewImplementation(true)
353+
.build()) {
354+
mockServer.addStubMapping(WireMock.post(WireMock.anyUrl())
355+
.willReturn(WireMock.aResponse()
356+
.withStatus(HttpStatus.SC_OK)
357+
.withChunkedDribbleDelay(2, 200)
358+
.withHeader("X-ClickHouse-Exception-Code", "241")
359+
.withHeader("X-ClickHouse-Summary",
360+
"{ \"read_bytes\": \"10\", \"read_rows\": \"1\"}")
361+
.withBody("Code: 241. DB::Exception: Memory limit (for query) exceeded: would use 97.21 MiB"))
362+
.build());
363+
364+
try (QueryResponse response = client.query("SELECT 1").get(1, TimeUnit.SECONDS)) {
365+
Assert.fail("Expected exception");
366+
} catch (ServerException e) {
367+
e.printStackTrace();
368+
Assert.assertEquals(e.getMessage(), "Code: 241. DB::Exception: Memory limit (for query) exceeded: would use 97.21 MiB");
369+
} catch (Exception e) {
370+
e.printStackTrace();
371+
Assert.fail("Unexpected exception", e);
372+
}
373+
} finally {
374+
mockServer.stop();
375+
}
376+
}
377+
341378
@Test(groups = { "integration" })
342379
public void testAdditionalHeaders() {
343380
WireMockServer mockServer = new WireMockServer( WireMockConfiguration

‎client-v2/src/test/java/com/clickhouse/client/command/CommandTests.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import com.clickhouse.client.ClickHouseProtocol;
66
import com.clickhouse.client.api.Client;
77
import com.clickhouse.client.api.ClientException;
8+
import com.clickhouse.client.api.ServerException;
89
import com.clickhouse.client.api.command.CommandResponse;
910
import com.clickhouse.client.api.enums.Protocol;
1011
import org.testng.Assert;
@@ -45,8 +46,10 @@ public void testCreateTable() throws Exception {
4546
public void testInvalidCommandExecution() throws Exception {
4647
try {
4748
client.execute("ALTER TABLE non_existing_table ADD COLUMN id2 UInt32").get(10, TimeUnit.SECONDS);
49+
} catch (ServerException e) {
50+
Assert.assertEquals(e.getCode(), 60);
4851
} catch (ExecutionException e) {
49-
Assert.assertTrue(e.getCause() instanceof ClientException);
52+
Assert.assertTrue(e.getCause() instanceof ServerException);
5053
} catch (ClientException e) {
5154
// expected
5255
}

‎client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -550,8 +550,10 @@ public void testQueryExceptionHandling() throws Exception {
550550
try {
551551
client.queryRecords("SELECT * FROM unknown_table").get(3, TimeUnit.SECONDS);
552552
Assert.fail("expected exception");
553+
} catch (ServerException e) {
554+
Assert.assertTrue(e.getMessage().contains("Unknown table"));
553555
} catch (ExecutionException e) {
554-
Assert.assertTrue(e.getCause() instanceof ClientException);
556+
Assert.assertTrue(e.getCause() instanceof ServerException);
555557
} catch (ClientException e) {
556558
// expected
557559
}

0 commit comments

Comments
 (0)