Fix CommandBehavior.SchemaOnly/SingleRow row limiting with a trailing comment or semicolon - #473
Conversation
…omment or semicolon ClickHouseCommand.ExecuteDbDataReaderAsync appended " LIMIT 0"/" LIMIT 1" verbatim to CommandText for SchemaOnly/SingleRow. A trailing single-line comment (-- or #) swallowed the clause (silently returning unbounded rows), and a trailing ; produced a "Multi-statements are not allowed" error. The clause is now appended on its own line and a trailing statement terminator is stripped, using a string/comment-aware scan (mirroring SqlParameterTypeExtractor) so semicolons and comment markers inside string literals or comments are left intact. CommandBehavior.Default is unchanged. Fixes: #471
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR fixes an ADO.NET contract violation in ClickHouseCommand.ExecuteDbDataReaderAsync where CommandBehavior.SchemaOnly / SingleRow row limiting could be defeated by trailing single-line comments or a trailing ;, by appending the limit clause on its own line and stripping a trailing statement terminator in a comment/string-aware way.
Changes:
- Introduces
RowLimitAppenderto safely strip a trailing top-level;and appendLIMIT 0/1on a new line. - Updates
ClickHouseCommand.ExecuteDbDataReaderAsyncto useRowLimitAppenderfor exactSchemaOnly/SingleRowbehaviors while keepingDefaultverbatim. - Adds unit + integration tests covering trailing comment/semicolon variants and updates release notes/changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| RELEASENOTES.md | Documents the bug fix for SchemaOnly / SingleRow with trailing comment/semicolon. |
| CHANGELOG.md | Adds the corresponding Unreleased bug-fix entry. |
| ClickHouse.Driver/ADO/RowLimitAppender.cs | New helper to strip trailing terminator and append row-limit clause safely. |
| ClickHouse.Driver/ADO/ClickHouseCommand.cs | Switches ExecuteDbDataReaderAsync row-limiting to use RowLimitAppender. |
| ClickHouse.Driver.Tests/ADO/RowLimitAppenderTests.cs | Unit tests pinning exact SQL transformations across edge cases. |
| ClickHouse.Driver.Tests/SQL/CommandBehaviorRowLimitTests.cs | Integration tests validating correct row counts through the real execution path. |
| public static string Append(string commandText, string limitClause) | ||
| { | ||
| if (string.IsNullOrEmpty(commandText)) | ||
| return "\n" + limitClause; |
There was a problem hiding this comment.
Thanks for the review. I checked this empirically (against the live server v26.5.1.882 and the .NET primitives) and the premise doesn't hold for SchemaOnly/SingleRow, so I'm leaving the code as-is.
new StringBuilder((string)null) does not throw — it yields an empty builder. So the previous ExecuteDbDataReaderAsync did not throw ArgumentNullException for a null CommandText; under SchemaOnly/SingleRow it produced " LIMIT 0" / " LIMIT 1" and sent that to the server. Verified on .NET 10:
new StringBuilder((string)null): NO THROW; Length=0; ToString()=[]
.Append(" LIMIT 0").ToString() = [ LIMIT 0]
The new code is observably identical for that case. RowLimitAppender.Append(null, "LIMIT 0") returns "\nLIMIT 0", and both the old " LIMIT 0" and the new "\nLIMIT 0" are rejected by the server with the same error — no rows, no side effect:
" LIMIT 0" -> Code 62. DB::Exception: Syntax error: failed at position 2 (LIMIT) ... (SYNTAX_ERROR)
"\nLIMIT 0" -> Code 62. DB::Exception: Syntax error: failed at position 2 (LIMIT) (line 2, col 1) ... (SYNTAX_ERROR)
So SchemaOnly/SingleRow with a null CommandText behaves exactly as before (a server-side SYNTAX_ERROR), not an unintended data-returning query.
The ArgumentNullException you're describing comes from the transport layer, not StringBuilder. new StringContent((string)null) throws ArgumentNullException (whereas new StringContent("") does not). That path is only reached when a null query string is passed through — i.e. ExecuteNonQueryAsync (unchanged by this PR — it has always passed CommandText straight to StringContent) and the CommandBehavior.Default reader / ExecuteScalar path. It is never reached for SchemaOnly/SingleRow, which always send "\nLIMIT 0/1".
For completeness: this PR does change the Default/ExecuteScalar path for a null CommandText from the old "" ("Empty query") to passing null through (transport ArgumentNullException), which actually makes it consistent with ExecuteNonQueryAsync's long-standing behavior. It's identical for every non-null CommandText (new StringBuilder(s).ToString() == s), so no real query is affected. A uniform null/empty-CommandText guard would be a reasonable follow-up, but it's out of scope for this trailing-comment/semicolon fix.
Leaving this thread unresolved for your call.
…ehavior-trailing-comment-semicolon # Conflicts: # CHANGELOG.md # RELEASENOTES.md
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…t reader path The #471 row-limit change routed the no-LIMIT CommandBehavior.Default branch of ExecuteDbDataReaderAsync straight through CommandText, dropping the null -> empty normalization the pre-#471 `new StringBuilder(CommandText)` applied to every behavior. A null CommandText therefore reached StringContent and threw a client-side ArgumentNullException instead of being sent as an empty query that the server rejects. Normalize CommandText to string.Empty for all behaviors, restoring the prior Default behavior; SchemaOnly/SingleRow already normalized via RowLimitAppender. Adds regression tests for the reader and scalar entry points.
…ehavior-trailing-comment-semicolon
The two null-CommandText cases in CommandBehaviorRowLimitTests asserted the server error code == 62, but the empty-query (Default/scalar) path returns 354 on ClickHouse 25.8 while returning 62 on 26.x, which failed the 25.8 regression leg. The contract these tests pin is "an unset CommandText reaches the server (rejected there) rather than throwing a client-side ArgumentNullException" — the exception *type* (ClickHouseServerException) encodes that, and the concrete numeric error code is a version-dependent detail. Assert the type only; the relaxed assertion still fails with ArgumentNullException if the null-normalization regression returns.
…ehavior-trailing-comment-semicolon # Conflicts: # CHANGELOG.md # RELEASENOTES.md
…ehavior-trailing-comment-semicolon # Conflicts: # CHANGELOG.md # RELEASENOTES.md
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 4 potential issues.
❌ 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 52a75d2. Configure here.
RowLimitAppender used its own hand-rolled scanner, which diverged from the server's token rules: // line comments, nestable /* /* */ */ block comments, backslash escapes in string literals, double-quoted and backtick identifiers, and heredocs were all mis-lexed. On queries the server accepts, that made the trailing ; survive and the request fail as a multi-statement - the very bug this PR fixes. Delegate to SqlTextScanner with the same dispatch order as SqlPlaceholderRewriter so there is one set of token rules, and treat a bare # as code (only "# " and "#!" start a comment), matching the server.
…ehavior-trailing-comment-semicolon # Conflicts: # CHANGELOG.md # RELEASENOTES.md
…ehavior-trailing-comment-semicolon # Conflicts: # CHANGELOG.md # RELEASENOTES.md
…ehavior-trailing-comment-semicolon # Conflicts: # CHANGELOG.md # RELEASENOTES.md
Description
Fixes #471.
ClickHouseCommand.ExecuteDbDataReaderAsyncimplementedCommandBehavior.SchemaOnly(appendLIMIT 0) andCommandBehavior.SingleRow(appendLIMIT 1) by appending the clause verbatim toCommandText. When the user's SQL ended with:-- …or# …), the appended clause landed inside the comment and the server never saw it —SchemaOnlyreturned data rows andSingleRowreturned the full result set, silently violating theCommandBehaviorcontract (no exception);;, the query became… ; LIMIT 0, which ClickHouse rejects withCode: 62 … Multi-statements are not allowed (SYNTAX_ERROR)— even though a trailing;is accepted on every other execution path of the driver.The fix appends the row-limit clause on its own line (so a trailing single-line comment cannot swallow it) and strips a trailing statement terminator before appending. The scan is string/comment-aware — mirroring
SqlParameterTypeExtractor— so a;or comment marker inside a string literal or comment is left untouched.CommandBehavior.Defaultis unchanged and continues to sendCommandTextverbatim.Changes
ClickHouse.Driver/ADO/RowLimitAppender.cs(new,internal):Append(commandText, limitClause)— strips a trailing top-level;(string/comment-aware) and appends the clause on a new line.ClickHouse.Driver/ADO/ClickHouseCommand.cs:ExecuteDbDataReaderAsyncnow builds the row-limited SQL viaRowLimitAppenderinstead of a verbatimStringBuilder.Append(" LIMIT …").CommandBehavior.Defaultstill sendsCommandTextunchanged (exact-flag match preserved).CHANGELOG.md/RELEASENOTES.md: Bug Fixes entry.Test
ClickHouse.Driver.Tests/ADO/RowLimitAppenderTests.cs— deterministic unit cases pinning the exact transformed text: plain, trailing;,--/#//* */comments,;-then-comment,;inside a string / line comment / block comment (preserved), doubled-quote escape, non-trailing;(preserved), empty input.ClickHouse.Driver.Tests/SQL/CommandBehaviorRowLimitTests.cs— integration tests through the realExecuteReaderAsyncpath against a live server:SchemaOnly→ 0 rows andSingleRow→ 1 row for every trailing-comment/semicolon variant; an in-string-;value guard (SELECT ';'); and aCommandBehavior.Defaultcontrast that returns all rows (verbatim;accepted).Verified against a live ClickHouse server (v26.5): the 8 comment/semicolon integration cases fail on
main(Code 62 or wrong row counts) and pass with this change; 1142 existing reader/schema/scalar tests remain green.Pre-PR validation gate
main, passes on branch)internalhelper)