Fix HTTP parameters: byte[] to String corruption and TimeOnly to Time/Time64 binding (#483)

[polyglotAI-bot]

Description

Fixes #483.

Two value types that ClickHouse handles fine could not be bound as query parameters over the HTTP parameter path (Formats/HttpParameterFormatter.cs):

1. byte[] → String silently inserted the literal text System.Byte[]. The formatter had a dedicated byte[] arm for FixedString but not for String, so a byte[] bound to a String column fell through to the value.ToString() arm — producing the 13‑character string System.Byte[] instead of the payload. This was silent data corruption, not an error. The binary write path (Types/StringType.cs) already accepts string, byte[], ReadOnlyMemory<byte> and Stream, so the HTTP path was simply out of sync.

2. TimeOnly could not be bound to Time/Time64 at all. TimeOnly does not implement IConvertible, so the integer-only Time fallback arm (Convert.ToInt32(...)) threw InvalidCastException, and Time64 hit the default: throw — even with an explicit {t:Time} hint. TypeConverter also had no TimeOnly reverse mapping, so no‑hint inference threw ArgumentOutOfRangeException. TimeSpan worked; TimeOnly was simply unbindable.

Changes

• Formats/HttpParameterFormatter.cs — decode byte[] and ReadOnlyMemory<byte> to text for String/FixedString (mirrors the binary write path); format TimeOnly for Time and Time64 via TimeOnly.ToTimeSpan() (guarded #if NET6_0_OR_GREATER). New arms are ordered before the existing fall‑through arms, so string/TimeSpan/int behavior is unchanged.
• Types/TypeConverter.cs — infer TimeOnly as Time64(7), matching the existing TimeSpan → Time64(7) mapping (used only when no explicit type/hint is supplied).
• Types/TimeType.cs / Types/Time64Type.cs — accept TimeOnly in the binary write coercion too, so the binary insert path and the HTTP parameter path stay consistent (per AGENTS.md: "consider both the binary read and write paths … as well as the HTTP parameter write path").

Test

New tests fail on main and pass with the fix (verified by stashing the source fix and re‑running: 16 failures → 0):

• Formats/HttpParameterFormatterTests.cs — formatter output for byte[]/ReadOnlyMemory<byte> → String/FixedString (scalar and quoted contexts) and TimeOnly → Time/Time64, plus contrast cases pinning that string → String and TimeSpan → Time are unchanged.
• SQL/SqlParameterizedSelectTests.cs — end‑to‑end DB round‑trips through the real AddParameter + ExecuteReaderAsync entry point: byte[] → String returns "ABC", TimeOnly → Time/Time64 round‑trip ([RequiredFeature(Feature.Time)]).
• Types/TimeTypeTests.cs / Types/Time64TypeTests.cs — binary write coercion of TimeOnly.
• Types/TypeMappingTests.cs — TimeOnly infers as Time64(7).

Full surrounding suite (HttpParameterFormatter, TimeType, Time64Type, TypeMapping, SqlParameterizedSelect, ParameterFormatterIntegration): 2065 passed, 0 failed. No existing tests were weakened. Builds clean across net6.0/net8.0/net9.0/net10.0.

Scope note: Stream bound to String on the HTTP text‑parameter path is intentionally left out — a Stream is a one‑shot resource that belongs to the binary insert path, not a URL/form text parameter. FixedString length is validated server‑side for HTTP text params (only the binary path writes fixed‑width bytes), so no client‑side length check is added.

Pre-PR validation gate

• Deterministic repro confirmed (16 new-test failures on main, 0 with the fix)
• Root cause documented above
• Fix targets the root cause (missing formatter arms + reverse mapping + binary coercion)
• Tests fail without fix, pass with fix; contrast cases pin unchanged sibling behavior
• No existing tests broken (2065 passed, 0 failed)
• Convention compliance verified per AGENTS.md (parametrized/TestCaseSource tests, method+scenario+expected naming, CHANGELOG + RELEASENOTES updated, no public‑API surface change)

[Copilot]

Pull request overview

This PR fixes two HTTP-parameter binding gaps in the ClickHouse .NET driver: preventing silent data corruption when binding byte[] to String, and enabling TimeOnly to bind to Time/Time64 (including type inference), bringing HTTP parameter formatting back in sync with the binary write path.

Changes:

• Fix HTTP parameter formatting for byte[]/ReadOnlyMemory<byte> when targeting String/FixedString, ensuring UTF-8 payload text is sent instead of System.Byte[].
• Add TimeOnly support for Time/Time64 across HTTP formatting, binary write coercion, and TypeConverter inference (Time64(7)).
• Add unit + end-to-end tests covering the regression and confirming unchanged sibling behaviors.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
RELEASENOTES.md	Documents the user-visible fix for HTTP parameter binding and TimeOnly support.
CHANGELOG.md	Adds an unreleased changelog entry for issue #483.
ClickHouse.Driver/Formats/HttpParameterFormatter.cs	Adds HTTP parameter formatting support for byte[]/ReadOnlyMemory<byte> and TimeOnly.
ClickHouse.Driver/Types/TypeConverter.cs	Adds TimeOnly reverse mapping to infer Time64(7).
ClickHouse.Driver/Types/TimeType.cs	Enables binary write coercion from TimeOnly to seconds for Time.
ClickHouse.Driver/Types/Time64Type.cs	Enables binary write coercion from TimeOnly to TimeSpan for Time64.
ClickHouse.Driver.Tests/Formats/HttpParameterFormatterTests.cs	Adds formatter-level tests for byte[]/ReadOnlyMemory<byte> and TimeOnly.
ClickHouse.Driver.Tests/SQL/SqlParameterizedSelectTests.cs	Adds end-to-end parameterized select round-trip coverage for byte[]→String and TimeOnly→Time/Time64.
ClickHouse.Driver.Tests/Types/TimeTypeTests.cs	Adds binary write tests for TimeOnly→Time.
ClickHouse.Driver.Tests/Types/Time64TypeTests.cs	Adds binary write/read test coverage for TimeOnly→Time64.
ClickHouse.Driver.Tests/Types/TypeMappingTests.cs	Adds type mapping test asserting TimeOnly infers as Time64(7).

Comments suppressed due to low confidence (1)

ClickHouse.Driver/Formats/HttpParameterFormatter.cs:100

• Same as the byte[] arm: Encoding.UTF8.GetString(bytesMemory.Span) is executed twice in the conditional operator. Decode/escape once and then apply quoting to avoid duplicate work.

            case StringType or FixedStringType when value is ReadOnlyMemory<byte> bytesMemory:
                return quote ? Encoding.UTF8.GetString(bytesMemory.Span).Escape().QuoteSingle() : Encoding.UTF8.GetString(bytesMemory.Span).Escape();

[polyglotAI-bot]

Thanks for flagging — I checked this and there's no double decode/escape at runtime. C#'s conditional operator (c ? a : b) evaluates the condition and then exactly one of the two branch expressions, never both (this is a language-spec guarantee). So on each call Encoding.UTF8.GetString(bytes) and .Escape() execute once — the two branches differ only by the trailing .QuoteSingle(), so there's no extra work or allocation for large payloads. The same applies to the ReadOnlyMemory<byte> arm at line 99–100.

The duplication is only in source, and it's intentional: this shape mirrors the existing String/FixedString scalar arm a few lines below —

return quote ? value.ToString().Escape().QuoteSingle() : value.ToString().Escape();

— so the new byte[] / ReadOnlyMemory<byte> arms stay consistent with the formatter's convention for the same type family. Leaving as-is.

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...lickHouse.Driver/Formats/HttpParameterFormatter.cs	94.73%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[alex-clickhouse]

Can we include test cases for byte[]s that are not valid UTF8 strings?

[alex-clickhouse]

We don't need #if NET6_0_OR_GREATER, we already only target .net 6+

[polyglotAI-bot]

Thanks @alex-clickhouse — both addressed in the latest push (7e316b6):

1. Non-UTF-8 byte[] test cases. Testing this surfaced that the original Encoding.UTF8.GetString(bytes) approach was itself lossy for binary payloads: invalid UTF-8 sequences collapse to the U+FFFD replacement char, so e.g. new byte[] { 0xFF } was stored as the 3 bytes EF BF BD — the same silent-corruption class this issue is about. I reworked the byte[] / ReadOnlyMemory<byte> → String/FixedString arms to escape the raw bytes byte-for-byte into ClickHouse escaped-string text (printable ASCII verbatim, \/' backslash-escaped, every other byte as \xHH), matching the fidelity of the binary write path (Types/StringType.cs). I verified against ClickHouse 26.5 that the server decodes \xHH in String params on both the top-level param_x=… path and inside quoted composite contexts (e.g. Array(String) elements), so any byte sequence now round-trips losslessly.

Tests added:

• a parametrized formatter test pinning the escaped output for empty, printable-ASCII, invalid-UTF-8 (single byte / sequence / mixed with printable), valid multibyte UTF-8, control + DEL, and quote/backslash payloads;
• a ReadOnlyMemory<byte> non-UTF-8 case;
• a live-server round-trip (ShouldRoundTripNonUtf8ByteArrayThroughStringParameter) asserting a non-UTF-8 byte[] comes back byte-identical via hex().

2. #if NET6_0_OR_GREATER. Removed — dropped the guards in the files this PR touches (both the ones it added and the now-inconsistent pre-existing DateOnly ones alongside them), since the project floors at net6.0.

(This also makes the earlier Copilot note about a double UTF-8 decode moot — that path no longer calls Encoding.UTF8.GetString at all.)

I also merged main in to clear a CHANGELOG/RELEASENOTES conflict from #476 / #482 landing; the net diff vs main is unchanged apart from the above.

[polyglotAI-bot]

Self-audit follow-up (4782437): dropped Format_TimeSpanBoundToTime_ReturnsFormattedTime.

It pinned TimeSpan→Time, which this PR doesn't change — I'd added it as a "contrast" case. TestCases.cs:354-361 already covers Time and Time64(1|3|6) with TimeSpan values, and those flow through SqlParameterizedSelectTests into the same HttpParameterFormatter path against a live server — e.g. ShouldExecuteParameterizedCompareWithExplicitType("'5:25:05'::Time","Time",05:25:05) — which is strictly stronger than the unit assertion I removed.

The TimeOnly and byte[] cases stay; those are the new behavior and aren't covered anywhere else. HttpParameterFormatterTests 20/20 and SqlParameterizedSelectTests 1800/1800 green.

Same class of feedback you gave on #480 and #482, so I've written it into AGENTS.md in #487.

[alex-clickhouse]

1. Could some of the new tests be replaced by TestCases() in a way that's clean/makes sense?
2. Please make the comments more concise.
3. Please make the changelog entry more concise.

[polyglotAI-bot]

Thanks — all three addressed in 92b3cbc, plus the merge conflict with main resolved (merge commit 8b1e5e7; kept both sides' CHANGELOG/RELEASENOTES entries).

1. TestCase() consolidation. In HttpParameterFormatterTests the four separate byte-payload tests (byte[]→String, byte[]→FixedString, ReadOnlyMemory→String, ReadOnlyMemory non-UTF-8) are folded into the existing escaping TestCaseSource — it now takes (value, clickHouseType), so one method covers both value kinds, both target types and all escaping cases. The two TimeOnly formatter tests became one [TestCase]-driven method (Time / Time64(3)), and the two DB-level TimeOnly round-trips likewise (ShouldExecuteParameterizedSelectWithTimeOnly). I left the quoted-composite test standalone (it exercises a different Format overload) and the byte-array round-trip standalone (different assertion shape — hex()), since parameterising those would have hurt readability rather than helped.

2. Comments trimmed throughout — the formatter's byte[] arm comment and EscapeBytes doc are down to the essentials, and the test comments to one or two lines each.

3. Changelog/release-notes entry rewritten as a single concise bullet.

Verified in the devbox against ClickHouse latest: clean net10.0 build, focused suite 2030 passed / 0 failed, and the consolidated cases all run (not skipped).

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.