Skip to content

fix: escape control characters in native TCP query parameters#1834

Open
Laotree wants to merge 4 commits intoClickHouse:mainfrom
Laotree:fix-1792-query-param-control-chars
Open

fix: escape control characters in native TCP query parameters#1834
Laotree wants to merge 4 commits intoClickHouse:mainfrom
Laotree:fix-1792-query-param-control-chars

Conversation

@Laotree
Copy link
Copy Markdown
Contributor

@Laotree Laotree commented Apr 17, 2026

String parameters containing tab, newline, CR, backslash, or NUL were silently truncated when sent via the native TCP protocol. The server decodes parameter values through two stages — readQuoted then deserializeTextEscaped — so control characters must be double-encoded by the client. Replace the single-quote-only ReplaceAll in encodeFieldDump with a strings.Replacer that handles all special characters correctly.

Fixes #1792

Summary

Checklist

Delete items not relevant to your PR:

String parameters containing tab, newline, CR, backslash, or NUL were
silently truncated when sent via the native TCP protocol. The server
decodes parameter values through two stages — readQuoted then
deserializeTextEscaped — so control characters must be double-encoded
by the client. Replace the single-quote-only ReplaceAll in
encodeFieldDump with a strings.Replacer that handles all special
characters correctly.

Fixes ClickHouse#1792

Signed-off-by: rui cheng <chengrui0428@gmail.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Laotree

This comment was marked as outdated.

@github-actions
Copy link
Copy Markdown

Summary

This PR fixes a real bug: string query parameters containing tab, newline, CR, backslash, or NUL were silently truncated when sent over the native TCP protocol because the server runs parameter values through two decode stages (readQuoted then deserializeTextEscaped), but the client only escaped single quotes. The fix is correct — replacing the single strings.ReplaceAll with a package-level strings.Replacer that double-encodes all special characters. The encoding logic and its comments are accurate and clear, the unit tests are table-driven and well-constructed, and the README addition is a welcome improvement. The approach is sound.

Should fix

  1. No HTTP protocol integration test. The review checklist requires coverage for both TCP and HTTP. The HTTP path encodes parameters as URL query params (a separate code path untouched by this PR), so the bug never affected it — but a test confirming special characters round-trip correctly over HTTP would catch any future regression and satisfy the checklist. Consider adding an HTTP variant in tests/issues/1792_test.go using GetConnectionHTTP (or the equivalent) with the same case table.

  2. No database/sql (std API) integration test. The checklist requires coverage for both the native Open() API and the database/sql OpenDB() API. Adding a Test1792Std that exercises sql.Named parameters with the same special characters via the std shim would complete the coverage.

Nits

  • nit: t.Skip(fmt.Errorf("unsupported clickhouse version")) wraps a string in an error solely to pass it to t.Skip, which accepts ...any. Prefer t.Skipf("unsupported clickhouse version") — it reads more naturally and matches the pattern used elsewhere in the test suite.
  • nit: The return immediately after t.Skip(...) is unreachable — t.Skip calls runtime.Goexit() internally. It's harmless but can be dropped.

Verdict

Request changes — the fix and unit tests are correct, but HTTP and std API integration test coverage is missing per the project checklist.

Laotree and others added 3 commits April 20, 2026 21:56
- Replace t.Skip(fmt.Errorf(...)) with t.Skipf(...) and remove unreachable return
- Extract shared test cases into controlCharCases to avoid duplication
- Add Test1792HTTP: round-trip control characters via HTTP protocol
- Add Test1792Std: round-trip control characters via database/sql with clickhouse.Named()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
String parameters containing tab, newline, CR, backslash, or NUL were
misinterpreted when sent via the HTTP protocol. The server applies
deserializeTextEscaped (TSV format) to param_<name> values, so raw
control characters act as field/record delimiters and cause parse errors,
and a literal backslash+t is read as a tab. Apply TSV escaping via
httpQueryParamReplacer before adding values to the URL query string.

Signed-off-by: rui cheng <chengrui0428@gmail.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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.

Doc/Bug: Query params behavior in clickhouse-go

2 participants