Fix clickhouse-jdbc: skip heredoc literals when extracting ? placeholders - #3036
Closed
polyglotAI-bot wants to merge 3 commits into
Closed
Fix clickhouse-jdbc: skip heredoc literals when extracting ? placeholders#3036polyglotAI-bot wants to merge 3 commits into
polyglotAI-bot wants to merge 3 commits into
Conversation
…ders JdbcParameterizedQuery.parse() skipped quoted strings and --/ comments but had no notion of a heredoc literal ($$...$$ / $tag$...$tag$), so the contents of a heredoc were parsed as SQL: a '?' inside one was counted as a bind parameter, a ';' was rejected as a multi-statement query, a '\'' broke the scan with "Missing quote: '", and a ':' could be mistaken for the delimiter of the ? : ternary operator, silently dropping a real placeholder. A heredoc is now skipped as an opaque token, both in the placeholder scan and in the ternary lookahead (including inside brackets, where a bracket or quote in a heredoc body used to end the enclosing bracket or string). A '$' only opens a heredoc when it does not continue an identifier (a$b, a$x$), its tag contains word characters only, and a matching closing tag exists - otherwise it stays an ordinary character, matching the server lexer. Fixes: #3035
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
This was referenced Aug 4, 2026
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 9cddd20. Configure here.
| i = ClickHouseUtils.skipQuotedString(originalQuery, i, len, ch) - 1; | ||
| } else if (ch == '?') { | ||
| int idx = ClickHouseUtils.skipContentsUntil(originalQuery, i + 2, len, '?', ':'); | ||
| int idx = skipUntilTernaryDelimiter(originalQuery, i + 2, len); |
There was a problem hiding this comment.
Adjacent heredoc breaks ternary scan
Medium Severity
After a ?, ternary lookahead starts at i + 2, so it skips the character right after the placeholder. When that character is the opening $ of a heredoc (?$$… or ?$tag$…), skipHeredoc never sees a valid opener and scans the heredoc body as SQL. A : in the body can drop a real bind parameter; a ? in the body can turn a ternary ? into an extra parameter. Spaced forms work; adjacent ones do not.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9cddd20. Configure here.
|
Contributor
|
Original issue is closed as wont fix. |
Collaborator
Author
|
Understood, thanks for the clear reason — closing on our side too. Note on the accepted critique: cloning the skip logic into a private helper instead of fixing Housekeeping: a push from an automated run landed on |
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
2 participants
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.
Description
Fixes #3035.
JdbcParameterizedQuery.parse()scans the SQL for?placeholders and knows about quoted strings and--//* */comments, but it has no notion of a ClickHouse heredoc literal ($$...$$/$tag$...$tag$). The contents of a heredoc were therefore parsed as SQL, so a?inside one wascounted as a bind parameter (the application cannot supply a value for it, and its real parameter ends
up bound to the wrong position), a
;was rejected as a multi-statement query, a'made the scan runoff the end of the statement (
Missing quote: '), and a:could be mistaken for the delimiter of the? :ternary operator, silently dropping a real placeholder. The server treats a heredoc as a singlestring literal, so all of these statements execute fine when sent as-is.
A heredoc is now skipped as an opaque token. A
$only opens one when it does not continue anidentifier (
a$b,a$x$), its tag contains word characters only, and a matching closing tag exists —otherwise it stays an ordinary character, matching the server lexer.
Changes
clickhouse-jdbc/.../JdbcParameterizedQuery.javaskipHeredoc? :) is a privateskipUntilTernaryDelimiter— identical toClickHouseUtils.skipContentsUntil(sql, i, len, '?', ':')for input without a heredoc, plus heredocskipping, so a
:or?inside a heredoc body no longer decides whether a?is a placeholderskipBrackets(identical toClickHouseUtils.skipBracketsotherwise, including the "Missing '<bracket>'" error), because a)or
'inside a heredoc used to end the enclosing bracket or string(
select ?, lower($$it's$$)threwMissing quote: ')ClickHouseUtils, whose scanners arealso used for type-name/value parsing where
$is not a heredoc — no shared behavior is changedCHANGELOG.md— entry under Bug FixesTest
JdbcParameterizedQueryTest.testParseQueriesWithHeredoc(@DataProvider, 25 rows): heredoc bodiescontaining
?,:,;,', comment markers and brackets, tagged/empty/numeric-tag heredocs, aheredoc at position 0, one nested in a function call, and a real ternary whose branch is a heredoc.
Contrast rows pin the unchanged behavior of a
$that does not open a heredoc: identifiers(
a$x$,a$b), an unterminated$$, a heredoc inside a quoted string or a comment, and the plain? :ternary. 14 rows fail onmain.JdbcParameterizedQueryTest.testParseInvalidQueriesWithHeredoc: multi-statement rejection still firesfor a
;outside a heredoc, and unterminated brackets/quotes still raiseIllegalArgumentException(passes both before and after — the new bracket helper preserves it).
ClickHousePreparedStatementTest.testQueryWithHeredocLiteral(integration, live path):select $$a?b$$ as s, ? as nreports one parameter and returnsa?b, 42. Fails onmain(
expected [1] but found [2]).mvn -pl clickhouse-jdbc test→ 114 unit tests green;-Dit.test=ClickHousePreparedStatementTest verify→ 93 integration tests green against ClickHouse 26.5.
Compatibility
No public API change. Behavior changes only for statements containing a heredoc literal, which the
driver previously mis-parsed or rejected. Statements without a heredoc are parsed exactly as before,
including the error messages for unbalanced brackets/quotes and multi-statement input.
docs/changes_checklist.mdno dependency or module changes — the change is confined to private parsing logic in one class.
CHANGELOG.mdupdated with the problem, the fix and the issue link.docs/features.mdis not applicable (this isclickhouse-jdbcv1, notclient-v2/jdbc-v2).Pre-PR validation gate
main)AGENTS.md(targeted Maven runs,@DataProviderinstead ofnear-identical methods, no issue numbers/narrative in test code, negative tests added)
Notes
clickhouse-jdbcis the legacy v1 stack — happy to close this if you would rather not take changes toit.
;inside a heredoc truncates the statement in the parser's error recovery), and
com.clickhouse.client.ClickHouseParameterizedQuery(named-parameter mode,clickhouse-client) has thesame heredoc blindness for
:name. jdbc-v2's equivalent of this fix is Fix jdbc-v2: skip // comments and heredocs when scanning for ? placeholders #3010 (issue [jdbc-v2] PreparedStatement placeholder scan misses // comments and $tag$ heredocs, so a ? inside them is counted as a parameter #3009).