Throw instead of dropping data for overlapping JSON paths

CHANGELOG.md

@@ -1,6 +1,9 @@
 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.
+
 v1.4.0
 ---
 

ClickHouse.Driver.Tests/Types/JsonTypeTests.cs

@@ -1180,6 +1180,22 @@
                 "{\"a\":{\"b\":null}}")
             .SetName("TypedLeafPathOverlappingTypedSubtreeWithNullValues");
 
+        // Mirror of the case above: the value belongs to the leaf, and the overlapping subtree
+        // (`{"b":null}`) carries none, so the leaf's value replaces it.
+        yield return new TestCaseData(
+                "a Nullable(Int64), a.b Nullable(Int64)",
+                "{\"a\": 5}",
+                "{\"a\":5}")
+            .SetName("TypedLeafPathOverlappingTypedSubtreeWithLeafValue");
+
+        // Same, with the overlapping subtree two levels deep: it is all-null throughout, so the
+        // leaf's value still replaces it.
+        yield return new TestCaseData(
+                "a Nullable(Int64), a.b.c Nullable(Int64)",
+                "{\"a\": 5}",
+                "{\"a\":5}")
+            .SetName("TypedLeafPathOverlappingDeeperTypedSubtreeWithLeafValue");
+
         // Both `a` and `a.b` are typed and the value belongs to the subtree. Descending into an
         // object at a path that overlaps a typed leaf was added after 25.8: 26.3 and later route
         // the value to `a.b`, while 25.8 parses `{"b":7}` against `a Nullable(Int64)` and rejects
@@ -1237,6 +1253,134 @@
         Assert.That(result.ToJsonString(), Is.EqualTo("{}"));
     }
 
+    public static IEnumerable<TestCaseData> OverlappingPathsHoldingValuesTestCases()
+    {
+        // A non-Nullable typed path is never absent — an absent path materializes as 0 — so `a`
+        // always holds a value and always collides with the `a.b` subtree, whichever of the two
+        // the document fills in.
+        yield return new TestCaseData("JSON(a Int64, a.b Int64)", "{\"a\": {\"b\": 7}}", "a.b")
+            .SetName("OverlappingNonNullableTypedPathsWithValueInSubtree");
+
+        yield return new TestCaseData("JSON(a Int64, a.b Int64)", "{\"a\": 5}", "a.b")
+            .SetName("OverlappingNonNullableTypedPathsWithValueInLeaf");
+
+        // The value is deeper than the path it collides with, so the error has to walk down to it
+        // rather than name the first key under `a`.
+        yield return new TestCaseData("JSON(a Int64, a.b.c Int64)", "{\"a\": {\"b\": {\"c\": 7}}}", "a.b.c")
+            .SetName("OverlappingNonNullableTypedPathsWithValueInDeeperSubtree");
+
+        // Nullable typed paths only collide where the document itself carries both, which needs a
+        // duplicate key. ClickHouse accepts the document and keeps both values.
+        yield return new TestCaseData("JSON(a Nullable(Int64), a.b Nullable(Int64))", "{\"a\": 5, \"a\": {\"b\": 7}}", "a.b")
+            .SetName("OverlappingNullableTypedPathsWithDuplicateKeyDocument");
+
+        // The same collision with no hints at all: `a` and `a.b` are both dynamic paths.
+        yield return new TestCaseData("JSON", "{\"a\": 5, \"a\": {\"b\": 7}}", "a.b")
+            .SetName("OverlappingDynamicPathsWithDuplicateKeyDocument");
+
+        // A Map path decodes to a JsonObject, which looks exactly like a subtree built for a
+        // deeper path. The map's entries are a value of `a` all the same, and the server sends
+        // them under a key of their own, so merging `a.b` into them would drop one of the two.
+        yield return new TestCaseData("JSON(a Map(String, Int64))", "{\"a\": {\"x\": 1}, \"a.b\": 7}", "a.b")
+            .SetName("OverlappingMapPathAndDeeperPathWithDisjointKeys");
+
+        // The same, where the map's own key is the one the deeper path would take.
+        yield return new TestCaseData("JSON(a Map(String, Int64))", "{\"a\": {\"b\": 1}, \"a.b\": 7}", "a.b")
+            .SetName("OverlappingMapPathAndDeeperPathWithTheSameKey");
+
+        // An array value cannot hold a subtree at all.
+        yield return new TestCaseData("JSON(a Array(Int64))", "{\"a\": [1, 2], \"a.b\": 7}", "a.b")
+            .SetName("OverlappingArrayPathAndDeeperPath");
+    }
+
+    [Test]
+    [RequiredFeature(Feature.Json)]
+    [TestCaseSource(nameof(OverlappingPathsHoldingValuesTestCases))]
+    public async Task Read_WithOverlappingPathsHoldingValues_ShouldThrow(string columnType, string jsonData, string expectedNestedPath)
+    {
+        var targetTable = CreateTableName();
+
+        await connection.ExecuteStatementAsync(
+            $"CREATE OR REPLACE TABLE {targetTable} (data {columnType}) ENGINE = Memory;");
+        await connection.ExecuteStatementAsync($"INSERT INTO {targetTable} VALUES ('{jsonData}')");
+
+        using var reader = await connection.ExecuteReaderAsync($"SELECT data FROM {targetTable}");
+
+        // The column is decoded by Read, so the throw lands there rather than on GetValue.
+        var exception = Assert.Throws<SerializationException>(() => reader.Read());
+
+        Assert.That(exception.Message, Does.Contain("'a'"), exception.Message);
+        Assert.That(exception.Message, Does.Contain($"'{expectedNestedPath}'"), exception.Message);
+        Assert.That(exception.Message, Does.Contain("JsonReadMode.String"), exception.Message);
+    }
+
+    [Test]
+    [RequiredFeature(Feature.Json)]
+    public async Task Read_WithOverlappingEmptyObjectValue_ShouldKeepTheSubtree()
+    {
+        var targetTable = CreateTableName();
+
+        // Every key of the document is routed to a path of its own — `a.b` typed, `a.x` dynamic —
+        // so the map at `a` is empty in every row.
+        await connection.ExecuteStatementAsync(
+            $"CREATE OR REPLACE TABLE {targetTable} (data JSON(a Map(String, Int64), `a.b` Int64)) ENGINE = Memory;");
+        await connection.ExecuteStatementAsync($"INSERT INTO {targetTable} VALUES ('{{\"a\": {{\"b\": 7, \"x\": 1}}}}')");
+
+        using var reader = await connection.ExecuteReaderAsync($"SELECT data FROM {targetTable}");
+        ClassicAssert.IsTrue(reader.Read());
+
+        var result = (JsonObject)reader.GetValue(0);
+
+        // An empty map holds no value, so it neither collides with the subtree nor erases it.
+        var a = (JsonObject)result["a"];
+        Assert.That((long)a["b"], Is.EqualTo(7L));
+        Assert.That((long)a["x"], Is.EqualTo(1L));
+    }
+
+    [Test]
+    [RequiredFeature(Feature.Json)]
+    public async Task Read_WithOverlappingPathsHoldingValues_ShouldBeReadableAsString()
+    {
+        var targetTable = CreateTableName();
+
+        await connection.ExecuteStatementAsync(
+            $"CREATE OR REPLACE TABLE {targetTable} (data JSON(a Int64, a.b Int64)) ENGINE = Memory;");
+        await connection.ExecuteStatementAsync($"INSERT INTO {targetTable} VALUES ('{{\"a\": {{\"b\": 7}}}}')");
+
+        using var stringClient = TestUtilities.GetTestClickHouseClient(jsonReadMode: JsonReadMode.String);
+
+        using var reader = await stringClient.ExecuteReaderAsync($"SELECT data FROM {targetTable}");
+        ClassicAssert.IsTrue(reader.Read());
+
+        Assert.That(reader.GetValue(0), Is.EqualTo("{\"a\":0,\"a\":{\"b\":7}}"));
+    }
+
+    [Test]
+    [RequiredFeature(Feature.Json)]
+    public async Task Read_WithNestedPathsThatDoNotOverlap_ShouldReadEveryPath()
+    {
+        var targetTable = CreateTableName();
+
+        await connection.ExecuteStatementAsync(
+            $"CREATE OR REPLACE TABLE {targetTable} " +
+            "(data JSON(a.b Int64, a.c Nullable(Int64), a.d.e String, f Int64)) ENGINE = Memory;");
+        await connection.ExecuteStatementAsync(
+            $"INSERT INTO {targetTable} VALUES ('{{\"a\": {{\"b\": 1, \"d\": {{\"e\": \"x\"}}}}, \"f\": 2}}')");
+
+        using var reader = await connection.ExecuteReaderAsync($"SELECT data FROM {targetTable}");
+        ClassicAssert.IsTrue(reader.Read());
+
+        var result = (JsonObject)reader.GetValue(0);
+
+        // The server sends the paths in its own order, so assert per path rather than on the
+        // rendered document.
+        var a = (JsonObject)result["a"];
+        Assert.That((long)a["b"], Is.EqualTo(1L));
+        Assert.That(a["c"], Is.Null);
+        Assert.That((string)((JsonObject)a["d"])["e"], Is.EqualTo("x"));
+        Assert.That((long)result["f"], Is.EqualTo(2L));
+    }
+
     private class NullableHintedData
     {
         public int? Value { get; set; }

ClickHouse.Driver/Types/JsonType.cs

@@ -64,6 +64,12 @@ private object ReadAsJsonObject(ExtendedBinaryReader reader)
     {
         JsonObject root = new();
 
+        // A Map path, or a Dynamic one holding an object, decodes to a JsonObject which is that
+        // path's value — not a container built here to hold deeper paths. The two are the same
+        // type, so the values are tracked by reference. Allocated only where such a path exists,
+        // which a column of plain scalar paths never has.
+        HashSet<object> objectValues = null;
+
         var nfields = reader.Read7BitEncodedInt();
         for (int i = 0; i < nfields; i++)
         {
@@ -82,29 +88,59 @@ private object ReadAsJsonObject(ExtendedBinaryReader reader)
             }
 
             var pathParts = name.Split('.');
+            var depth = 0;
             foreach (var part in pathParts.SkipLast1(1))
             {
-                if (current[part] is { } existing)
+                depth++;
+                var occupant = current[part];
+
+                if (occupant is JsonObject subtree && !IsObjectValue(objectValues, subtree))
                 {
-                    current = (JsonObject)existing;
+                    // A container built earlier in this row for an overlapping deeper path.
+                    current = subtree;
                 }
-                else
+                else if (occupant is null || (occupant is JsonObject empty && IsAllNull(empty, out _)))
                 {
-                    // Either the parent is not there yet, or it holds the null of an overlapping
-                    // typed leaf path (ClickHouse allows both `a` and `a.b` to be typed); the
-                    // subtree replaces that null so the deeper path stays readable.
+                    // Nothing is there yet, or what is there holds no value — the null of an
+                    // overlapping leaf path, or an empty object value. ClickHouse allows both `a`
+                    // and `a.b` to be declared; the subtree replaces it so the deeper path stays
+                    // readable.
                     var newCurrent = new JsonObject();
                     current[part] = newCurrent;
                     current = newCurrent;
                 }
+                else
+                {
+                    // The parent holds a value of its own, so this row needs both a value and a
+                    // subtree under one key. Nothing can be dropped without losing data.
+                    throw OverlappingPathsException(string.Join(".", pathParts.Take(depth)), name);
+                }
             }
 
             var leaf = pathParts.Last();
-            if (jsonNode is null && current[leaf] is JsonObject)
+            if (current[leaf] is JsonObject occupied)
             {
-                // Mirror of the walk above for the opposite path order: a typed leaf path's null
-                // must not erase the subtree an overlapping deeper typed path already filled in.
-                continue;
+                // A deeper overlapping path was read first and put its subtree here.
+                if (jsonNode is null || (jsonNode is JsonObject incoming && IsAllNull(incoming, out _)))
+                {
+                    // This path holds no value — a null, or an empty object value — so it must not
+                    // erase the subtree.
+                    continue;
+                }
+
+                if (!IsAllNull(occupied, out var occupiedPath))
+                {
+                    throw OverlappingPathsException(name, $"{name}.{occupiedPath}");
+                }
+
+                // The subtree carries no value either, so this path's value replaces it.
+            }
+
+            if (jsonNode is JsonObject objectValue)
+            {
+                // Mark it as a value so that an overlapping deeper path read later throws instead
+                // of descending into it and merging the two.
+                (objectValues ??= new HashSet<object>(ObjectReferenceEqualityComparer.Instance)).Add(objectValue);
             }
 
             current[leaf] = jsonNode;
@@ -113,6 +149,62 @@ private object ReadAsJsonObject(ExtendedBinaryReader reader)
         return root;
     }
 
+    /// <summary>
+    /// Reports whether the object is a path's own value rather than a container built to hold
+    /// deeper paths.
+    /// </summary>
+    private static bool IsObjectValue(HashSet<object> objectValues, JsonObject value) =>
+        objectValues is not null && objectValues.Contains(value);
+
+    /// <summary>
+    /// Reports whether every value the object holds, at any depth, is a JSON null. An empty object
+    /// holds no value, so it counts as all-null. An array counts as a value.
+    /// </summary>
+    /// <param name="valuePath">
+    /// Dotted path of the first value which is not null, relative to <paramref name="value"/>;
+    /// <c>null</c> when the object is all-null.
+    /// </param>
+    private static bool IsAllNull(JsonObject value, out string valuePath)
+    {
+        foreach (var property in value)
+        {
+            if (property.Value is null)
+            {
+                continue;
+            }
+
+            if (property.Value is JsonObject nested)
+            {
+                if (IsAllNull(nested, out var nestedPath))
+                {
+                    continue;
+                }
+
+                valuePath = $"{property.Key}.{nestedPath}";
+                return false;
+            }
+
+            valuePath = property.Key;
+            return false;
+        }
+
+        valuePath = null;
+        return true;
+    }
+
+    /// <summary>
+    /// Builds the error for a row where one path holds a value and is also the parent of another
+    /// path which holds a value. ClickHouse accepts such a column (for example
+    /// <c>JSON(a Int64, a.b Int64)</c>) and renders the row with a duplicate key, which a
+    /// <see cref="JsonObject"/> cannot hold.
+    /// </summary>
+    private static SerializationException OverlappingPathsException(string path, string nestedPath) =>
+        new SerializationException(
+            $"JSON paths '{path}' and '{nestedPath}' overlap and both hold a value in this row. " +
+            $"'{path}' is a value and is also the parent of '{nestedPath}', so the server sends the row with a " +
+            $"duplicate '{path}' key, which a JsonObject cannot hold. " +
+            $"Read this column with JsonReadMode.String to get the server's JSON text unchanged.");
+
     public override ParameterizedType Parse(
         SyntaxTreeNode node,
         Func<SyntaxTreeNode, ClickHouseType> parseClickHouseType,

docs/overview.mdx

@@ -1505,6 +1505,24 @@ var settings = new ClickHouseClientSettings("Host=localhost")
 // "Host=localhost;JsonReadMode=String"
 ```
 
+##### Overlapping paths
+
+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)`. Both paths are present in every row, so the server
+renders the row with a duplicate key: `{"a":0,"a":{"b":7}}`. A `JsonObject` cannot hold two values
+for one key, so `JsonReadMode.Binary` throws a `SerializationException` naming the two paths. The
+same holds where the value is a `Map`, as in `JSON(a Map(String, Int64))` read from a row which
+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
+error: `JSON(a Nullable(Int64), a.b Nullable(Int64))` gives `{"a":5}` and `{"a":{"b":7}}` as
+expected.
+
+Read such a column with `JsonReadMode.String` to get the server's JSON text unchanged, duplicate
+key included.
+
 ---
 
 #### Map type {#type-map-reading-map}