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 typed 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. Such a row 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. Where the overlap is declared `Nullable`, the row normally fills in one side only and continues 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,14 @@
                 "{\"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");
+
         // 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 +1245,92 @@
         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}}")
+            .SetName("OverlappingNonNullableTypedPathsWithValueInSubtree");
+
+        yield return new TestCaseData("JSON(a Int64, a.b Int64)", "{\"a\": 5}")
+            .SetName("OverlappingNonNullableTypedPathsWithValueInLeaf");
+
+        // 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}}")
+            .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}}")
+            .SetName("OverlappingDynamicPathsWithDuplicateKeyDocument");
+    }
+
+    [Test]
+    [RequiredFeature(Feature.Json)]
+    [TestCaseSource(nameof(OverlappingPathsHoldingValuesTestCases))]
+    public async Task Read_WithOverlappingPathsHoldingValues_ShouldThrow(string columnType, string jsonData)
+    {
+        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("'a.b'"), exception.Message);
+        Assert.That(exception.Message, Does.Contain("JsonReadMode.String"), exception.Message);
+    }
+
+    [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

@@ -82,29 +82,48 @@ 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++;
+                switch (current[part])
                 {
-                    current = (JsonObject)existing;
-                }
-                else
-                {
-                    // 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.
-                    var newCurrent = new JsonObject();
-                    current[part] = newCurrent;
-                    current = newCurrent;
+                    case JsonObject existing:
+                        current = existing;
+                        break;
+
+                    case null:
+                        // Either the parent is not there yet, or it holds the null of an overlapping
+                        // leaf path (ClickHouse allows both `a` and `a.b` to be declared); the
+                        // subtree replaces that null so the deeper path stays readable.
+                        var newCurrent = new JsonObject();
+                        current[part] = newCurrent;
+                        current = newCurrent;
+                        break;
+
+                    default:
+                        // 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 occupant)
             {
-                // 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)
+                {
+                    // The null of the leaf path must not erase that subtree.
+                    continue;
+                }
+
+                if (!IsAllNull(occupant, out var occupiedPath))
+                {
+                    throw OverlappingPathsException(name, $"{name}.{occupiedPath}");
+                }
+
+                // The subtree carries no value, so the leaf's value replaces it.
             }
 
             current[leaf] = jsonNode;
@@ -113,6 +132,55 @@ private object ReadAsJsonObject(ExtendedBinaryReader reader)
         return root;
     }
 
+    /// <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,20 @@ var settings = new ClickHouseClientSettings("Host=localhost")
 // "Host=localhost;JsonReadMode=String"
 ```
 
+##### Overlapping typed 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.
+
+This applies only where both paths hold a value in that row. Where the overlap is declared with
+`Nullable` types, the row usually fills in one side only — `JSON(a Nullable(Int64), a.b
+Nullable(Int64))` reads `{"a":5}` and `{"a":{"b":7}}` normally — so nothing throws.
+
+Read such a column with `JsonReadMode.String` to get the server's JSON text unchanged, duplicate
+key included.
+
 ---
 
 #### Map type {#type-map-reading-map}