Skip to content

fix: query parameter detection in quoted SQL - #1938

Open
fallintoplace wants to merge 2 commits into
ClickHouse:mainfrom
fallintoplace:ignore-quoted-query-parameter-syntax
Open

fix: query parameter detection in quoted SQL#1938
fallintoplace wants to merge 2 commits into
ClickHouse:mainfrom
fallintoplace:ignore-quoted-query-parameter-syntax

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

Server-side query parameter detection currently applies a regular expression to the complete SQL query. Parameter-like text inside a string, quoted identifier, or comment can therefore select the native parameter path and reject ordinary positional arguments with "unsupported query parameter type".

Detect parameter syntax only outside quoted and commented contexts by reusing the existing binding scanner. The detection also requires a nonempty parameter name and type. Regression tests cover strings, quoted identifiers, line and block comments, and a real native parameter with a quoted Enum type.

Checklist

  • Unit and integration tests covering the common scenarios were added

Testing

  • go test ./...

Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Comment thread query_parameters.go

func isQueryParameter(query string) bool {
nameEnd := 0
for nameEnd < len(query) && isNameChar(query[nameEnd]) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Should fix — name scan rejects whitespace ClickHouse accepts, and accepts digit-leading names it doesn't

ClickHouse parses {name:Type} from tokens with whitespace already dropped, so SELECT { p : String } is valid SQL — it used to match hasQueryParamsRe, but this scan requires the name to sit flush against { and :, so such a query now falls through to bind, which leaves the braces in the text and sends no parameters. The server then fails with Substitution 'p' is not set.

  • The same scan accepts a digit-leading name, so a map literal such as SELECT {1:'a'}, ? still takes the parameter path and reports unsupported query parameter type — ClickHouse lexes a leading digit as a number, never as a substitution name. Pre-existing, but cheap to close here.
-	nameEnd := 0
-	for nameEnd < len(query) && isNameChar(query[nameEnd]) {
+	nameStart := skipSpace(query, 0) // small helper: advances over ' ', '\t', '\r', '\n'
+	if nameStart >= len(query) || isDigit(query[nameStart]) {
+		return false
+	}
+	nameEnd := nameStart
+	for nameEnd < len(query) && isNameChar(query[nameEnd]) {
 		nameEnd++
 	}
-	if nameEnd == 0 || nameEnd >= len(query) || query[nameEnd] != ':' {
+	colon := skipSpace(query, nameEnd)
+	if nameEnd == nameStart || colon >= len(query) || query[colon] != ':' {
 		return false
 	}

The type scan then starts at colon+1 and its guard becomes i > colon+1. Two table cases would lock this down: { p : String } detected, {1:'a'} not.

Comment thread query_parameters.go
return bind(timezone, query, args...)
}

func hasQueryParameters(query string) bool {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Nit — nit: document the two new scanner helpers

bind.go documents every scanner helper it owns (bindQuoteState, inProtectedContext, isNameChar) and the rest of this file carries dense rationale comments; these two functions carry none. One line each — what shape counts as a parameter, and why quoted/commented regions are skipped — keeps the invariant discoverable for whoever next touches the scanner.

@github-actions

Copy link
Copy Markdown

🤖 Claude review

Replaces the {.+:.+} regex that decides whether a query takes the server-side parameter path with a scan that reuses bind.go's quote/comment state machine, so parameter-like text inside string literals, quoted identifiers, and comments no longer hijacks ordinary positional args. The direction is right, the reuse of bindQuoteState is the correct call, and because every caller (conn_query.go, conn_exec.go, conn_http_query.go, conn_http_exec.go, conn_http_format.go, both async-insert paths) funnels through bindQueryOrAppendParameters, the fix lands on the native, HTTP, and database/sql surfaces at once.

The new detector is strictly narrower than the old regex in every direction except multi-line types, so the risk in this PR is false negatives, not false positives.

Key concern:

  • The name scan is stricter than ClickHouse's own parser: { p : String } is valid server-side SQL that used to match the regex and now silently falls through to client-side bind, leaving the braces in the query with no parameters attached (see inline on query_parameters.go).

Blind spots:

  • No live server available in this review, so the whitespace claim rests on reading ClickHouse's token-based ParserSubstitution (whitespace tokens are dropped before parsing), not on a round trip. Worth confirming with SELECT { p : String } + param_p before acting on it.
  • Coverage is unit-level in the root package only. That is defensible — the change is protocol-agnostic query-text handling — but nothing exercises the fixed path end to end, and there is no tests/issues/issue_<N>_test.go regression test or linked issue as the repo workflow prescribes.
  • Residual false positives outside quotes (e.g. SELECT {1:'a'}) are unchanged from the old regex; noted inline as a cheap tightening rather than a defect this PR introduced.

Verdict: ⚠️ Request changes

Inline comments are attached to the relevant lines. This summary updates in place on re-review.

@fallintoplace fallintoplace changed the title Fix query parameter detection in quoted SQL fix: query parameter detection in quoted SQL Jul 31, 2026
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants