Fix JSON overlapping paths: give way in the parent walk when the deeper path holds nothing

CHANGELOG.md

@@ -1,9 +1,6 @@
 Unreleased
 ---
 
-**Bug Fixes:**
-* **Breaking Change**: Fixed a `JSON` column with overlapping paths silently losing data. ClickHouse accepts a column which declares a path both as a value and as the parent of another path — for example `JSON(a Int64, a.b Int64)` — and sends both paths in every row, so the row renders as a document with a duplicate key (`{"a":0,"a":{"b":7}}`). A `JsonObject` cannot hold two values for one key, so the driver silently kept whichever path the server sent last: reading `{"a":{"b":7}}` back out of that column returned `{"a":0}` and the `7` was gone. The same loss hit a `Map` path overlapped by a deeper path (`JSON(a Map(String, Int64))` holding `{"a":{"b":1},"a.b":7}` returned `{"a":{"b":7}}`, dropping the map's own entry), except that there the two values were merged into one object rather than one replacing the other. A row where both sides of such an overlap hold a value now throws a `SerializationException` naming the two paths. With non-`Nullable` paths the driver cannot tell a stored `0` from an absent path, so no rule can recover the real value — the choice is between a wrong answer and a clear failure. A side which holds nothing is not a collision: a null, an empty object, and an all-null subtree all give way to the side which has the data, so `Nullable` overlaps continue to read as before. Use `JsonReadMode.String` to read such a column as the server's JSON text, duplicate key included. To restore the previous behavior and keep reading the column as a `JsonObject`, set the new `AllowDuplicateJsonKeys` setting (connection string key `AllowDuplicateJsonKeys=true`, or `ClickHouseClientSettings.AllowDuplicateJsonKeys`); the driver then keeps whichever value the row carries last, with the data loss described above. An overlap whose parent holds a scalar or an array still throws — that shape raised `InvalidCastException` before and is now a `SerializationException`.
-
 v1.4.0
 ---
 
@@ -61,6 +58,7 @@ v1.4.0
 * `ClickHouseClient.MemoryStreamManager` is now `[Obsolete]`. Since binary inserts stream directly into the request body (see above), this property is no longer used and has no effect; it will be removed in a future version.
 
 **Bug Fixes:**
+* **Breaking Change**: Fixed a `JSON` column with overlapping paths silently losing data. ClickHouse accepts a column which declares a path both as a value and as the parent of another path — for example `JSON(a Int64, a.b Int64)` — and sends both paths in every row, so the row renders as a document with a duplicate key (`{"a":0,"a":{"b":7}}`). A `JsonObject` cannot hold two values for one key, so the driver silently kept whichever path the server sent last: reading `{"a":{"b":7}}` back out of that column returned `{"a":0}` and the `7` was gone. The same loss hit a `Map` path overlapped by a deeper path (`JSON(a Map(String, Int64))` holding `{"a":{"b":1},"a.b":7}` returned `{"a":{"b":7}}`, dropping the map's own entry), except that there the two values were merged into one object rather than one replacing the other. A row where both sides of such an overlap hold a value now throws a `SerializationException` naming the two paths. With non-`Nullable` paths the driver cannot tell a stored `0` from an absent path, so no rule can recover the real value — the choice is between a wrong answer and a clear failure. A side which holds nothing is not a collision: a null, an empty object, and an all-null subtree all give way to the side which has the data, whichever of the two paths the server sends first, so `Nullable` overlaps continue to read as before. Use `JsonReadMode.String` to read such a column as the server's JSON text, duplicate key included. To restore the previous behavior and keep reading the column as a `JsonObject`, set the new `AllowDuplicateJsonKeys` setting (connection string key `AllowDuplicateJsonKeys=true`, or `ClickHouseClientSettings.AllowDuplicateJsonKeys`); the driver then keeps whichever value the row carries last, with the data loss described above. A path which holds a value and whose parent holds a scalar or an array still throws — that shape raised `InvalidCastException` before and is now a `SerializationException`. (#583)
 * Fixed uncompressed binary inserts (`InsertOptions.Compressor = null`) inflating the HTTP request body. The request body is sent chunked, and the row serializer wrote straight to it, so every field write became its own HTTP chunk.
 * Fixed binary inserts flushing the compression stream once per value written by copying a stream — binary-mode `JSON` POCOs, and `String`/`FixedString` values given as a `Stream`.
 * Fixed `InsertOptions.WithColumnTypes()` and `InsertOptions.WithQueryId()` silently dropping some caller-set options (such as `AcceptEncoding`) when copying.

ClickHouse.Driver.Tests/Types/JsonOverlappingPathReadTests.cs

@@ -0,0 +1,130 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Runtime.Serialization;
+using System.Text.Json.Nodes;
+using ClickHouse.Driver.Formats;
+using ClickHouse.Driver.Types;
+using NUnit.Framework;
+
+namespace ClickHouse.Driver.Tests.Types;
+
+/// <summary>
+/// Pins the rule for a JSON column which declares a path both as a value and as the parent of
+/// another path: a row where both sides hold a value throws, and a row where one side holds
+/// nothing reads the other. The rule is meant to be order-independent, so both wire orders are
+/// covered.
+/// </summary>
+/// <remarks>
+/// The row is decoded from bytes rather than read back from a server because a server sends the
+/// deeper of two typed paths first, so no insert produces a row where the shallower one comes
+/// first and the deeper one holds nothing. JsonTypeTests covers the orders an insert does produce.
+/// </remarks>
+[TestFixture]
+public class JsonOverlappingPathReadTests
+{
+    private static readonly Dictionary<string, long> EmptyMap = new();
+    private static readonly Dictionary<string, long?> AllNullMap = new() { ["x"] = null };
+    private static readonly Dictionary<string, long> MapValue = new() { ["x"] = 1 };
+
+    public static IEnumerable<TestCaseData> Cases()
+    {
+        // The parent is read first and holds a value, and the deeper path holds nothing: it gives
+        // way, and the parent's value survives. Each case is a different shape of "nothing".
+        yield return Case("ParentValueBeforeNullPath",
+            "JSON(a Int64, `a.b` Nullable(Int64))", [("a", 5L), ("a.b", null)], "{\"a\":5}");
+
+        yield return Case("ParentValueBeforeEmptyObjectPath",
+            "JSON(a Int64, `a.b` Map(String, Int64))", [("a", 5L), ("a.b", EmptyMap)], "{\"a\":5}");
+
+        yield return Case("ParentValueBeforeAllNullObjectPath",
+            "JSON(a Int64, `a.b` Map(String, Nullable(Int64)))", [("a", 5L), ("a.b", AllNullMap)], "{\"a\":5}");
+
+        // The collision is one level above the path which gives way, so the walk has to stop
+        // before it builds a container for the remaining part.
+        yield return Case("ParentValueBeforeDeeperNullPath",
+            "JSON(a Int64, `a.b.c` Nullable(Int64))", [("a", 5L), ("a.b.c", null)], "{\"a\":5}");
+
+        // The parent's value is an object or an array rather than a scalar. Neither can hold a
+        // subtree, but nothing has to: the deeper path holds nothing.
+        yield return Case("ObjectParentValueBeforeNullPath",
+            "JSON(a Map(String, Int64), `a.b` Nullable(Int64))", [("a", MapValue), ("a.b", null)], "{\"a\":{\"x\":1}}");
+
+        yield return Case("ArrayParentValueBeforeNullPath",
+            "JSON(a Array(Int64), `a.b` Nullable(Int64))", [("a", new long[] { 1, 2 }), ("a.b", null)], "{\"a\":[1,2]}");
+
+        // The collision is below a container the row built earlier, so the walk descends before it
+        // reaches the value which gives way.
+        yield return Case("NestedParentValueBeforeNullPath",
+            "JSON(`a.b` Int64, `a.b.c` Nullable(Int64))", [("a.b", 5L), ("a.b.c", null)], "{\"a\":{\"b\":5}}");
+
+        // Contrast: the deeper path holds a value, so the two collide and neither can be dropped.
+        yield return Case("ParentValueBeforePathHoldingValue",
+            "JSON(a Int64, `a.b` Nullable(Int64))", [("a", 5L), ("a.b", 7L)], null);
+
+        // Contrast: a path which gives way leaves nothing behind, so the sibling read after it
+        // still finds the parent's value and collides with it.
+        yield return Case("PathHoldingValueAfterOneWhichGaveWay",
+            "JSON(a Int64, `a.b` Nullable(Int64), `a.c` Nullable(Int64))",
+            [("a", 5L), ("a.b", null), ("a.c", 7L)], null, expectedNestedPath: "a.c");
+
+        // Contrast: the order a server produces is unchanged — the deeper path is read first, and
+        // the leaf which lands on its subtree gives way or collides by the same rule.
+        yield return Case("NullPathAfterDeeperValue",
+            "JSON(a Nullable(Int64), `a.b` Nullable(Int64))", [("a.b", 7L), ("a", null)], "{\"a\":{\"b\":7}}");
+
+        yield return Case("ValueAfterDeeperValue",
+            "JSON(a Nullable(Int64), `a.b` Nullable(Int64))", [("a.b", 7L), ("a", 5L)], null);
+
+        // AllowDuplicateJsonKeys keeps a collision readable by dropping one side, but a path which
+        // holds nothing needs no such licence, and a subtree still has nowhere to go under a
+        // scalar the row also carries.
+        yield return Case("ParentValueBeforeNullPathWithDuplicateKeysAllowed",
+            "JSON(a Int64, `a.b` Nullable(Int64))", [("a", 5L), ("a.b", null)], "{\"a\":5}", allowDuplicateJsonKeys: true);
+
+        yield return Case("ParentValueBeforePathHoldingValueWithDuplicateKeysAllowed",
+            "JSON(a Int64, `a.b` Nullable(Int64))", [("a", 5L), ("a.b", 7L)], null, allowDuplicateJsonKeys: true);
+    }
+
+    private static TestCaseData Case(string name, string columnType, (string Path, object Value)[] fields, string expectedJson, bool allowDuplicateJsonKeys = false, string expectedNestedPath = "a.b")
+        => new TestCaseData(columnType, fields, allowDuplicateJsonKeys, expectedJson, expectedNestedPath) { TestName = name };
+
+    [Test]
+    [TestCaseSource(nameof(Cases))]
+    public void Read_WithOverlappingPaths_ShouldKeepTheValueWhicheverOrderTheWireUses(
+        string columnType, (string Path, object Value)[] fields, bool allowDuplicateJsonKeys, string expectedJson, string expectedNestedPath)
+    {
+        if (expectedJson is not null)
+        {
+            Assert.That(ReadRow(columnType, fields, allowDuplicateJsonKeys).ToJsonString(), Is.EqualTo(expectedJson));
+            return;
+        }
+
+        var exception = Assert.Throws<SerializationException>(() => ReadRow(columnType, fields, allowDuplicateJsonKeys));
+
+        Assert.That(exception.Message, Does.Contain("'a'"), exception.Message);
+        Assert.That(exception.Message, Does.Contain($"'{expectedNestedPath}'"), exception.Message);
+    }
+
+    /// <summary>
+    /// Decodes one JSON column value made of the given paths, in the given order.
+    /// </summary>
+    private static JsonObject ReadRow(string columnType, (string Path, object Value)[] fields, bool allowDuplicateJsonKeys)
+    {
+        var settings = TypeSettings.Default with { allowDuplicateJsonKeys = allowDuplicateJsonKeys };
+        var type = (JsonType)TypeConverter.ParseClickHouseType(columnType, settings);
+
+        using var stream = new MemoryStream();
+        using var writer = new ExtendedBinaryWriter(stream);
+        writer.Write7BitEncodedInt(fields.Length);
+        foreach (var (path, value) in fields)
+        {
+            writer.Write(path);
+            type.HintedTypes[path].Write(writer, value);
+        }
+
+        stream.Seek(0, SeekOrigin.Begin);
+        using var reader = new ExtendedBinaryReader(stream);
+        return (JsonObject)type.Read(reader);
+    }
+}

ClickHouse.Driver/Types/JsonType.cs

@@ -93,6 +93,7 @@ private object ReadAsJsonObject(ExtendedBinaryReader reader)
 
             var pathParts = name.Split('.');
             var depth = 0;
+            var givesWay = false;
             foreach (var part in pathParts.SkipLast1(1))
             {
                 depth++;
@@ -114,6 +115,16 @@ private object ReadAsJsonObject(ExtendedBinaryReader reader)
                     current[part] = newCurrent;
                     current = newCurrent;
                 }
+                else if (HoldsNoValue(jsonNode))
+                {
+                    // The parent holds a value, but this path holds none — a null, or an empty
+                    // object value — so it gives way instead of colliding with it, the same rule
+                    // the leaf applies to a subtree it lands on. Nothing is lost by dropping it,
+                    // and no container was built for it: a part which reaches this branch has an
+                    // occupant, so it was already there before this path was walked.
+                    givesWay = true;
+                    break;
+                }
                 else
                 {
                     // The parent holds a value of its own, so this row needs both a value and a
@@ -122,11 +133,16 @@ private object ReadAsJsonObject(ExtendedBinaryReader reader)
                 }
             }
 
+            if (givesWay)
+            {
+                continue;
+            }
+
             var leaf = pathParts.Last();
             if (current[leaf] is JsonObject occupied)
             {
                 // A deeper overlapping path was read first and put its subtree here.
-                if (jsonNode is null || (jsonNode is JsonObject incoming && IsAllNull(incoming, out _)))
+                if (HoldsNoValue(jsonNode))
                 {
                     // This path holds no value — a null, or an empty object value — so it must not
                     // erase the subtree.
@@ -155,6 +171,14 @@ private object ReadAsJsonObject(ExtendedBinaryReader reader)
         return root;
     }
 
+    /// <summary>
+    /// Reports whether the node holds no value — a JSON null, or an object which is empty or
+    /// all-null. Such a path gives way to an overlapping one instead of colliding with it,
+    /// whichever of the two the wire puts first.
+    /// </summary>
+    private static bool HoldsNoValue(JsonNode node) =>
+        node is null || (node is JsonObject value && IsAllNull(value, out _));
+
     /// <summary>
     /// Reports whether the object is a path's own value rather than a container built to hold
     /// deeper paths.

RELEASENOTES.md

@@ -55,6 +55,7 @@ v1.4.0
 * `ClickHouseClient.MemoryStreamManager` is now `[Obsolete]`. Since binary inserts stream directly into the request body (see above), this property is no longer used and has no effect; it will be removed in a future version.
 
 **Bug Fixes:**
+* **Breaking Change**: Fixed a `JSON` column with overlapping paths silently losing data. ClickHouse accepts a column which declares a path both as a value and as the parent of another path — for example `JSON(a Int64, a.b Int64)` — and sends both paths in every row, so the row renders as a document with a duplicate key (`{"a":0,"a":{"b":7}}`). A `JsonObject` cannot hold two values for one key, so the driver silently kept whichever path the server sent last: reading `{"a":{"b":7}}` back out of that column returned `{"a":0}` and the `7` was gone. The same loss hit a `Map` path overlapped by a deeper path (`JSON(a Map(String, Int64))` holding `{"a":{"b":1},"a.b":7}` returned `{"a":{"b":7}}`, dropping the map's own entry), except that there the two values were merged into one object rather than one replacing the other. A row where both sides of such an overlap hold a value now throws a `SerializationException` naming the two paths. With non-`Nullable` paths the driver cannot tell a stored `0` from an absent path, so no rule can recover the real value — the choice is between a wrong answer and a clear failure. A side which holds nothing is not a collision: a null, an empty object, and an all-null subtree all give way to the side which has the data, whichever of the two paths the server sends first, so `Nullable` overlaps continue to read as before. Use `JsonReadMode.String` to read such a column as the server's JSON text, duplicate key included. To restore the previous behavior and keep reading the column as a `JsonObject`, set the new `AllowDuplicateJsonKeys` setting (connection string key `AllowDuplicateJsonKeys=true`, or `ClickHouseClientSettings.AllowDuplicateJsonKeys`); the driver then keeps whichever value the row carries last, with the data loss described above. A path which holds a value and whose parent holds a scalar or an array still throws — that shape raised `InvalidCastException` before and is now a `SerializationException`. (#583)
 * Fixed uncompressed binary inserts (`InsertOptions.Compressor = null`) inflating the HTTP request body. The request body is sent chunked, and the row serializer wrote straight to it, so every field write became its own HTTP chunk.
 * Fixed binary inserts flushing the compression stream once per value written by copying a stream — binary-mode `JSON` POCOs, and `String`/`FixedString` values given as a `Stream`.
 * Fixed `InsertOptions.WithColumnTypes()` and `InsertOptions.WithQueryId()` silently dropping some caller-set options (such as `AcceptEncoding`) when copying.

docs/overview.mdx

@@ -1530,8 +1530,8 @@ same holds where the value is a `Map`, as in `JSON(a Map(String, Int64))` read f
 also has a dynamic `a.b`.
 
 This applies only where both sides hold a value in that row. A side which holds nothing — a null,
-an empty object, or a subtree whose values are all null — gives way to the side which has the data.
-An overlap declared with `Nullable` types therefore fills in one side per row and reads without
+an empty object, or a subtree whose values are all null — gives way to the side which has the data,
+whichever of the two paths the server sends first. An overlap declared with `Nullable` types therefore fills in one side per row and reads without
 error: `JSON(a Nullable(Int64), a.b Nullable(Int64))` gives `{"a":5}` and `{"a":{"b":7}}` as
 expected.
 
@@ -1540,8 +1540,9 @@ key included.
 
 Set `AllowDuplicateJsonKeys` to keep reading the column as a `JsonObject` instead of throwing. The
 driver then keeps whichever of the two values the row carries last and drops the other, so the
-result is lossy: `JSON(a Int64, a.b Int64)` holding `{"a.b":7}` reads as `{"a":0}`. A path whose
-parent holds a scalar or an array still throws, because a subtree cannot be placed under either.
+result is lossy: `JSON(a Int64, a.b Int64)` holding `{"a.b":7}` reads as `{"a":0}`. A path which
+holds a value and whose parent holds a scalar or an array still throws, because a subtree cannot be
+placed under either.
 
 ```csharp
 var settings = new ClickHouseClientSettings("Host=localhost")