ClickHouse / ClickHouse/clickhouse-cs

InsertBinaryAsync / InsertRawStreamAsync concatenate the raw table name into SQL — names needing backquotes cannot be used

Open
#602 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
94
Forks
22
Avg merge
11h 26m
Merged PRs (30d)
22

Description

## Description

`ClickHouseClient`'s insert paths take a `table` string and splice it straight into the generated SQL, with no identifier quoting. A perfectly legal ClickHouse table name that requires backquotes (`my-table`, `user events`, a name starting with a digit, …) therefore fails with a server-side `SYNTAX_ERROR` even though the table exists — `FROM my-table` parses as `my` minus `table`.

Affected call sites (all on `main`):

- `ClickHouse.Driver/Utility/SchemaResolver.cs:91-92` — the schema probe: `$"SELECT {columnsExpr} FROM {table} WHERE 1=0"`
- `ClickHouse.Driver/ClickHouseClient.cs:691` — `$"INSERT INTO {table} ({string.Join(", ", columnNames)}) FORMAT {options.Format}"`
- `ClickHouse.Driver/ClickHouseClient.cs:899-900` — `InsertRawStreamAsync`: `$"INSERT INTO {table} {columnList} FORMAT {format}"`, and here the *columns* are unquoted too (`string.Join(", ", columns)`), unlike every other insert path

The inconsistency is the tell: column names on the `InsertBinaryAsync` path already go through `StringExtensions.EncloseColumnName()` (`SchemaResolver.cs:88, 94, 123, 174`), and `SchemaResolver.BuildCacheKey` encloses both the database and the table name for the cache key — but the table name that actually reaches the SQL is never enclosed. `EncloseColumnName()` already implements the backward-compatible "pass through if already enclosed" contract, so the helper needed for a fix exists and is used a few lines away.

Practical impact: callers must know to pre-quote the argument themselves (`client.InsertBinaryAsync("\`my-table\`", …)` works), which is undocumented — the XML docs describe the parameter as "The destination table name" / "Table name", not as a SQL fragment.

## ClickHouse server version

`26.8.1.2041` (official build), reached over HTTP at `localhost:8123`.

## Reproduction

NUnit test in `ClickHouse.Driver.Tests` (run with `dotnet test ClickHouse.Driver.Tests/ClickHouse.Driver.Tests.csproj -f net10.0 --filter "FullyQualifiedName~QuotedTableName"`):

```csharp
[TestFixture]
public class QuotedTableNameTests : AbstractConnectionTestFixture
{
private const string BareName = "scratch-quoted-table";

[Test]
public async Task InsertBinaryAsync_TableNameNeedingBackquotes_ShouldWork()
{
await client.ExecuteNonQueryAsync($"DROP TABLE IF EXISTS `{BareName}`");
await client.ExecuteNonQueryAsync(
$"CREATE TABLE `{BareName}` (id UInt64, value String) ENGINE = MergeTree() ORDER BY id");
try
{
var rows = new List { new object[] { 1UL, "a" } };
await client.InsertBinaryAsync(BareName, new[] { "id", "value" }, rows);
var count = await client.ExecuteScalarAsync($"SELECT count() FROM `{BareName}`");
Assert.That(count, Is.EqualTo(1UL));
}
finally
{
await client.ExecuteNonQueryAsync($"DROP TABLE IF EXISTS `{BareName}`");
}
}

// Control: the same insert succeeds when the caller pre-quotes the name.
[Test]
public async Task InsertBinaryAsync_PreQuotedTableName_ShouldWork()
{
await client.ExecuteNonQueryAsync($"DROP TABLE IF EXISTS `{BareName}2`");
await client.ExecuteNonQueryAsync(
$"CREATE TABLE `{BareName}2` (id UInt64, value String) ENGINE = MergeTree() ORDER BY id");
try
{
var rows = new List { new object[] { 1UL, "a" } };
await client.InsertBinaryAsync($"`{BareName}2`", new[] { "id", "value" }, rows);
var count = await client.ExecuteScalarAsync($"SELECT count() FROM `{BareName}2`");
Assert.That(count, Is.EqualTo(1UL));
}
finally
{
await client.ExecuteNonQueryAsync($"DROP TABLE IF EXISTS `{BareName}2`");
}
}
}
```

**Expected:** both tests pass — one row inserted into the existing table.

**Actual:** `Failed: 1, Passed: 1`. The pre-quoted control passes; the bare-name test throws from the schema probe before any data is sent:

```
ClickHouse.Driver.ClickHouseServerException : Code: 62. DB::Exception: Syntax error:
failed at position 33 (-): -quoted-table WHERE 1=0. Expected one of: ... (SYNTAX_ERROR)
at ClickHouse.Driver.Utility.SchemaResolver.LoadAsync(...) SchemaResolver.cs:line 92
at ClickHouse.Driver.Utility.SchemaResolver.ResolveAsync(...) SchemaResolver.cs:line 79
at ClickHouse.Driver.ClickHouseClient.PrepareInsertAsync(...) ClickHouseClient.cs:line 682
at ClickHouse.Driver.ClickHouseClient.InsertBinaryAsync(...) ClickHouseClient.cs:line 835
```

Supplying `InsertOptions.ColumnTypes` skips the probe, but the insert then fails identically on `INSERT INTO scratch-quoted-table (…)` built at `ClickHouseClient.cs:691`.

## Suggested fix

Settle the contract for the `table` parameter, mirroring what the upstream Java issue asks for:

1. **Treat it as a raw identifier** (what the docs imply): run it through `EncloseColumnName()` at the three sites above. That helper's already-enclosed pass-through preserves today's pre-quoting workaround, so existing callers keep working. A dotted `db.table` argument is the wrinkle to decide on — `SchemaResolver.BuildCacheKey` and the `InsertBinarySchemaTests` comments show qualified names are an accepted input shape, and blanket-enclosing would turn `db.table` into `` `db.table` ``; splitting on an unquoted dot, or enclosing each part, would be needed. This is also worth deciding for `InsertOptions.Database`.
2. **Or document it as a SQL fragment** and state in the XML docs on `InsertBinaryAsync`, `InsertRawStreamAsync` and `InsertOptions.Database` that the caller must quote names that need it.

Either way, `InsertRawStreamAsync`'s unquoted *column* list (`ClickHouseClient.cs:899`) looks like an oversight relative to the other insert paths and should use `EncloseColumnName()`.

Note that #316 (`EncloseColumnName` skips escaping when input starts and ends with the quote char) interacts with option 1: a name like `` `weird` `` that legitimately begins and ends with a backtick is passed through unescaped. That is a separate defect, but a fix here would inherit it.

## Link

Same bug reported upstream for clickhouse-java client-v2: https://github.com/ClickHouse/clickhouse-java/issues/3089

Contributor guide

Open the contributing guide

Research direction

Start by running the named QuotedTableName test filter, then read SchemaResolver.cs around lines 79-94 and ClickHouseClient.cs around lines 682-691 and 899-900. Compare the existing EncloseColumnName uses and the qualified-name comments in InsertBinarySchemaTests; done means the bare and pre-quoted table tests pass and the raw-stream column handling is covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp, sql
Domain
backend-api-design, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.