Fix JSON string values base64-corrupted under ReadStringsAsByteArrays - #485
Merged
alex-clickhouse merged 6 commits intoAug 5, 2026
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
`ReadStringsAsByteArrays` propagates into the JSON decoder, so every string leaf inside a `JSON` column was read as a `byte[]`. `ReadJsonValue`'s type switch had no arm for one, so it fell through to the `JsonSerializer.SerializeToElement` default, which renders a byte array as base64: `payload["event"].GetValue<string>()` returned `"aW5mbw=="` instead of `"info"`, with no exception and no visible change to the node's type. Numeric and boolean leaves were unaffected, which made the corruption easy to miss, and reading a document then writing it back persisted the base64 text into ClickHouse. A `Map(String, ...)` path failed differently: `ReadJsonMap` cast the decoded key straight to `string`, throwing `InvalidCastException`. Both now go through a shared `DecodeString` helper, which is what `ReadJsonFixedString` already did four lines away. Because `ReadJsonArray` and `ReadJsonMap` recurse through `ReadJsonNode` -> `ReadJsonValue`, the single new switch arm covers top-level, array-nested and map-nested strings at once, including `LowCardinality(String)` and `Nullable(String)`. Behavioural change, deliberately not honouring the flag inside JSON: RFC 8259 defines JSON strings as text, so the "a ClickHouse String is arbitrary bytes" rationale behind `ReadStringsAsByteArrays` does not apply within a JSON document, and `JsonValue` has no byte-array representation to expose instead. Decoding is lenient (invalid sequences yield U+FFFD rather than failing the row), matching `ReadJsonFixedString`. The flag is unchanged for ordinary `String`/`FixedString`/`Dynamic` columns. Adds `JsonStringAsByteArrayTests` (15 cases), covering the gap that allowed this: no existing test combined the flag with a `JSON` column. 12 of the 15 fail before this change; the other three are regression guards and the source documents which are which and why. They assert the decoded values, that string leaves are backed by `string` rather than `JsonElement` (the exact discriminator between the two code paths), that flag-on output is byte-identical to flag-off across ten JSON shapes covering both the hinted and the dynamic path-type construction sites, and that a read-then-write round trip no longer stores base64. No public API change; `PublicAPI/*.txt` is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review caught a regression in the previous commit. The new `byte[]` arm keyed
off the CLR type of the decoded value, but a `byte[]` is not on its own
evidence of a string: `ArrayType.Read` allocates by
`UnderlyingType.FrameworkType`, so `Array(UInt8)` materializes as a real
`byte[]` too - with `ReadStringsAsByteArrays` on *or off*.
`ReadJsonNode` intercepts `ArrayType` directly, so a plain `Array(UInt8)` hint
was fine, but any wrapper it does not intercept passes the value through to
`ReadJsonValue`, where the arm reinterpreted those bytes as UTF-8:
JSON(v Variant(Array(UInt8), String)) [1, 2] "AQI=" -> "��"
JSON(s SimpleAggregateFunction(anyLast, Array(UInt8))) "AQI=" -> "��"
JSON(v Variant(Array(UInt8), String)) [255, 254] "//4=" -> two U+FFFD
Two things made this worse than the bug being fixed. It fired at the *default*
flag setting, so it was an undisclosed behaviour change for everyone rather
than for opt-in users. And it was lossy where the old output was not: `"//4="`
decodes back to `[255, 254]`, whereas the replacement characters have destroyed
the bytes.
The arm is now gated on `IsTextBacked(type)`, which decides from the
originating ClickHouse type: `String`/`FixedString`, and those wrapped in
`LowCardinality`, `Nullable` or `SimpleAggregateFunction`. `Variant` and
`Dynamic` answer false deliberately - their subtype is chosen per value at read
time and is not knowable from the static type, so they keep their existing
behaviour instead of risking the same misinterpretation. Every shape the tests
and release notes actually claim still decodes as text.
The four new cases assert against fixed expected values rather than comparing
flag-on to flag-off, because `Array(UInt8)` reads identically under both
settings - the flag-on-equals-flag-off invariant is structurally incapable of
catching this, and only pinning the literal output does. Verified: they fail
with the guard removed and pass with it.
Also corrects two claims the review found inaccurate:
- the notes implied `Map` keys work under `LowCardinality`; `ReadJsonMap`'s
guard is an exact `is not StringType` test, so `Map(LowCardinality(String),
String)` still throws `NotSupportedException`. Pre-existing, now stated.
- `Dynamic` was listed as unaffected, but a `Dynamic` column holding a `JSON`
value does get the fix, since its leaves go through the JSON reader. Only a
`Dynamic` holding a string still yields `byte[]`.
And renames four `using var connection` locals that shadowed the inherited
fixture field, since one test deliberately uses the inherited one to mean the
flag-off connection.
net9.0: 0 failed, 9584 passed, 142 skipped (baseline 9565/142, so exactly +19).
Green on net6.0 and net10.0 too. Full build 0 errors, 293 warnings, matching
baseline.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fix applies to whichever read mode decodes a JSON document structurally, and
that is two of the three modes, not just the default. Tests only exercised
`Binary`, so `None` was fixed without being covered and `String` was untouched
without that being pinned. Verified against pristine main:
JsonReadMode flag main this branch
Binary true {"n":42,"s":"aW5mbw=="} {"n":42,"s":"info"}
Binary false {"n":42,"s":"info"} unchanged
None true {"n":42,"s":"aW5mbw=="} {"n":42,"s":"info"}
None false {"n":42,"s":"info"} unchanged
String true {"n":42,"s":"info"} unchanged
String false {"n":42,"s":"info"} unchanged
`None` differs from `Binary` only in not sending the server-side format setting
(for read-only connections that cannot set one), so it decodes through the same
path and carried the same bug. Its new case fails without the fix, alongside
`Binary`'s.
`String` never had the bug: the server sends the whole document as a single
string and `JsonType.Read` returns it via `ExtendedBinaryReader.ReadString()`
before any per-path type dispatch, so `ReadStringsAsByteArrays` cannot reach it.
Pinned with a guard test rather than left implicit.
That last point is worth more than a footnote, so the release notes now say it:
one of the driver's three JSON read modes *already* returned real text
regardless of the flag. Treating JSON strings as text is therefore not a new
convention invented by this PR - `Binary` and `None` were the inconsistent ones.
The lenient decoding lines up too, since `ExtendedBinaryReader` is constructed
with a replacement-fallback UTF-8 decoder, which is exactly what
`Encoding.UTF8.GetString` does.
net9.0: 0 failed, 9587 passed, 142 skipped (baseline 9565/142, so exactly +22).
Green on net6.0 and net10.0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The entry had grown to 273 words in CHANGELOG.md and longer still in RELEASENOTES.md, where it had also diverged into a differently-worded lead bullet with four sub-bullets. Both are wrong for this repo: - Every other Bug Fixes entry in `Unreleased` is byte-identical between the two files. Mine was the only one that differed. - Existing entries run 15-102 words (median ~40). At 273 it was nearly 3x the longest one in the file. Now one 73-word entry, identical in both files, covering just what a reader needs: the symptom, that a round trip persisted it, the map-key exception, the behavioural change with its precedent, and the action to take. The details that were in the sub-bullets - affected read modes, the `Array(UInt8)` exclusion, `Variant`/`Dynamic` scope, U+FFFD on invalid UTF-8 - stay in the PR description and the code comments, which is where that depth belongs. Notably most of them described things that did *not* change, which do not warrant changelog space. Docs only; no code or test changes. Verified: the `Unreleased` Bug Fixes blocks of the two files now diff clean, build 0 errors / 293 warnings, fixture 22/22. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback, and now also the house rules AGENTS.md picked up in #487: comments stay short and assert only what has been verified, and changelog entries stay to the user-visible change with the detail left to the PR. The two XML doc blocks in JsonType.cs kept only the non-obvious part -- that the text/bytes decision has to come from the ClickHouse type, since Array(UInt8) also reads as a byte[]. The changelog entry goes from 73 words to 52, identical in both files. No behaviour or test-assertion change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alex-clickhouse
force-pushed
the
alex-clickhouse/json-bytearray-string-corruption
branch
from
August 4, 2026 07:40
856dae6 to
5020fc1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a ReadStringsAsByteArrays = true interaction where JSON string leaves were being surfaced as base64 (via JsonSerializer fallback) and Map(String, ...) JSON keys could throw InvalidCastException, by explicitly decoding string-backed byte[] values as UTF-8 text when reading JSON columns.
Changes:
- Added a guarded
byte[]switch arm in the JSON value decoder plus sharedDecodeString/IsTextBackedhelpers to correctly interpret string-backed byte arrays without corruptingArray(UInt8)paths. - Updated JSON map key decoding to handle
ReadStringsAsByteArrayswithout invalid casts. - Added a dedicated test suite covering dynamic/typed JSON paths, map keys, structured read modes, and non-text
byte[]cases, and documented the behavior change in changelog/release notes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ClickHouse.Driver/Types/JsonType.cs | Decodes JSON string leaves from byte[] when the originating ClickHouse type is string-backed, preventing base64 corruption and fixing map key decoding. |
| ClickHouse.Driver.Tests/Types/JsonStringAsByteArrayTests.cs | Adds coverage for the ReadStringsAsByteArrays + JSON interaction across read modes, shapes, and byte-array non-text guard cases. |
| CHANGELOG.md | Notes the user-visible behavior change for JSON strings under ReadStringsAsByteArrays. |
| RELEASENOTES.md | Mirrors the changelog entry to inform users about the behavior change and potential workaround removal. |
alex-clickhouse
enabled auto-merge (squash)
August 4, 2026 09:54
alex-clickhouse
disabled auto-merge
August 4, 2026 09:57
alex-clickhouse
enabled auto-merge (squash)
August 4, 2026 09:59
alex-clickhouse
deleted the
alex-clickhouse/json-bytearray-string-corruption
branch
August 5, 2026 08:32
alex-clickhouse
added a commit
that referenced
this pull request
Aug 5, 2026
main added 22 new Unreleased entries since this branch was cut. Each is now its own changelog.d/ fragment, extracted verbatim by line number rather than retyped, so the assembled Unreleased section reproduces main's exactly (as a set of lines; sorting by PR number reorders entries within their sections). New fragments, one per (PR, category): #390 improvements multidim blittable inserts #472 improvements per-scalar Span<byte> reads #484 fixes byte[]/TimeOnly HTTP parameters #485 fixes JSON strings under ReadStringsAsByteArrays #490 breaking raw results return compressed bytes #490 features AcceptEncoding response compression #490 improvements lz4 by default, HttpClient, errors, deflate #492 fixes HTTP response disposal #493 fixes Enum type declarations #494 fixes raw-stream double dispose #497 fixes GetSchema("Columns") restrictions #498 fixes JSON paths starting with setting names #503 fixes quoted JSON typed paths #504 fixes quoted Tuple/Nested element names #509 fixes {name:Type} scanner vs server lexer #511 fixes {name:Type} hints after a non-hint brace #513 fixes @name placeholders, heredocs, $ in names #390's entry was appended to the *released* v1.3.0 section on main (v1.3.0 shipped 2026-06-29), so it would have documented an unreleased change under a shipped version and never appeared in 1.4.0's notes. It moves to Unreleased as a fragment; the rest of v1.3.0 is byte-identical. RELEASENOTES.md regenerated with --sync-notes. `--check` passes, the solution builds, and the packed .nupkg's releaseNotes open on v1.3.0 with no Unreleased stub and no #390 bullet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
ReadStringsAsByteArrays = truesilently returned base64 in place of every string value in aJSONcolumn:The setting propagates into the JSON decoder, so every string leaf read as a
byte[].ReadJsonValue's type switch has astringarm but nobyte[]arm, so the value fell through toJsonSerializer.SerializeToElement, which renders a byte array as base64 — no exception, no visible change to the node's type. Numeric and boolean leaves were unaffected, which is what made it easy to miss, and a read-then-write round trip persisted the base64 into ClickHouse.Map(String, ...)keys failed differently:ReadJsonMapcast straight to(string), so they threwInvalidCastException.The fix
One switch arm plus a shared
DecodeStringhelper, reused byReadJsonValue,ReadJsonMap's key read, andReadJsonFixedString(which had the logic inline already). SinceReadJsonArray/ReadJsonMaprecurse throughReadJsonNode, that one arm covers top-level, array-nested and map-nested strings at once.The arm is gated on
IsTextBacked(type), and that guard is the part worth reviewing. Abyte[]is not on its own evidence of a string:ArrayType.Readallocates byUnderlyingType.FrameworkType, soArray(UInt8)also materializes as a realbyte[]— with the setting on or off. Deciding from the CLR type therefore manglesArray(UInt8)at the default setting, and lossily, since[255,254]collapses to two U+FFFD where the prior base64"//4="was at least reversible. So the decision comes from the originating ClickHouse type, recursing throughLowCardinality/Nullable/SimpleAggregateFunction.Variant/Dynamicanswer false — their subtype is chosen per value and isn't knowable statically.No public API change;
PublicAPI/*.txtis untouched.Behaviour change
String leaves inside a
JSONcolumn are now always decoded as UTF-8 text, regardless ofReadStringsAsByteArrays. This is deliberate: JSON strings are text by definition (RFC 8259),JsonValuehas no byte-array representation, and the driver already made this call forFixedStringpaths and inJsonReadMode.String— soBinary/Nonewere the inconsistent ones. Decoding is lenient (invalid bytes become U+FFFD), matching the UTF-8 decoderExtendedBinaryReaderalready uses; happy to switch to strict if you'd rather it be loud.The setting is unchanged for ordinary
String/FixedStringcolumns and forDynamicholding a string. Anyone base64-decoding these values as a workaround will double-decode after this — called out in the release notes.Tests
New
JsonStringAsByteArrayTests, 22 cases: hinted and dynamic paths, map keys and values, both affected read modes, and a flag-on-equals-flag-off invariant across ten JSON shapes. 14 fail without the fix.The four
Array(UInt8)cases assert literal values rather than flag-on-vs-flag-off, because those paths read identically under both settings — the invariant is structurally blind to them. All four fail without theIsTextBackedguard.Four cases pin behaviour this change doesn't alter (
FixedStringwas already correct,NullStringshort-circuits,EmptyStringcan't discriminate since base64 of zero bytes is also empty, andJsonReadMode.Stringcan't reach the setting at all). Per the "don't restate existing coverage" rule in AGENTS.md these are droppable; I've kept them becauseJsonReadMode.Stringis the precedent for the behaviour change rather than an idle control. Say if you'd prefer them gone.Root enabler worth stating: repo-wide, no test combined
ReadStringsAsByteArrayswith aJSONcolumn.Verification
Measured against a second worktree pinned to
main@ 9cde16c, so the delta is measured rather than assumed:mainDelta is exactly +22 — the new tests, nothing else. Also green on net6.0 and net10.0, since the CLR-type assertions depend on
System.Text.Jsoninternals that differ across frameworks.Types/JsonType.cscoverage is 95.6% (390/408) with no new uncovered line.Known gaps, pre-existing and left alone
Variant/Dynamicsubtypes — aJSON(d Dynamic)holding a string still base64s. Fixing it properly means havingReadJsonNoderecurse on the decoded subtype, which would also fixDynamicholding anArray/Map(currentlyInvalidOperationException). Separate change, own tests.Tuple/Nestedtyped paths insideJSONcan't be read at all, with the setting on or off.Map(LowCardinality(String), String)keys throwNotSupportedException.Nullable(String)path is dropped from the document entirely ({}rather than{"n":null}).Two more instances of this same root cause —
GetSchema("Columns")throwing andGetString()returning"System.Byte[]"— are filed separately as #486.Still marked draft: tell me if you'd like strict decoding, or any of the gaps folded in, before this goes up for real review.
🤖 Generated with Claude Code