ClickHouse / ClickHouse/clickhouse-java
[client-v2] serializeArrayData silently writes zero bytes for a wrong-typed Array/geo value and desynchronizes the RowBinary stream
- Dominant language
- Java
- Stars
- 1.6k
- Forks
- 636
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 28
Description
## Description
`SerializerUtils.serializeArrayData` writes **zero bytes** for a non-`null` value that is neither a Java array nor a `List`. The guard `if (value instanceof List || isArray)` is false and there is **no `else` branch**, so the method returns having emitted nothing at all for the column — not even the var-int length `0`.
In `RowBinary` this **desynchronizes the stream**: the bytes of the *following* column are consumed as this column's length prefix, so every subsequent column in the row is misframed. Depending on the format the user gets either a confusing server-side `CANNOT_READ_ALL_DATA` (Code 33) or — with `RowBinaryWithDefaults` — a **silently successful insert that stores phantom, corrupted rows**.
Six dispatch paths in `serializeData` reach `serializeArrayData` and are all affected:
* `case Array` → `serializeArrayData` directly;
* `case Ring` / `LineString` → unwrap `ClickHouseGeoRingValue`, then `serializeArrayData(…, GEO_RING_ARRAY)`;
* `case Polygon` / `MultiLineString` → `GEO_POLYGON_ARRAY`;
* `case MultiPolygon` → `GEO_MULTI_POLYGON_ARRAY`.
For the geo cases the unwrap is conditional (`value instanceof ClickHouseGeoXxxValue ? … : value`), so a wrong-typed value stays un-unwrapped and lands in the silent-skip.
This is a **misuse** scenario (passing a scalar to an array-shaped column), so it is not high-frequency. What makes it worth fixing is the *failure mode*: silent stream corruption instead of a clear client-side error — and it is inconsistent with the sibling serializers, which all fail fast (see “Contrast” below). Whether you'd prefer this as a misuse guard or consider the current behavior acceptable is a maintainer call; the reproduction below is offered so that call can be made on evidence.
### Steps to reproduce
1. `CREATE TABLE t (id UInt32, val Array(String), tail String) ENGINE MergeTree ORDER BY id`
2. Insert one row via `RowBinaryFormatWriter`, setting `val` to a `String` (`"not-an-array"`) instead of a `List`/array, and `tail` to `"TAILVALUE"`.
3. Observe: no client-side error; the server rejects the row with Code 33 (`RowBinary`), or the insert *succeeds* storing 4 corrupted rows (`RowBinaryWithDefaults`).
### Error Log or Exception StackTrace
Direct byte-level evidence — `serializeArrayData` / `serializeData` into a `ByteArrayOutputStream`:
```
Array(String) <- String : bytes written = 0 <-- defect
Ring <- String : bytes written = 0 <-- defect
LineString <- String : bytes written = 0 <-- defect
Polygon <- String : bytes written = 0 <-- defect
MultiLineString <- String : bytes written = 0 <-- defect
MultiPolygon <- String : bytes written = 0 <-- defect
Array(String) <- null : bytes written = 1 (var-int 0 — correct)
Array(String) <- List["a","b"] : bytes written = 5 (correct)
```
End-to-end through `RowBinaryFormatWriter` + `client.insert(...)` against a live server (26.5.1.882):
```
Array(String) <- String (RowBinary)
=> Code: 33. DB::Exception: Cannot read all data. Bytes read: 8. Bytes expected: 84:
(at row 1) : While executing BinaryRowInputFormat. (CANNOT_READ_ALL_DATA)
Array(UInt32) <- Integer (RowBinary)
=> Code: 33. DB::Exception: Cannot read all data. Bytes read: 1. Bytes expected: 4: (at row 1) ...
Ring <- String (RowBinary) => Code: 33 ... Bytes read: 1. Bytes expected: 8: (at row 1)
LineString <- String (RowBinary) => Code: 33 ... Bytes read: 1. Bytes expected: 8: (at row 1)
Polygon <- String (RowBinary) => Code: 33 ... Bytes read: 0. Bytes expected: 8: (at row 1)
MultiLineString <- String (RowBinary) => Code: 33 ... Bytes read: 0. Bytes expected: 8: (at row 1)
MultiPolygon <- String (RowBinary) => Code: 33 ... Bytes read: 7. Bytes expected: 8: (at row 1)
```
The worst variant — **the insert silently succeeds and stores garbage**:
```
Array(String) <- String (RowBinaryWithDefaults)
=> INSERT SUCCEEDED writtenRows=4
stored = [id=0, val=[], tail=] [id=0, val=[], tail=] [id=0, val=[], tail=] [id=1, val=[], tail=]
```
One committed row became four rows, the `id` and `tail` values were lost, and no exception was raised anywhere.
### Expected Behaviour
A non-`null` value that cannot represent an array should be rejected client-side with a clear `IllegalArgumentException` naming the column and the offending value type — matching what the sibling serializers already do today.
**Contrast (current, correct behaviors — verified in the same run, must stay unchanged):**
```
Tuple(UInt32, String) <- String => IllegalArgumentException: Cannot serialize not-a-tuple as a tuple
Geometry <- String => IllegalArgumentException: Cannot write value of class class java.lang.String
into column with geometry type Geometry
Map(String, String) <- String => ClassCastException: class java.lang.String cannot be cast to
class java.util.Map
QBit(...) <- String => IllegalArgumentException (guarded in #2939)
Array(String) <- null => single var-int 0
Array(String) <- List["a","b"] => round-trips: stored val=['a','b'], tail=TAILVALUE
```
So `Array` and the five array-backed geo types are the only container paths that fail *silently*.
### Code Example
```java
// CREATE TABLE t (id UInt32, val Array(String), tail String) ENGINE MergeTree ORDER BY id
TableSchema schema = client.getTableSchema("t");
ClickHouseFormat format = ClickHouseFormat.RowBinary; // or RowBinaryWithDefaults
client.insert("t", out -> {
RowBinaryFormatWriter w = new RowBinaryFormatWriter(out, schema, format);
w.setValue(schema.nameToColumnIndex("id"), 1);
w.setValue(schema.nameToColumnIndex("val"), "not-an-array"); // wrong type, silently skipped
w.setValue(schema.nameToColumnIndex("tail"), "TAILVALUE");
w.commitRow();
}, format, new InsertSettings()).get();
// RowBinary -> server Code 33 CANNOT_READ_ALL_DATA
// RowBinaryWithDefaults-> insert succeeds, 4 phantom rows stored
```
Byte-level minimal case, no server needed:
```java
ByteArrayOutputStream out = new ByteArrayOutputStream();
SerializerUtils.serializeArrayData(out, "not-an-array", ClickHouseColumn.of("arr", "Array(String)"));
assert out.size() == 0; // currently true — nothing is written for the column
```
## Root cause
`client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java`, `serializeArrayData` (line 516 on `main` @ `601ade16`):
```java
public static void serializeArrayData(OutputStream stream, Object value, ClickHouseColumn column) throws IOException {
if (value == null) {
writeVarInt(stream, 0);
return;
}
boolean isArray = value.getClass().isArray();
if (value instanceof List || isArray) {
...
}
// <-- no else: a non-null, non-array, non-List value falls off the end and writes 0 bytes
}
```
The dispatch sites are `serializeData` lines 70-71 (`Array`) and 105-118 (`Ring`/`LineString`/`Polygon`/`MultiLineString`/`MultiPolygon`).
## Suggested fix
In `serializeArrayData`, after the `null` check, reject a value that is neither a Java array nor a `List`:
```java
if (!(value instanceof List) && !value.getClass().isArray()) {
throw new IllegalArgumentException("Array column '" + column.getColumnName()
+ "' expects a Java array or List but got " + value.getClass().getName());
}
```
This mirrors the guards already present in `serializerGeometry`, `serializeTupleData`, and `serializeQBitData` (added in #2939 for exactly this failure class, scoped to the `QBit` path only). Because the throw sits in `serializeArrayData` itself, all six dispatch paths are covered at once. For the geo cases the message will name the synthetic `GEO_*_ARRAY` column, so it may be worth passing the real column name through or naming the geo type in the message.
Regression tests should assert that a wrong-typed value throws (and that zero bytes were written) for `Array` plus each of the five geo types, with contrast cases that a valid `List`/array still round-trips and that `null` still yields a single var-int `0`.
Suggested labels: `bug`, `client-api-v2`, `area:data-type`, `area:client-insert`.
## Configuration
#### Client Configuration
```java
new Client.Builder()
.addEndpoint(Protocol.HTTP, host, port, false)
.setUsername("default").setPassword(...)
.setDefaultDatabase(...)
.serverSetting(ServerSettings.ASYNC_INSERT, "0")
.serverSetting(ServerSettings.WAIT_END_OF_QUERY, "1")
.build();
```
#### Environment
* [ ] Cloud
* Client version: `main` @ `601ade16` (0.10.0-rc1-SNAPSHOT)
* Language version: OpenJDK 17
* OS: Linux (x86_64, container)
#### ClickHouse Server
* ClickHouse Server version: 26.5.1.882 (official build)
* ClickHouse Server non-default settings, if any: none relevant (`async_insert=0`, `wait_end_of_query=1` set per-query)
* `CREATE TABLE` statements for tables involved:
```sql
CREATE TABLE t (id UInt32, val Array(String), tail String) ENGINE MergeTree ORDER BY id;
-- and the same shape with val of type Ring / LineString / Polygon / MultiLineString / MultiPolygon
```
* Sample data: none needed — the defect is on the insert path.
## Relationship to other issues
Distinct from #2938 / PR #2940, which cover the **`null`-into-non-nullable-`Array`** double-`0x00` write in `RowBinaryFormatSerializer.writeValuePreamble`. That is a different site and a different root cause; this report is about a **non-`null` wrong-typed** value in `serializeArrayData` itself. The two are complementary and #2940 does not touch this path.
## Origin
Found by automated analysis of `client-v2` while working on the Cursor Bugbot review of PR #2939 (issue #2610) — the `QBit` guard added there fixed one dispatch path of this same mechanism, and this report covers the general case that was left out of that feature PR's scope. Verified by running against a live ClickHouse server (byte-level and end-to-end), not by inspection alone.
Contributor guide
Assessment
This issue has not been assessed yet.