Fix clickhouse-client: skip heredoc literals when extracting named parameters - #3038
Closed
polyglotAI-bot wants to merge 3 commits into
Closed
Fix clickhouse-client: skip heredoc literals when extracting named parameters#3038polyglotAI-bot wants to merge 3 commits into
polyglotAI-bot wants to merge 3 commits into
Conversation
…rameters ClickHouseParameterizedQuery scanned SQL for :name / :name(Type) placeholders while skipping quoted strings and -- / /* */ comments, but had no notion of a heredoc (dollar quoted) literal. A :name inside $$...$$ or $tag$...$tag$ was therefore extracted as a parameter and the literal's text was rewritten on substitution; a ' inside it broke the scan with "Missing quote: '", and a -- or /* inside it swallowed the rest of the statement. Both the instance path (parse) and the static apply(sql, params) path now skip a heredoc as an opaque token. A dollar sign that does not open a heredoc keeps its previous meaning. Fixes: #3037
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
7 tasks
…med-param-heredoc
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…med-param-heredoc
|
Contributor
|
V1 - wont fix because requires parser refactor - will be fixed in V2. |
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 #3037.
com.clickhouse.client.ClickHouseParameterizedQueryextracts named parameters (:name,:name(Type)) by scanning the SQL. The scan skips quoted strings and--//* */comments, but it has no notion of a heredoc (dollar-quoted) literal —$$...$$/$tag$...$tag$. A:nameinside a heredoc was therefore extracted as a parameter and the literal's text was rewritten on substitution (select $$a:b$$, :nwithn=Xbecameselect $$aNULL$$, X), a'inside a heredoc started a quoted-string scan and failed the whole query withMissing quote: ', and a--or/*inside a heredoc started a comment scan that swallowed the rest of the statement (dropping the real parameter, or failing withUnclosed multi-line comment). A heredoc is now skipped as an opaque token, in both scan sites of the class: the instance path (parse(), used byClickHouseRequest.getPreparedQuery(), the v1 JDBC driver withnamedParameter=true, andclickhouse-r2dbc) and the staticapply(sql, params)path.A dollar sign that does not open a heredoc keeps its previous meaning.
$is a valid identifier character, so a$that follows a word character continues an identifier (a$x$, and the existing:a$bparameter name), a tag containing a non-word character is not a heredoc tag, and a$$with no matching closing tag does not open a heredoc.This is the named-parameter sibling of #3036 (
?placeholders inclickhouse-jdbc); the heredoc helper is deliberately kept private to this class rather than added to the sharedClickHouseUtilsscanners, to bound the blast radius (ClickHouseUtilsalso uses$for type-name parsing).Changes
clickhouse-client/.../ClickHouseParameterizedQuery.java: added a privateskipHeredoc(plus anisWordCharhelper) and a$branch in bothparse()and the staticapply(StringBuilder, String, Map)scan loops, so a heredoc is passed through verbatim and its content is never scanned for parameters.CHANGELOG.md: bug-fix entry under0.11.0-rc1.Test
ClickHouseParameterizedQueryTest.testParseQueryWithHeredoc— a TestNG@DataProvidercovering both the parse path and the staticapplypath for each row. Heredoc rows: untagged/tagged/numeric/underscore tags, empty heredoc,$inside the body,::inside the body,--and/*inside the body,'inside the body, heredoc before/after the parameter, inside brackets, back-to-back heredocs, and a heredoc immediately followed by:name. Contrast rows pin the prior behavior for dollar signs that do not open a heredoc (:a$bparameter name, unterminated$$and$tag$, a tag with a non-word character), plus the already-correct quote/comment/ternary/::cases. 13 of the rows fail onmain(wrong parameter list, orMissing quote: '/Unclosed multi-line comment) and all pass with the fix.ClickHousePreparedStatementTest.testQueryWithNamedParameterAndHeredoc(integration) — exercises the live runtime path end-to-end:select $$a:b it's$$ as s, :n(String) as nwithnamedParameter=truereports one parameter and the server returns the heredoc intact. Onmainthe parameter count is 2.mvn -pl clickhouse-client test(158 tests),mvn -pl clickhouse-jdbc test(89 tests), the two named-parameter integration tests, andclickhouse-r2dbccompile. No existing test was edited or weakened.Pre-PR validation gate
main)AGENTS.md: single logical change, targeted module tests,@DataProviderinstead of near-identical methods, no issue references inside test code, integration test on the live path, CHANGELOG updateddocs/features.mdnot applicable (v1 stack, not client-v2/jdbc-v2)Note
clickhouse-clientis the deprecated v1 stack — close this if v1 changes are out of scope for the current line.ClickHouseUtils.skipContentsUntil/skipBracketsinclickhouse-dataremain heredoc-unaware by design here; the(Type)span of:name(Type)still uses the sharedskipBrackets, which only ever spans a ClickHouse type name.