From cdf11ce600293a235cdd80ee6c0d406eabf12ab0 Mon Sep 17 00:00:00 2001 From: Sergey Chernov Date: Wed, 23 Oct 2024 14:17:56 -0700 Subject: [PATCH 1/3] fixed handling error when successful http status by error code header present --- .../client/http/ApacheHttpConnectionImpl.java | 4 +- .../com/clickhouse/client/api/Client.java | 14 ++++++- .../api/internal/HttpAPIClientHelper.java | 3 +- .../clickhouse/client/HttpTransportTests.java | 38 +++++++++++++++++++ 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/clickhouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java b/clickhouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java index 587e9699e..3e0d80fc3 100644 --- a/clickhouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java +++ b/clickhouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java @@ -194,11 +194,11 @@ private void setHeaders(HttpRequest request, Map headers) { } private void checkResponse(ClickHouseConfig config, CloseableHttpResponse response) throws IOException { - if (response.getCode() == HttpURLConnection.HTTP_OK) { + final Header errorCode = response.getFirstHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE); + if (response.getCode() == HttpURLConnection.HTTP_OK && errorCode == null) { return; } - final Header errorCode = response.getFirstHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE); final Header serverName = response.getFirstHeader(ClickHouseHttpProto.HEADER_SRV_DISPLAY_NAME); if (response.getEntity() == null) { throw new ConnectException( diff --git a/client-v2/src/main/java/com/clickhouse/client/api/Client.java b/client-v2/src/main/java/com/clickhouse/client/api/Client.java index f68ae03df..d95582439 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/Client.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/Client.java @@ -1,6 +1,7 @@ package com.clickhouse.client.api; import com.clickhouse.client.ClickHouseClient; +import com.clickhouse.client.ClickHouseException; import com.clickhouse.client.ClickHouseNode; import com.clickhouse.client.ClickHouseRequest; import com.clickhouse.client.ClickHouseResponse; @@ -80,6 +81,7 @@ import java.util.TimeZone; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; @@ -1455,6 +1457,11 @@ public CompletableFuture insert(String tableName, return response; } catch (ExecutionException e) { throw new ClientException("Failed to get insert response", e.getCause()); + } catch (CompletionException e) { + if (e.getCause() instanceof ClickHouseException) { + throw new ServerException(((ClickHouseException)e.getCause()).getErrorCode(), e.getCause().getMessage().trim()); + } + throw new ClientException("Failed to get query response", e.getCause()); } catch (InterruptedException | TimeoutException e) { throw new ClientException("Operation has likely timed out.", e); } @@ -1575,7 +1582,7 @@ public CompletableFuture query(String sqlQuery, Map query(String sqlQuery, Map= HttpStatus.SC_BAD_REQUEST) { + } else if (httpResponse.getCode() >= HttpStatus.SC_BAD_REQUEST || httpResponse.containsHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE)) { try { throw readError(httpResponse); } finally { diff --git a/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java b/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java index a2d4e581b..70196161d 100644 --- a/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java +++ b/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java @@ -25,6 +25,7 @@ import org.apache.hc.core5.http.ConnectionRequestTimeoutException; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.net.URIBuilder; +import org.eclipse.jetty.server.Server; import org.testcontainers.utility.ThrowingFunction; import org.testng.Assert; import org.testng.annotations.DataProvider; @@ -338,6 +339,43 @@ public static Object[] testServerErrorHandlingDataProvider() { return new Object[] { ClickHouseFormat.JSON, ClickHouseFormat.TabSeparated, ClickHouseFormat.RowBinary }; } + + @Test(groups = { "integration" }) + public void testErrorWithSuccessfulResponse() { + WireMockServer mockServer = new WireMockServer( WireMockConfiguration + .options().port(9090).notifier(new ConsoleNotifier(false))); + mockServer.start(); + + try (Client client = new Client.Builder().addEndpoint(Protocol.HTTP, "localhost", mockServer.port(), false) + .setUsername("default") + .setPassword("") + .compressServerResponse(false) + .useNewImplementation(true) + .build()) { + mockServer.addStubMapping(WireMock.post(WireMock.anyUrl()) + .willReturn(WireMock.aResponse() + .withStatus(HttpStatus.SC_OK) + .withChunkedDribbleDelay(2, 200) + .withHeader("X-ClickHouse-Exception-Code", "241") + .withHeader("X-ClickHouse-Summary", + "{ \"read_bytes\": \"10\", \"read_rows\": \"1\"}") + .withBody("Code: 241. DB::Exception: Memory limit (for query) exceeded: would use 97.21 MiB")) + .build()); + + try (QueryResponse response = client.query("SELECT 1").get(1, TimeUnit.SECONDS)) { + Assert.fail("Expected exception"); + } catch (ServerException e) { + e.printStackTrace(); + Assert.assertEquals(e.getMessage(), "Code: 241. DB::Exception: Memory limit (for query) exceeded: would use 97.21 MiB"); + } catch (Exception e) { + e.printStackTrace(); + Assert.fail("Unexpected exception", e); + } + } finally { + mockServer.stop(); + } + } + @Test(groups = { "integration" }) public void testAdditionalHeaders() { WireMockServer mockServer = new WireMockServer( WireMockConfiguration From be0e9f63190b593b5fa216c4c5596930de60e382 Mon Sep 17 00:00:00 2001 From: Sergey Chernov Date: Wed, 23 Oct 2024 15:58:03 -0700 Subject: [PATCH 2/3] fixed tests after changing exceptions --- .../src/main/java/com/clickhouse/client/api/Client.java | 2 ++ .../java/com/clickhouse/client/HttpTransportTests.java | 9 ++++----- .../java/com/clickhouse/client/command/CommandTests.java | 5 ++++- .../java/com/clickhouse/client/query/QueryTests.java | 4 +++- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/client-v2/src/main/java/com/clickhouse/client/api/Client.java b/client-v2/src/main/java/com/clickhouse/client/api/Client.java index d95582439..77333e5aa 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/Client.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/Client.java @@ -1807,6 +1807,8 @@ private TableSchema getTableSchemaImpl(String describeQuery, String name, String throw new ClientException("Operation has likely timed out after " + getOperationTimeout() + " seconds.", e); } catch (ExecutionException e) { throw new ClientException("Failed to get table schema", e.getCause()); + } catch (ServerException e) { + throw e; } catch (Exception e) { throw new ClientException("Failed to get table schema", e); } diff --git a/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java b/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java index 70196161d..b81183cca 100644 --- a/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java +++ b/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java @@ -321,12 +321,11 @@ public void testServerErrorHandling(ClickHouseFormat format) { try (QueryResponse response = client.query("SELECT invalid;statement", querySettings).get(1, TimeUnit.SECONDS)) { Assert.fail("Expected exception"); - } catch (ClientException e) { + } catch (ServerException e) { e.printStackTrace(); - ServerException serverException = (ServerException) e.getCause(); - Assert.assertEquals(serverException.getCode(), 62); - Assert.assertTrue(serverException.getMessage().startsWith("Code: 62. DB::Exception: Syntax error (Multi-statements are not allowed): failed at position 15 (end of query)"), - "Unexpected error message: " + serverException.getMessage()); + Assert.assertEquals(e.getCode(), 62); + Assert.assertTrue(e.getMessage().startsWith("Code: 62. DB::Exception: Syntax error (Multi-statements are not allowed): failed at position 15 (end of query)"), + "Unexpected error message: " + e.getMessage()); } } catch (Exception e) { e.printStackTrace(); diff --git a/client-v2/src/test/java/com/clickhouse/client/command/CommandTests.java b/client-v2/src/test/java/com/clickhouse/client/command/CommandTests.java index 16c33aea1..06d56a8e1 100644 --- a/client-v2/src/test/java/com/clickhouse/client/command/CommandTests.java +++ b/client-v2/src/test/java/com/clickhouse/client/command/CommandTests.java @@ -5,6 +5,7 @@ import com.clickhouse.client.ClickHouseProtocol; import com.clickhouse.client.api.Client; import com.clickhouse.client.api.ClientException; +import com.clickhouse.client.api.ServerException; import com.clickhouse.client.api.command.CommandResponse; import com.clickhouse.client.api.enums.Protocol; import org.testng.Assert; @@ -45,8 +46,10 @@ public void testCreateTable() throws Exception { public void testInvalidCommandExecution() throws Exception { try { client.execute("ALTER TABLE non_existing_table ADD COLUMN id2 UInt32").get(10, TimeUnit.SECONDS); + } catch (ServerException e) { + Assert.assertEquals(e.getCode(), 60); } catch (ExecutionException e) { - Assert.assertTrue(e.getCause() instanceof ClientException); + Assert.assertTrue(e.getCause() instanceof ServerException); } catch (ClientException e) { // expected } diff --git a/client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java b/client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java index e5711212e..f149ddab2 100644 --- a/client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java +++ b/client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java @@ -550,8 +550,10 @@ public void testQueryExceptionHandling() throws Exception { try { client.queryRecords("SELECT * FROM unknown_table").get(3, TimeUnit.SECONDS); Assert.fail("expected exception"); + } catch (ServerException e) { + Assert.assertTrue(e.getMessage().contains("Unknown table")); } catch (ExecutionException e) { - Assert.assertTrue(e.getCause() instanceof ClientException); + Assert.assertTrue(e.getCause() instanceof ServerException); } catch (ClientException e) { // expected } From 30ad400fb2d754708026fde8ac22f4ec5ddd9f9d Mon Sep 17 00:00:00 2001 From: Sergey Chernov Date: Wed, 23 Oct 2024 22:05:52 -0700 Subject: [PATCH 3/3] removed unused code --- .../com/clickhouse/client/api/internal/HttpAPIClientHelper.java | 1 - 1 file changed, 1 deletion(-) diff --git a/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java b/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java index 7fa017df7..808f47909 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java @@ -348,7 +348,6 @@ public ClassicHttpResponse executeRequest(ClickHouseNode server, Map