ClickHouse / ClickHouse/ch-go

ch.Parameters does not escape quotes/backslashes in string values

Open Beginner friendly
#1,198 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
428
Forks
84
Avg merge
9d 6h
Merged PRs (30d)
4

Description

## Summary

`ch.Parameters` formats each value with `fmt.Sprintf("'%v'", v)` and sends it as a custom setting / query parameter. That wraps the value in single quotes but does **not** escape `'` or `\` inside the value, so a string like `it's` becomes `'it's'` on the wire and can break the server's quoted-value reader (`readQuoted` / Field dump restore).

This is related to, but **not the same as**, [clickhouse-go#1834](https://github.com/ClickHouse/clickhouse-go/pull/1834) / [#1792](https://github.com/ClickHouse/clickhouse-go/issues/1792).

## Current behavior

```go
// query_params.go
Value: fmt.Sprintf("'%v'", v),
```

`proto.Parameter.Encode` then `PutString`s that text as-is. There is no Field-dump escaper.

Documented / tested contract today is that callers supply **pre-escaped** Escaped-format text (e.g. `` `line 1\\nline 2` `` for a newline), and raw tab/newline are expected to be rejected by the server. That contract can stay.

## Gap

Even under the pre-escaped contract, values that contain a literal `'` or `\` need escaping for the outer quoted dump. The helper does not do that:

| Go value | Wire today | Problem |
|---|---|---|
| `it's` | `'it's'` | premature end of quoted string |
| `a\b` | `'a\b'` | bare `\` starts an escape sequence in `readQuoted` |

Callers can work around this by building `[]proto.Parameter` by hand with correct quotes/escapes, but the helper should be safe for ordinary strings.

## Suggested fix (minimal)

Escape `\` and `'` when building the quoted value in `ch.Parameters` (inverse of the server's quoted-string reader), e.g. `it's` → `'it\'s'`, `a\b` → `'a\\b'`. Keep the existing pre-escaped Escaped-format contract for `\n` / `\t` / etc. unless maintainers want a separate literal-value API.

## Optional follow-up (design)

Whether to also auto-escape raw control characters (`\t`, `\n`, `\r`, `\0`) so ordinary Go strings round-trip, similar to clickhouse-go's `Named` path after #1834 — or keep rejecting them and document manual escaping as today.

## References

- clickhouse-go PR: https://github.com/ClickHouse/clickhouse-go/pull/1834
- clickhouse-go issue: https://github.com/ClickHouse/clickhouse-go/issues/1792
- Discussion on that PR about fixing ch-go first: this path is independent; clickhouse-go does not use `ch.Parameters` / ch-go's `Query` encoder for its `Named` / HTTP parameter binding.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in query_params.go at the fmt.Sprintf("'%v'", v) construction and inspect proto.Parameter.Encode, then compare it with the server's readQuoted / Field dump restore behavior. Add coverage for literal quotes and backslashes while preserving the existing pre-escaped \n and \t contract; done means those values round-trip without breaking quoted parsing.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.