Fix parameters: rewrite @name placeholders only in code positions, start a heredoc only at a token boundary, and treat $ as part of a placeholder name

[polyglotAI-bot]

Description

Fixes #512.

ClickHouseParameterCollection.ReplacePlaceholders rewrote each ADO-style @name into {name:ResolvedType} through StringExtensions.ReplaceMultipleWords, which built one regex out of the parameter names and replaced every match anywhere in the query text. That scanner had no notion of quoting or commenting, so an occurrence inside a string literal was rewritten as well — and because the server does not substitute query parameters inside literals, the rewritten text was returned verbatim as data: SELECT 'user@id' AS s came back as user{id:Int32}. Silent corruption, no error. The {name:Type} hint extractor on the other side of the same call site is quote/comment-aware, so the two scanners disagreed about what a placeholder is. The rewrite now walks the query once and replaces a placeholder only when it appears in a code position; string literals, double-quoted and backtick identifiers, heredocs and -- / # / /* */ comments are copied through untouched.

Realistic shapes that hit the bug: email addresses and handles in literals, LIKE '%@id%' patterns, and any literal containing @ followed by a parameter name — common with ORMs that name parameters after columns.

Changes

• ADO/Parameters/SqlPlaceholderRewriter.cs (new): single-pass, context-aware @name rewrite. Placeholders are matched longest-first (so @id cannot shadow @id_2) and, as before, are not recognized inside a longer identifier (@idx stays @idx). Unchanged queries are returned as-is; a rewritten query is assembled with one StringBuilder and spliced copies rather than a regex.
• ADO/Parameters/SqlTextScanner.cs (new): the skip primitives — quoted regions (both \' and '' escapes), heredocs ($$…$$, $tag$…$tag$), line and block comments.
• ADO/Parameters/SqlParameterTypeExtractor.cs: its two private comment-skip helpers moved to SqlTextScanner verbatim and are now called from there. No behavior change.
• Utility/StringExtensions.cs: dropped ReplaceMultipleWords, whose only caller was the placeholder rewrite.
• CHANGELOG.md / RELEASENOTES.md: bug-fix entry.

Only internal types changed, so PublicAPI/*.txt is untouched.

Test

• ClickHouse.Driver.Tests/SQL/SqlParameterizedSelectTests.AddParameter_AdoPlaceholderInsideStringLiteral_LeavesLiteralIntact — end-to-end against a real server, the reproduction from the issue: SELECT 'user@id' AS s, @id AS v must return user@id and 42. Fails on main with user{id:Int32}.
• ClickHouse.Driver.Tests/ADO/SqlPlaceholderRewriteTests — one parametrized test over the contexts the rewrite has to distinguish: literals (plain, ''-escaped, \'-escaped, LIKE pattern, empty, unterminated), double-quoted and backtick identifiers, plain and tagged heredocs, -- / # / #! / /* */ comments (including that block comments do not nest, and that code after a line comment is still rewritten), plus the code-position cases that must keep working: single and repeated placeholders, @id_2 not shadowed by @id, @idx and @idä left alone, a lone @, and a placeholder followed by an operator. 14 of its 26 cases fail without the fix; the 12 code-position cases pass before and after, pinning the unchanged behavior.

Full ClickHouse.Driver.Tests suite on net10.0: 9667 passed, 0 failed, no existing test modified.

Follow-up (not in this PR)

While verifying the two scanners now agree, the {name:Type} extractor still has its own literal tracking that honours '' but not \', and skips neither quoted identifiers nor heredocs — so it can still extract a phantom hint from inside a literal. That is the other half already described in #508 and is left to it, to keep this PR to one concern.

Pre-PR validation gate

• Deterministic repro confirmed
• Root cause documented above
• Fix targets the root cause
• Test fails without fix, passes with fix
• No existing tests broken
• Convention compliance verified per AGENTS.md (parametrized TestCaseSource, integration test against a real server, CHANGELOG + RELEASENOTES updated, no public API change)

---

Also fixes #514 — a heredoc must only start where a token starts

Added at @alex-clickhouse's request (folded in here because it is the same lexing surface): fixes #514.

$ is a word character of an ordinary ClickHouse token, so the server lexes b$c$ as a single identifier and only opens a $tag$ heredoc at a $ that begins a token. Both scanners attempted heredoc detection at every $, so an unquoted identifier containing a $ was mistaken for an opener and everything up to the next occurrence of the same $...$ text was skipped as heredoc body — silently dropping any {name:Type} hint or @name placeholder in between. WITH 1 AS b$c$ SELECT {d:Date} AS v, b$c$ AS x failed with Code: 457 ... cannot be parsed as Date (hint dropped, value formatted as an inferred DateTime), and the @id form failed with Code: 62 Syntax error ... (@).

Both TrySkipHeredoc implementations — SqlParameterTypeExtractor (hint extractor) and SqlTextScanner (used by the @name rewriter) — now refuse to open a heredoc at a $ that continues a token, i.e. when the preceding character is an ASCII letter, digit, _ or $. Verified against a live 26.5.1.882 server: b$c$, a1$c$, a_$c$, a$$c$$ and a$$c$ are each single identifiers with parameter substitution continuing around them, while a heredoc still opens at index 0, after whitespace/comma/newline, and directly after a closing backtick or quote.

Documented divergence (XML doc comment on the new helper): inspecting the preceding character misses the case where it ends a literal rather than a word — 1$tag$…$tag$, $$a$$$tag$…$tag$ — where the server does open a heredoc. Both shapes place two literals next to each other, which the server rejects as a syntax error, so no query it accepts is affected.

Tests: 11 fail without the fix (verified by stashing only the two source files) — 5 new cases in the extractor's existing DollarSignThatDoesNotOpenAHeredoc provider, 4 in HintInsideCommentOrQuotedToken pinning that a real heredoc still opens, 7 new SqlPlaceholderRewriteTests cases split the same way, and two end-to-end cases in SqlParameterizedSelectTests reproducing the issue's Code: 457 and Code: 62. No existing test edited, weakened or deleted. Focused Parameter|Sql subset on net10.0: 4400 passed / 0 failed / 136 skipped. CHANGELOG + RELEASENOTES carry a separate #514 entry.

---

Also fixes #516 — a $ is part of the placeholder name

Added at @alex-clickhouse's request, again the same surface: fixes #516.

ClickHouse accepts a $ in a query parameter name, and its lexer treats $ as a word character. Verified on a live 26.5.1.882 server: SELECT {id$x:Int32} with param_id$x=42 returns 42, as do {$x:Int32} and {id$:Int32}, and WITH 1 AS id$x SELECT id$x returns 1. The ADO @name rewrite disagreed. On main the regex built from the parameter names made @id$x\b, in which $ is an end-of-input anchor, so a $ name could never be bound at all; and the \b boundary treated $ as a non-word character, so @id matched inside @id$x.

The scanner introduced by this PR already fixed the first half (ordinal compare, longest name first). The second half survived, because MatchPlaceholder's end-of-name check still excluded $:

with only id defined	before	after
SELECT @id2	Code: 62 — left alone, server rejects it	unchanged
SELECT @id_x	Code: 62 — left alone, server rejects it	unchanged
SELECT @id$x	⚠️ silently rewritten to SELECT {id:Int32}$x	left alone, server rejects it

So an unknown placeholder was reported for a name ending in a letter, digit or _, but silently turned into a different, still valid query when the name contained a $ (SELECT @id$x executed as SELECT 45 AS \$x`). SqlPlaceholderRewriter.IsWordCharnow counts$as part of the name, matching the server's word lexer,</code>SqlParameterTypeExtractor.IsParameterNameChar<code>(which already accepted$`), and the token-boundary rule added above for #514.

Non-ASCII letters and digits are deliberately left in IsWordChar even though the server's word lexer is ASCII-only: such a name cannot be sent as param_<name> at all, so keeping it part of the name leaves the query untouched for the server to reject instead of rewriting it into another one. The existing @idä case pins that.

Tests: 10 fail without the fix (verified by stashing only SqlPlaceholderRewriter.cs) — 5 new cases in the existing SqlPlaceholderRewriteTests provider for a name continued by a $, a new DollarNamedPlaceholders provider (17 cases) covering $ at the start/middle/end of a name, doubled $, $1, longest-name precedence, dollar names in literals/quoted identifiers/heredocs/comments, and a name whose trailing $ must not be taken for a heredoc opener, plus three end-to-end tests in SqlParameterizedSelectTests: AddParameter_NameContainingDollar_IsBound, AddParameter_TypeHintForNameContainingDollar_IsApplied (the hint extractor and the rewriter must agree on where a $ name ends) and AddParameter_NameIsPrefixOfALongerName_IsNotSubstituted, parametrized over @id$x / @id2 / @id_x so the equivalence claimed above is asserted rather than assumed.

One expectation added earlier in this PR was corrected: the case SELECT $tag$@id$other$ asserted that @id is rewritten there, which #516 disproves — id$other$ is one name (and a legal parameter name: {id$other$:Int32} returns 42), and substituting @id produced $tag$42$other$, which the server lexes as one unknown identifier (Code: 47). Its original intent — that the text between a tag and a different tag is code — is now pinned by two cases that separate the two concerns: SELECT $tag$ @id $other$ and SELECT $tag$@id$ AS a, $other$ (with id$ defined). No other test was edited, weakened or deleted.

Focused Parameter|Sql subset on net10.0: 4434 passed / 0 failed / 136 skipped. CHANGELOG + RELEASENOTES carry a separate #516 entry. Only internal types changed, so PublicAPI/*.txt is untouched.

[codecov]

Codecov Report

❌ Patch coverage is 97.75281% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...se.Driver/ADO/Parameters/SqlPlaceholderRewriter.cs	95.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR fixes a silent data-corruption bug in the ADO.NET parameter pipeline by rewriting @name placeholders only when they appear in SQL code positions (not inside string literals, quoted identifiers, heredocs, or comments), aligning placeholder rewriting with ClickHouse server behavior and preventing literal content like 'user@id' from being mutated into 'user{id:Int32}'.

Changes:

• Replaced the prior regex-based placeholder rewriting with a single-pass, context-aware scanner/rewriter.
• Refactored shared SQL text “skip” primitives into a new SqlTextScanner helper and reused them from the type-hint extractor.
• Added both an end-to-end regression test and a focused unit test suite for placeholder rewriting contexts; updated changelog/release notes.

Reviewed changes

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

Show a summary per file

File	Description
ClickHouse.Driver/ADO/Parameters/SqlPlaceholderRewriter.cs	New single-pass, context-aware @name → {name:Type} rewriter (skips strings/idents/heredocs/comments).
ClickHouse.Driver/ADO/Parameters/SqlTextScanner.cs	New shared primitives for skipping quoted regions, heredocs, and comments during SQL scans.
ClickHouse.Driver/ADO/Parameters/SqlParameterTypeExtractor.cs	Uses SqlTextScanner for comment skipping helpers (no intended behavior change).
ClickHouse.Driver/ADO/Parameters/ClickHouseParameterCollection.cs	Switched placeholder replacement to SqlPlaceholderRewriter and documented the new behavior.
ClickHouse.Driver/Utility/StringExtensions.cs	Removed ReplaceMultipleWords (no remaining callers).
ClickHouse.Driver.Tests/SQL/SqlParameterizedSelectTests.cs	Added an end-to-end regression test reproducing issue #512 against a real server.
ClickHouse.Driver.Tests/ADO/SqlPlaceholderRewriteTests.cs	Added parametrized unit tests covering rewrite/non-rewrite contexts and edge cases.
CHANGELOG.md	Added unreleased bug-fix entry for issue #512.
RELEASENOTES.md	Added unreleased bug-fix entry for issue #512.

[alex-clickhouse]

Please resolve the conflicts.

[polyglotAI-bot]

Conflicts resolved — the PR is MERGEABLE again on head 306629a.

The conflict came from #511 landing on main (both PRs touch the parameter/SQL-lexing surface). Details of the resolution:

• Only one file actually conflicted: ClickHouse.Driver.Tests/SQL/SqlParameterizedSelectTests.cs, where Fix {name:Type} hints being dropped when an earlier brace is not a type hint #511 and this PR each appended a new test method at the same position. Resolved purely additively — main's AddParameter_HintPrecededByBraceThatIsNotAHint_KeepsSubSecondPrecision (with its [TestCase] set) is kept first, followed by this PR's AddParameter_AdoPlaceholderInsideStringLiteral_LeavesLiteralIntact. Nothing was dropped, edited or weakened on either side.
• CHANGELOG.md / RELEASENOTES.md auto-merged cleanly this time; both retain main's entries and this PR's @-style parameter placeholders are rewritten inside string literals: ReplacePlaceholders uses a context-blind regex while the hint extractor is quote/comment-aware #512 entry.
• SqlParameterTypeExtractor.cs merged cleanly: Fix {name:Type} hints being dropped when an earlier brace is not a type hint #511's extractor changes came in verbatim. This PR's fix lives in SqlPlaceholderRewriter.cs, SqlTextScanner.cs and ClickHouseParameterCollection.cs, none of which main touched — so no behavior of Fix {name:Type} hints being dropped when an earlier brace is not a type hint #511 was altered.

Merged as a real merge commit (git merge origin/main) — no rebase, no force-push.

Verification on the merged head (net10.0): driver builds with 0 errors; focused Parameter|Sql subset 4368 passed / 0 failed / 136 skipped (up from 4337 pre-merge, i.e. #511's new tests are included and green).

Note that resolving the conflict may have dismissed a pending review state — re-review welcome.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 306629a. Configure here.}

[Copilot]

Pull request overview

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

Suppressed comments (2)

ClickHouse.Driver.Tests/SQL/SqlParameterizedSelectTests.cs:423

• The test reads from ExecuteReaderAsync() without disposing the returned DbDataReader. If the reader owns the HTTP response/stream, this can keep resources open longer than necessary and can cause flakiness under parallel test runs. Prefer a using var reader = await ... and then read the single row from it.

        using var command = connection.CreateCommand();
        command.CommandText = "SELECT 'user@id' AS s, @id AS v";
        command.AddParameter("id", 42);

        var result = (await command.ExecuteReaderAsync()).GetEnsureSingleRow();

ClickHouse.Driver/ADO/Parameters/SqlTextScanner.cs:9

• SqlTextScanner largely duplicates the SQL-skipping helpers already implemented in SqlParameterTypeExtractor (quoted tokens, heredocs, block/line comments). Keeping two independent implementations reintroduces the drift risk that caused this class of bugs in the first place. Consider making SqlParameterTypeExtractor delegate to SqlTextScanner (or vice versa) so both placeholder rewrite and type-hint extraction share one authoritative scanner.

/// <summary>
/// Low-level helpers for walking SQL text and skipping over regions where parameter placeholders
/// must not be interpreted: quoted strings and identifiers, heredocs and comments.
/// </summary>
internal static class SqlTextScanner

[alex-clickhouse]

Can we include a fix for #514 in this PR?

[polyglotAI-bot]

@alex-clickhouse yes — done, folded into this PR as 8b7590e. It is the same lexing surface as #512 (the two scanners disagreeing with the server about where a heredoc starts), so it fits here rather than in a follow-up.

Root cause of #514. $ is a word character of an ordinary ClickHouse token, so the server lexes b$c$ as a single identifier and only opens a $tag$ heredoc at a $ that begins a token. Both scanners attempted heredoc detection at every $, so an unquoted identifier containing a $ was mistaken for an opener and everything up to the next occurrence of the same $...$ text was skipped as heredoc body — silently dropping any {name:Type} hint (issue #514) or @name placeholder in between:

WITH 1 AS b$c$ SELECT {d:Date} AS v, b$c$ AS x
  -> Code: 457 ... Value 2020-01-02T00:00:00 cannot be parsed as Date   (hint dropped, DateTime inferred)
WITH 1 AS b$c$ SELECT @id AS v, b$c$ AS x
  -> Code: 62 Syntax error ... failed at position 23 (@)                (placeholder not rewritten)

Fix (2 source files, ~10 lines): both TrySkipHeredoc implementations — SqlParameterTypeExtractor (the hint extractor) and SqlTextScanner (used by the @name rewriter) — now refuse to open a heredoc at a $ that continues a token, i.e. when the preceding character is an ASCII letter, digit, _ or $.

Verified against a live server (26.5.1.882) rather than by inspection:

WITH 1 AS b$c$   SELECT {id:Int32} AS v, b$c$   AS x   -> 42  1
WITH 1 AS a1$c$  SELECT {id:Int32} AS v, a1$c$  AS x   -> 42  1
WITH 1 AS a_$c$  SELECT {id:Int32} AS v, a_$c$  AS x   -> 42  1
WITH 1 AS a$$c$$ SELECT {id:Int32} AS v, a$$c$$ AS x   -> 42  1
SELECT a$$c$ , {id:Int32} , $c$   -> identifier `a$$c$`, _CAST(42,'Int32'), identifier `$c$`
WITH 1 AS b$c$ SELECT b$c$ , {id:Int32} , $t$ {id:String} $t$   -> 1  42  " {id:String} "

The last line is the contrast: a real heredoc in the same query stays opaque. A heredoc still opens at index 0, after whitespace/comma/newline, and directly after a closing backtick or quote.

Known, documented divergence (in an XML doc comment on the new helper): looking at the preceding character misses the case where it ends a literal rather than a word — 1$tag$…$tag$ or $$a$$$tag$…$tag$, where the server does open a heredoc. Both shapes place two literals next to each other, which the server rejects as a syntax error, so no query it accepts is affected.

Tests — 11 of them fail without the fix (verified by stashing only the two source files), including both new end-to-end cases which reproduce #514's exact Code: 457 and Code: 62:

• SqlParameterTypeExtractorTests: 5 new cases in the existing DollarSignThatDoesNotOpenAHeredoc provider, plus 4 in HintInsideCommentOrQuotedToken pinning that a real heredoc still opens (start of query, after a quoted identifier, after a comma, after a newline).
• SqlPlaceholderRewriteTests: 7 new TestCaseSource cases, same split between "not a heredoc" and "still a heredoc".
• SqlParameterizedSelectTests: AddParameter_HintBetweenIdentifiersContainingDollar_IsStillTyped (the issue's {d:Date} example) and AddParameter_PlaceholderBetweenIdentifiersContainingDollar_IsStillBound.

No existing test was edited, weakened or deleted. Focused Parameter|Sql subset on net10.0: 4400 passed / 0 failed / 136 skipped. CHANGELOG + RELEASENOTES entries added for #514 alongside the #512 one.

[alex-clickhouse]

Let's fold in #516 as well into this PR?

[polyglotAI-bot]

@alex-clickhouse done — #516 is folded in as 1662ac3, alongside #512 and #514. It is the same surface again: the two scanners have to agree with the server on where a name ends, and $ is a word character to the server both inside identifiers and inside query parameter names.

Concretely, the scanner in this PR had already fixed the "cannot bind a $ name at all" half (the old regex turned @id$x into an end-of-input anchor). What survived was the end-of-name check excluding $, so with only id defined:

• SELECT @id2 / SELECT @id_x → left alone, server rejects them (correct)
• SELECT @id$x → silently rewritten to SELECT {id:Int32}$x, i.e. executed as a different but still valid query

SqlPlaceholderRewriter.IsWordChar now counts $ as part of the name, matching SqlParameterTypeExtractor.IsParameterNameChar (which already accepted it) and the #514 token-boundary rule. 10 tests fail without the one-line change; the focused Parameter|Sql subset is 4434 passed / 0 failed on net10.0. Details, the server transcripts, and the one earlier expectation in this PR that #516 disproved (SELECT $tag$@id$other$ — id$other$ is a single, legal parameter name) are in the "Also fixes #516" section of the PR description. CHANGELOG and RELEASENOTES have a separate #516 entry, and #516 is listed so it closes on merge.