From 577e765395222e69b6c544274315aa3ae95c28cd Mon Sep 17 00:00:00 2001 From: Polyglot AI <293096396+polyglotAI-bot@users.noreply.github.com> Date: Mon, 20 Jul 2026 09:07:53 +0000 Subject: [PATCH] Fix client-v2: reject null into non-nullable Array in RowBinary writer Writing a Java null into a non-nullable Array(...) column via RowBinaryFormatWriter emitted two bytes (00 00) instead of one: writeValuePreamble special-cased Array and wrote a stray writeNonNull marker (0x00) on top of the array length that serializeArrayData(null) already writes (var-uint 0 = 0x00). The server read the extra byte as a phantom extra row (single-column inserts) or a column shift that failed the whole insert with CANNOT_READ_ALL_DATA (multi-column inserts). A non-nullable Array cannot represent a null, so it now throws IllegalArgumentException naming the column - like every other non-nullable type and the merged Enum fix (#2932) - in both the RowBinary and RowBinaryWithDefaults branches. Empty arrays still serialize as a single length byte, columns with a DDL default still use the default under RowBinaryWithDefaults, and Dynamic columns (which can hold a null as the implicit Nothing type) are left unchanged. Fixes: https://github.com/ClickHouse/clickhouse-java/issues/2938 --- CHANGELOG.md | 9 ++ .../RowBinaryFormatSerializer.java | 11 +- .../datatypes/RowBinaryFormatWriterTest.java | 117 ++++++++++++++++++ .../client/internal/SerializerUtilsTests.java | 14 +++ 4 files changed, 144 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4f9aabad..0c6c8b10c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,15 @@ NPE instead of a clear error. It now throws `IllegalArgumentException` naming the column, consistent with the existing `IllegalArgumentException` for other unsupported enum values. Nullable enum columns are unaffected. (https://github.com/ClickHouse/clickhouse-java/issues/2931) +- **[client-v2]** Fixed silent data corruption when serializing a Java `null` into a non-nullable + `Array(...)` column via `RowBinaryFormatWriter`. `RowBinaryFormatSerializer.writeValuePreamble` + special-cased `Array`, emitting a stray marker byte on top of the array length; the server read the + extra byte as a phantom extra row (single-column inserts) or as a column shift that failed the insert + with `CANNOT_READ_ALL_DATA` (multi-column inserts). A non-nullable `Array` cannot represent a `null`, + so it now throws `IllegalArgumentException` naming the column — consistent with every other non-nullable + type — in both the `RowBinary` and `RowBinaryWithDefaults` paths. Empty arrays (`[]`) still serialize + correctly, and `Dynamic` columns, which can hold a `null` as the implicit `Nothing` type, are + unaffected. (https://github.com/ClickHouse/clickhouse-java/issues/2938) - **[client-v2]** Fixed POJO insert error classification so transport write failures such as java.net.SocketException: Broken pipe (Write failed) are now surfaced as transfer/network errors instead of being wrapped as DataSerializationException. This only changes the exception type reported for request-body transport failures during diff --git a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/RowBinaryFormatSerializer.java b/client-v2/src/main/java/com/clickhouse/client/api/data_formats/RowBinaryFormatSerializer.java index ad8ee680a..edc602bb4 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/RowBinaryFormatSerializer.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/data_formats/RowBinaryFormatSerializer.java @@ -204,10 +204,9 @@ public static boolean writeValuePreamble(OutputStream out, boolean defaultsSuppo SerializerUtils.writeNonNull(out); SerializerUtils.writeNull(out);//Then we send null, write 1 return false;//And we're done - } else if (dataType == ClickHouseDataType.Array) {//If the column is an array - SerializerUtils.writeNonNull(out);//Then we send nonNull } else if (dataType == ClickHouseDataType.Dynamic) { - SerializerUtils.writeNonNull(out); + // A Dynamic column can hold a null: it is serialized below as the implicit `Nothing` type. + SerializerUtils.writeNonNull(out);//Write 0 for no default } else { throw new IllegalArgumentException(String.format("An attempt to write null into not nullable column '%s'", column)); } @@ -220,10 +219,8 @@ public static boolean writeValuePreamble(OutputStream out, boolean defaultsSuppo } SerializerUtils.writeNonNull(out); } else if (value == null) { - if (dataType == ClickHouseDataType.Array) { - SerializerUtils.writeNonNull(out); - } else if (dataType == ClickHouseDataType.Dynamic) { - // do nothing + if (dataType == ClickHouseDataType.Dynamic) { + // A Dynamic column can hold a null: it is serialized below as the implicit `Nothing` type. } else { throw new IllegalArgumentException(String.format("An attempt to write null into not nullable column '%s'", column)); } diff --git a/client-v2/src/test/java/com/clickhouse/client/datatypes/RowBinaryFormatWriterTest.java b/client-v2/src/test/java/com/clickhouse/client/datatypes/RowBinaryFormatWriterTest.java index 333feaa5f..a5776eb0c 100644 --- a/client-v2/src/test/java/com/clickhouse/client/datatypes/RowBinaryFormatWriterTest.java +++ b/client-v2/src/test/java/com/clickhouse/client/datatypes/RowBinaryFormatWriterTest.java @@ -335,6 +335,123 @@ private static Map singleEntryMap(String key, Object value) { return map; } + @DataProvider(name = "rowBinaryWriterFormats") + private Object[][] rowBinaryWriterFormats() { + return new Object[][] { + {ClickHouseFormat.RowBinary}, + {ClickHouseFormat.RowBinaryWithDefaults}, + }; + } + + // A non-nullable Array column cannot represent a null, so writing a Java null into it must fail + // loudly like every other non-nullable type instead of emitting a stray marker byte on top of the + // array length. That stray byte was read by the server as a phantom extra row (single column) or + // shifted every following column (multi column). 'tail' sits after the array so a stray byte corrupts it. + @Test (groups = { "integration" }, dataProvider = "rowBinaryWriterFormats") + public void writeNullIntoNonNullableArrayThrowsTest(ClickHouseFormat format) throws Exception { + String tableName = "rowBinaryFormatWriterTest_nonNullableArrayNull_" + UUID.randomUUID().toString().replace('-', '_'); + initTable(tableName, + "CREATE TABLE \"" + tableName + "\" (id Int32, arr Array(Int32), tail Int32) Engine = MergeTree ORDER BY id", + new CommandSettings()); + TableSchema schema = client.getTableSchema(tableName); + + Exception thrown = null; + try (InsertResponse response = client.insert(tableName, out -> { + RowBinaryFormatWriter w = new RowBinaryFormatWriter(out, schema, format); + w.setValue(schema.nameToColumnIndex("id"), 1); + w.setValue(schema.nameToColumnIndex("arr"), null); + w.setValue(schema.nameToColumnIndex("tail"), 7); + w.commitRow(); + }, format, settings).get(EXECUTE_CMD_TIMEOUT, TimeUnit.SECONDS)) { + // The insert must not succeed: a null in a non-nullable Array column is invalid. + } catch (Exception e) { + thrown = e; + } + + Assert.assertNotNull(thrown, "Expected the insert to fail for a null in a non-nullable Array column using " + format); + boolean clearMessage = false; + for (Throwable t = thrown; t != null; t = t.getCause()) { + if (t.getMessage() != null && t.getMessage().contains("An attempt to write null into not nullable column")) { + clearMessage = true; + break; + } + } + Assert.assertTrue(clearMessage, "Expected a clear non-nullable column error using " + format + ", but got: " + thrown); + } + + // Contrast: an empty (and a populated) non-nullable Array must still round-trip, and the fixed-width + // 'tail' column after it keeps its value - proving the array length is still written as a single + // leading byte and that rejecting null did not perturb valid array writes. + @Test (groups = { "integration" }, dataProvider = "rowBinaryWriterFormats") + public void writeNonNullableArrayRoundTripsTest(ClickHouseFormat format) throws Exception { + String tableName = "rowBinaryFormatWriterTest_nonNullableArrayRoundTrip_" + UUID.randomUUID().toString().replace('-', '_'); + initTable(tableName, + "CREATE TABLE \"" + tableName + "\" (id Int32, arr Array(Int32), tail Int32) Engine = MergeTree ORDER BY id", + new CommandSettings()); + TableSchema schema = client.getTableSchema(tableName); + + try (InsertResponse response = client.insert(tableName, out -> { + RowBinaryFormatWriter w = new RowBinaryFormatWriter(out, schema, format); + w.setValue(schema.nameToColumnIndex("id"), 1); + w.setValue(schema.nameToColumnIndex("arr"), new ArrayList()); + w.setValue(schema.nameToColumnIndex("tail"), 7); + w.commitRow(); + w.setValue(schema.nameToColumnIndex("id"), 2); + w.setValue(schema.nameToColumnIndex("arr"), Arrays.asList(1, 2, 3)); + w.setValue(schema.nameToColumnIndex("tail"), 8); + w.commitRow(); + }, format, settings).get(EXECUTE_CMD_TIMEOUT, TimeUnit.SECONDS)) { + // inserted + } + + List records = client.queryAll( + "SELECT id, toInt32(length(arr)) AS alen, arrayStringConcat(arr, ',') AS acat, tail FROM \"" + + tableName + "\" ORDER BY id"); + Assert.assertEquals(records.size(), 2); + + GenericRecord row1 = records.get(0); + Assert.assertEquals(row1.getInteger("alen"), 0); + Assert.assertEquals(row1.getString("acat"), ""); + Assert.assertEquals(row1.getInteger("tail"), 7); + + GenericRecord row2 = records.get(1); + Assert.assertEquals(row2.getInteger("alen"), 3); + Assert.assertEquals(row2.getString("acat"), "1,2,3"); + Assert.assertEquals(row2.getInteger("tail"), 8); + } + + // Contrast: with RowBinaryWithDefaults, a null into a non-nullable Array column that HAS a DDL + // default must still fall back to that default (the null-to-default coercion is decided before the + // type dispatch), not throw - proving the fix only rejects null for non-nullable arrays with no default. + @Test (groups = { "integration" }) + public void writeNullIntoDefaultedArrayUsesDefaultTest() throws Exception { + String tableName = "rowBinaryFormatWriterTest_defaultedArrayNull_" + UUID.randomUUID().toString().replace('-', '_'); + initTable(tableName, + "CREATE TABLE \"" + tableName + "\" (id Int32, arr Array(Int32) DEFAULT [1, 2], tail Int32) Engine = MergeTree ORDER BY id", + new CommandSettings()); + TableSchema schema = client.getTableSchema(tableName); + ClickHouseFormat format = ClickHouseFormat.RowBinaryWithDefaults; + + try (InsertResponse response = client.insert(tableName, out -> { + RowBinaryFormatWriter w = new RowBinaryFormatWriter(out, schema, format); + w.setValue(schema.nameToColumnIndex("id"), 1); + w.setValue(schema.nameToColumnIndex("arr"), null); + w.setValue(schema.nameToColumnIndex("tail"), 7); + w.commitRow(); + }, format, settings).get(EXECUTE_CMD_TIMEOUT, TimeUnit.SECONDS)) { + // inserted; the null arr must fall back to the DDL default + } + + List records = client.queryAll( + "SELECT toInt32(length(arr)) AS alen, arrayStringConcat(arr, ',') AS acat, tail FROM \"" + + tableName + "\" ORDER BY id"); + Assert.assertEquals(records.size(), 1); + GenericRecord row = records.get(0); + Assert.assertEquals(row.getInteger("alen"), 2); + Assert.assertEquals(row.getString("acat"), "1,2"); + Assert.assertEquals(row.getInteger("tail"), 7); + } + @Test (groups = { "integration" }) public void writeNumbersTest() throws Exception { diff --git a/client-v2/src/test/java/com/clickhouse/client/internal/SerializerUtilsTests.java b/client-v2/src/test/java/com/clickhouse/client/internal/SerializerUtilsTests.java index 8c76828ef..5d2baaefa 100644 --- a/client-v2/src/test/java/com/clickhouse/client/internal/SerializerUtilsTests.java +++ b/client-v2/src/test/java/com/clickhouse/client/internal/SerializerUtilsTests.java @@ -38,6 +38,20 @@ public void testDynamicNullWithDefaultsWritesPreambleAndNothingTag() throws Exce Assert.assertEquals(out.toByteArray(), new byte[]{0, 0}); } + @Test + public void testDynamicNullWithoutDefaultsWritesNothingTag() throws Exception { + // A non-nullable Dynamic column can still hold a null (serialized as the implicit `Nothing` + // type), unlike a plain non-nullable Array which must reject null. This pins that behavior in + // the plain RowBinary path: no null-marker, just the single Nothing type tag. + ClickHouseColumn column = ClickHouseColumn.of("dyn", "Dynamic"); + ByteArrayOutputStream out = new ByteArrayOutputStream(); + + Assert.assertTrue(RowBinaryFormatSerializer.writeValuePreamble(out, false, column, null)); + SerializerUtils.serializeData(out, null, column); + + Assert.assertEquals(out.toByteArray(), new byte[]{0}); + } + @Test public void testSerializeNumbersRoundTrip() throws IOException { String[] names = new String[]{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o",