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. 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
 ---
 

ClickHouse.Driver.Tests/Types/JsonTypeTests.cs

@@ -1180,6 +1180,22 @@ public static IEnumerable<TestCaseData> TypedPathNullValueTestCases()
                 "{\"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,176 @@ await connection.ExecuteStatementAsync(
         Assert.That(result.ToJsonString(), Is.EqualTo("{}"));
     }
 
+    /// <param name="whenAllowed">
+    /// Document read when <c>AllowDuplicateJsonKeys</c> is set, which keeps whichever of the two
+    /// values the row carries last; <c>null</c> where the overlap stays unreadable because the
+    /// parent holds a scalar or an array, which no subtree can be placed under.
+    /// </param>
+    private static IEnumerable<(string Name, string ColumnType, string JsonData, string NestedPath, string WhenAllowed)> OverlappingPathShapes()
+    {
+        // The documents below reach a path under `a` through a flattened key rather than a nested
+        // object, because 25.8 parses `{"b": 7}` against the overlapping `a Int64` and rejects the
+        // insert with INCORRECT_DATA. The two forms are indistinguishable once stored — both give
+        // `{"a":0,"a":{"b":7}}` — so the read path under test is the same either way. See
+        // TypedPathNullValueTestCases for the same limitation.
+
+        // 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 ("OverlappingNonNullableTypedPathsWithValueInSubtree",
+            "JSON(a Int64, a.b Int64)", "{\"a.b\": 7}", "a.b", "{\"a\":0}");
+
+        yield return ("OverlappingNonNullableTypedPathsWithValueInLeaf",
+            "JSON(a Int64, a.b Int64)", "{\"a\": 5}", "a.b", "{\"a\":5}");
+
+        // 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 ("OverlappingNonNullableTypedPathsWithValueInDeeperSubtree",
+            "JSON(a Int64, a.b.c Int64)", "{\"a.b.c\": 7}", "a.b.c", "{\"a\":0}");
+
+        // Nullable typed paths only collide where the document itself carries both, which needs a
+        // duplicate key. 26.3 and later accept the document and keep both values; 25.8 rejects it
+        // with INCORRECT_DATA unless duplicated paths are skipped, which would drop one of the two
+        // values and so remove the very thing under test.
+        if (TestUtilities.ServerVersion >= Version.Parse("26.3"))
+        {
+            yield return ("OverlappingNullableTypedPathsWithDuplicateKeyDocument",
+                "JSON(a Nullable(Int64), a.b Nullable(Int64))", "{\"a\": 5, \"a\": {\"b\": 7}}", "a.b", "{\"a\":5}");
+
+            // The same collision with no hints at all: `a` and `a.b` are both dynamic paths. The
+            // scalar `a` is sent first here, so the subtree has nowhere to go even when duplicate
+            // keys are allowed.
+            yield return ("OverlappingDynamicPathsWithDuplicateKeyDocument",
+                "JSON", "{\"a\": 5, \"a\": {\"b\": 7}}", "a.b", null);
+        }
+
+        // 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 ("OverlappingMapPathAndDeeperPathWithDisjointKeys",
+            "JSON(a Map(String, Int64))", "{\"a\": {\"x\": 1}, \"a.b\": 7}", "a.b", "{\"a\":{\"x\":1,\"b\":7}}");
+
+        // The same, where the map's own key is the one the deeper path would take.
+        yield return ("OverlappingMapPathAndDeeperPathWithTheSameKey",
+            "JSON(a Map(String, Int64))", "{\"a\": {\"b\": 1}, \"a.b\": 7}", "a.b", "{\"a\":{\"b\":7}}");
+
+        // An array value cannot hold a subtree at all.
+        yield return ("OverlappingArrayPathAndDeeperPath",
+            "JSON(a Array(Int64))", "{\"a\": [1, 2], \"a.b\": 7}", "a.b", null);
+    }
+
+    public static IEnumerable<TestCaseData> OverlappingPathsHoldingValuesTestCases()
+    {
+        foreach (var (name, columnType, jsonData, nestedPath, whenAllowed) in OverlappingPathShapes())
+        {
+            yield return new TestCaseData(columnType, jsonData, false, nestedPath, null)
+                .SetName(name);
+
+            yield return new TestCaseData(columnType, jsonData, true, nestedPath, whenAllowed)
+                .SetName($"{name}_DuplicateKeysAllowed");
+        }
+    }
+
+    [Test]
+    [RequiredFeature(Feature.Json)]
+    [TestCaseSource(nameof(OverlappingPathsHoldingValuesTestCases))]
+    public async Task Read_WithOverlappingPathsHoldingValues_ShouldThrowUnlessDuplicateKeysAreAllowed(
+        string columnType, string jsonData, bool allowDuplicateJsonKeys, string expectedNestedPath, string expectedJson)
+    {
+        var targetTable = CreateTableName();
+
+        await connection.ExecuteStatementAsync(
+            $"CREATE OR REPLACE TABLE {targetTable} (data {columnType}) ENGINE = Memory;");
+        await connection.ExecuteStatementAsync($"INSERT INTO {targetTable} VALUES ('{jsonData}')");
+
+        var settings = new ClickHouseClientSettings(TestUtilities.GetTestClickHouseClientSettings())
+        {
+            AllowDuplicateJsonKeys = allowDuplicateJsonKeys,
+        };
+        using var overlapClient = new ClickHouseClient(settings);
+
+        using var reader = await overlapClient.ExecuteReaderAsync($"SELECT data FROM {targetTable}");
+
+        if (expectedJson is not null)
+        {
+            ClassicAssert.IsTrue(reader.Read());
+            Assert.That(((JsonObject)reader.GetValue(0)).ToJsonString(), Is.EqualTo(expectedJson));
+            return;
+        }
+
+        // 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();
+
+        // The row fills in the typed `a.b` and leaves the map at `a` empty, so the server renders
+        // it as `{"a":{},"a":{"b":7}}` — an overlap where only one side holds a value.
+        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}}')");
+
+        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.
+        Assert.That(result.ToJsonString(), Is.EqualTo("{\"a\":{\"b\":7}}"));
+    }
+
+    [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/ADO/ClickHouseClientSettings.cs

@@ -92,6 +92,7 @@ public ClickHouseClientSettings(ClickHouseClientSettings other)
         JsonWriteMode = other.JsonWriteMode;
 
         MapReadMode = other.MapReadMode;
+        AllowDuplicateJsonKeys = other.AllowDuplicateJsonKeys;
 
         // Copy parameter type resolver
         ParameterTypeResolver = other.ParameterTypeResolver;
@@ -365,6 +366,17 @@ public string SessionId
     /// </summary>
     public MapReadMode MapReadMode { get; init; } = ClickHouseDefaults.MapReadMode;
 
+    /// <summary>
+    /// Gets or sets how to read a JSON row where one path holds a value and is also the parent of
+    /// another path which holds a value. The server sends such a row with a duplicate key, which a
+    /// JsonObject cannot hold.
+    /// false (default): throw, because either value can only be kept by dropping the other
+    /// true: keep whichever value the row carries last and drop the other
+    /// Applies to JsonReadMode.Binary and JsonReadMode.None. JsonReadMode.String returns the
+    /// server's JSON text unchanged and is unaffected.
+    /// </summary>
+    public bool AllowDuplicateJsonKeys { get; init; } = ClickHouseDefaults.AllowDuplicateJsonKeys;
+
     /// <summary>
     /// Gets or sets a custom resolver for mapping .NET types to ClickHouse types
     /// during @-style parameter substitution. When set, this resolver is consulted
@@ -461,6 +473,7 @@ internal static ClickHouseClientSettings FromConnectionStringBuilder(ClickHouseC
             JsonReadMode = builder.JsonReadMode,
             JsonWriteMode = builder.JsonWriteMode,
             MapReadMode = builder.MapReadMode,
+            AllowDuplicateJsonKeys = builder.AllowDuplicateJsonKeys,
             AcceptEncoding = builder.AcceptEncoding,
         };
 
@@ -511,6 +524,7 @@ public bool Equals(ClickHouseClientSettings other)
                JsonReadMode == other.JsonReadMode &&
                JsonWriteMode == other.JsonWriteMode &&
                MapReadMode == other.MapReadMode &&
+               AllowDuplicateJsonKeys == other.AllowDuplicateJsonKeys &&
                ParameterTypeResolver == other.ParameterTypeResolver &&
                ParameterFormatter == other.ParameterFormatter &&
                ReadValueConverter == other.ReadValueConverter &&
@@ -555,6 +569,7 @@ public override int GetHashCode()
         hash.Add(JsonReadMode);
         hash.Add(JsonWriteMode);
         hash.Add(MapReadMode);
+        hash.Add(AllowDuplicateJsonKeys);
         hash.Add(ParameterTypeResolver);
         hash.Add(ParameterFormatter);
         hash.Add(ReadValueConverter);
@@ -608,6 +623,7 @@ public override string ToString()
                $"ReadBufferSize={ReadBufferSize};" +
                $"JsonReadMode={JsonReadMode};JsonWriteMode={JsonWriteMode};" +
                $"MapReadMode={MapReadMode};" +
+               $"AllowDuplicateJsonKeys={AllowDuplicateJsonKeys};" +
                $"UseFormDataParameters={UseFormDataParameters}";
 
         if (!string.IsNullOrEmpty(AcceptEncoding))

ClickHouse.Driver/ADO/ClickHouseConnectionStringBuilder.cs

@@ -97,6 +97,12 @@ public bool ReadStringsAsByteArrays
         set => this["ReadStringsAsByteArrays"] = value;
     }
 
+    public bool AllowDuplicateJsonKeys
+    {
+        get => GetBooleanOrDefault("AllowDuplicateJsonKeys", ClickHouseDefaults.AllowDuplicateJsonKeys);
+        set => this["AllowDuplicateJsonKeys"] = value;
+    }
+
     /// <summary>
     /// Gets or sets the size, in bytes, of the buffer used when reading HTTP query responses.
     /// Default: 8 KiB
@@ -260,6 +266,7 @@ public static ClickHouseConnectionStringBuilder FromSettings(ClickHouseClientSet
             JsonReadMode = settings.JsonReadMode,
             JsonWriteMode = settings.JsonWriteMode,
             MapReadMode = settings.MapReadMode,
+            AllowDuplicateJsonKeys = settings.AllowDuplicateJsonKeys,
             AcceptEncoding = settings.AcceptEncoding,
         };
 

ClickHouse.Driver/ADO/ClickHouseDefaults.cs

@@ -72,6 +72,11 @@ public static class ClickHouseDefaults
     /// </summary>
     public const MapReadMode MapReadMode = ClickHouse.Driver.MapReadMode.Dictionary;
 
+    /// <summary>
+    /// Default handling of a JSON row where two overlapping paths both hold a value (false = throw)
+    /// </summary>
+    public const bool AllowDuplicateJsonKeys = false;
+
     /// <summary>
     /// Default setting for server certificate validation (false = validate certificates)
     /// </summary>

ClickHouse.Driver/ClickHouseClient.cs

@@ -171,7 +171,7 @@ internal string RedactedConnectionString
     /// <summary>
     /// Gets the type settings for serialization.
     /// </summary>
-    internal TypeSettings TypeSettings => new(Settings.UseCustomDecimals, Settings.ReadStringsAsByteArrays, jsonTypeRegistry, Settings.JsonReadMode, Settings.JsonWriteMode, Settings.MapReadMode);
+    internal TypeSettings TypeSettings => new(Settings.UseCustomDecimals, Settings.ReadStringsAsByteArrays, jsonTypeRegistry, Settings.JsonReadMode, Settings.JsonWriteMode, Settings.MapReadMode, Settings.AllowDuplicateJsonKeys);
 
     /// <summary>
     /// Gets the per-client POCO type registry shared by binary insert and read materialization.

ClickHouse.Driver/PublicAPI/PublicAPI.Unshipped.txt

@@ -258,3 +258,8 @@ ClickHouse.Driver.ADO.ClickHouseClientSettings.MapReadMode.init -> void
 ClickHouse.Driver.ADO.ClickHouseConnectionStringBuilder.MapReadMode.get -> ClickHouse.Driver.MapReadMode
 ClickHouse.Driver.ADO.ClickHouseConnectionStringBuilder.MapReadMode.set -> void
 const ClickHouse.Driver.ADO.ClickHouseDefaults.MapReadMode = ClickHouse.Driver.MapReadMode.Dictionary -> ClickHouse.Driver.MapReadMode
+ClickHouse.Driver.ADO.ClickHouseClientSettings.AllowDuplicateJsonKeys.get -> bool
+ClickHouse.Driver.ADO.ClickHouseClientSettings.AllowDuplicateJsonKeys.init -> void
+ClickHouse.Driver.ADO.ClickHouseConnectionStringBuilder.AllowDuplicateJsonKeys.get -> bool
+ClickHouse.Driver.ADO.ClickHouseConnectionStringBuilder.AllowDuplicateJsonKeys.set -> void
+const ClickHouse.Driver.ADO.ClickHouseDefaults.AllowDuplicateJsonKeys = false -> bool

ClickHouse.Driver/TypeSettings.cs

@@ -2,7 +2,7 @@
 
 namespace ClickHouse.Driver;
 
-internal record struct TypeSettings(bool useBigDecimal, bool readStringsAsByteArrays, JsonTypeRegistry jsonTypeRegistry, JsonReadMode jsonReadMode, JsonWriteMode jsonWriteMode, MapReadMode mapReadMode = MapReadMode.Dictionary)
+internal record struct TypeSettings(bool useBigDecimal, bool readStringsAsByteArrays, JsonTypeRegistry jsonTypeRegistry, JsonReadMode jsonReadMode, JsonWriteMode jsonWriteMode, MapReadMode mapReadMode = MapReadMode.Dictionary, bool allowDuplicateJsonKeys = false)
 {
-    public static TypeSettings Default => new TypeSettings(true, false, null, JsonReadMode.Binary, JsonWriteMode.String, MapReadMode.Dictionary);
+    public static TypeSettings Default => new TypeSettings(true, false, null, JsonReadMode.Binary, JsonWriteMode.String, MapReadMode.Dictionary, false);
 }