ClickHouse / ClickHouse/clickhouse-java

[clickhouse-http-client v1] HTTP transport ignores X-ClickHouse-Exception-Code on HTTP 200 (HttpURLConnection, Java11 HttpClient)

Đang mở
#2,854 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
area:network client-v1 module-http
Ngôn ngữ chính
Java
Star
1.6k
Fork
636
Merge trung bình
2 ngày 23 giờ
Pull request đã merge (30 ngày)
29

Mô tả

## Description

ClickHouse server can return `HTTP 200 OK` while reporting a query failure via the `X-ClickHouse-Exception-Code` response header (the error text being in the body). This happens when:

- The server already started streaming response headers before encountering an error mid-stream (chunked transfer; `__exception__` marker not always present).
- A distributed DDL via `ON CLUSTER` surfaces a per-shard error encoded inside a result block (see https://github.com/ClickHouse/clickhouse-go/issues/1398 — `Sorting key contains nullable columns` returned as `HTTP/1.1 200 OK` with the error inside the Native body and no `__exception__` marker).

In `clickhouse-http-client` (the legacy v1 HTTP client used by `jdbc-v1`), two of the three HTTP connection providers only consult the HTTP status code and **ignore** `X-ClickHouse-Exception-Code` when status is 200, even though the constant is defined in `ClickHouseHttpProto.HEADER_EXCEPTION_CODE`:

**`clickhouse-http-client/src/main/java/com/clickhouse/client/http/HttpUrlConnectionImpl.java`** (JDK `HttpURLConnection`), around line 176:

```java
private void checkResponse(HttpURLConnection conn) throws IOException {
log.debug("http response code [%d]", conn.getResponseCode());
if (conn.getResponseCode() != HttpURLConnection.HTTP_OK) {
String errorCode = conn.getHeaderField("X-ClickHouse-Exception-Code");
...
}
// status 200 → returns; X-ClickHouse-Exception-Code never inspected
}
```

**`clickhouse-http-client/src/main/java11/com/clickhouse/client/http/HttpClientConnectionImpl.java`** (Java 11 `HttpClient`), around line 140:

```java
private HttpResponse checkResponse(ClickHouseConfig config, HttpResponse r)
throws IOException {
if (r.statusCode() != HttpURLConnection.HTTP_OK) {
String errorCode = r.headers().firstValue("X-ClickHouse-Exception-Code").orElse("");
...
}
return r;
}
```

For comparison, the third legacy provider, `ApacheHttpConnectionImpl.checkResponse` (line ~217), is correct — it gates on both:

```java
final Header errorCode = response.getFirstHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE);
if (response.getCode() == HttpURLConnection.HTTP_OK && errorCode == null) {
return;
}
```

And `client-v2` (current HTTP client, `HttpAPIClientHelper.doPostRequest` line ~584) is also correct:

```java
} else if (httpResponse.getCode() >= HttpStatus.SC_BAD_REQUEST
|| httpResponse.containsHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE)) {
throw readError(req, httpResponse);
}
```

**Net effect (v1 path only):** a query (especially `Exec` / `INSERT` / `ON CLUSTER` DDL) the server marks as failed via the response header on a 200 response is observed as success when `jdbc-v1` is backed by `HttpUrlConnectionImpl` or `HttpClientConnectionImpl`. Risks: silent data loss, silent schema-change failure.

Note: `jdbc-v1`'s default HTTP connection provider is `ApacheHttpConnectionImpl` (which is correct), so users on defaults are unaffected. Users who select `HTTP_URL_CONNECTION` or `HTTP_CLIENT` as the HTTP connection provider hit the bug. This is also adjacent to but distinct from #650 (mid-result error rows / `__exception__` marker; `blocked-by-clickhouse`) — the fix here is purely client-side: inspect the header.

## ClickHouse server version

Code analysis only; not verified against a running server. Server version available in test environment: `26.4.2.10`. Behavior is consistent with prior server versions per the source bug.

## Reproduction

Conceptual reproduction with a stub HTTP server (Java 11+ pseudocode using `com.sun.net.httpserver`):

```java
@Test
public void httpUrlConnection_ignoresExceptionCodeHeaderOn200() throws Exception {
HttpServer stub = HttpServer.create(new InetSocketAddress(0), 0);
stub.createContext("/", exchange -> {
exchange.getResponseHeaders().add("X-ClickHouse-Exception-Code", "44");
exchange.getResponseHeaders().add("X-ClickHouse-Server-Display-Name", "test");
byte[] body = "Code: 44. DB::Exception: Sorting key contains nullable columns\n"
.getBytes(StandardCharsets.UTF_8);
exchange.sendResponseHeaders(200, body.length); // HTTP 200 with error header
try (var os = exchange.getResponseBody()) { os.write(body); }
});
stub.start();
int port = stub.getAddress().getPort();

ClickHouseNode server = ClickHouseNode.builder()
.host("127.0.0.1").port(ClickHouseProtocol.HTTP, port)
.addOption("http_connection_provider", "HTTP_URL_CONNECTION") // or HTTP_CLIENT
.build();

try (ClickHouseClient client = ClickHouseClient.newInstance(ClickHouseProtocol.HTTP);
ClickHouseResponse resp = client.read(server)
.query("CREATE TABLE t (x Nullable(String)) ENGINE=ReplicatedMergeTree ORDER BY x")
.executeAndWait()) {
fail("expected exception, got success: " + resp.getSummary());
} catch (ClickHouseException e) {
assertEquals(44, e.getErrorCode()); // expected
} finally {
stub.stop(0);
}
}
```

- **Expected**: `ClickHouseException` with code 44 (server's exception code from the header).
- **Actual** (with `HTTP_URL_CONNECTION` or `HTTP_CLIENT` provider): no exception thrown; the call returns success.

A user-visible variant of this is the closed `clickhouse-go#1398` and the source bug `clickhouse-rs#255`: distributed DDL with `ON CLUSTER` that the server reports failed via header on a 200 response.

## Suggested fix

In each of the two affected providers, gate the early-return on **both** status and header presence (matching what `ApacheHttpConnectionImpl` and `client-v2` already do):

- `clickhouse-http-client/src/main/java/com/clickhouse/client/http/HttpUrlConnectionImpl.java` (~line 178):
```java
String errorCode = conn.getHeaderField(ClickHouseHttpProto.HEADER_EXCEPTION_CODE);
if (conn.getResponseCode() == HttpURLConnection.HTTP_OK
&& (errorCode == null || errorCode.isEmpty())) {
return;
}
// fall through to existing error path; pass errorCode in
```
- `clickhouse-http-client/src/main/java11/com/clickhouse/client/http/HttpClientConnectionImpl.java` (~line 142):
```java
String errorCode = r.headers().firstValue(ClickHouseHttpProto.HEADER_EXCEPTION_CODE).orElse("");
if (r.statusCode() == HttpURLConnection.HTTP_OK && errorCode.isEmpty()) {
return r;
}
// fall through to existing error path
```

A small unit test against a stub HTTP server (as above) for each provider would lock the behavior down.

## Link

Source bug: https://github.com/ClickHouse/clickhouse-rs/issues/255
Related (user-reported manifestation in clickhouse-go, closed via server-side workaround): https://github.com/ClickHouse/clickhouse-go/issues/1398
Adjacent existing issue in this repo (different root cause — mid-result error rows): #650
Central tracking: ClickHouse/integrations-ai-playground#147

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Start with checkResponse in clickhouse-http-client/src/main/java/com/clickhouse/client/http/HttpUrlConnectionImpl.java and the Java 11 counterpart in src/main/java11/com/clickhouse/client/http/HttpClientConnectionImpl.java; compare them with ApacheHttpConnectionImpl and client-v2's HttpAPIClientHelper.doPostRequest. Add coverage using the described stub-server scenario for both providers, and confirm a 200 response carrying X-ClickHouse-Exception-Code produces ClickHouseException while a normal 200 response remains successful.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
java
Lĩnh vực
api, databases
Loại issue
Lỗi
Độ khó
3/5
Thời gian dự kiến
1-2 ngày
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
72/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.