Fix inserts into tables whose name requires quoting (#602)

ClickHouse.Driver.Tests/Copy/InsertBinaryRequestBufferingTests.cs

@@ -182,7 +182,8 @@ public void InsertBinaryAsync_WhenSerializationFailsAndTheFlushAlsoFails_KeepsTh
     private static byte[] ExpectedPayload(int rows)
     {
         using var expected = new MemoryStream();
-        expected.Write(Encoding.UTF8.GetBytes($"INSERT INTO {FakeEndpointTable} (`Id`) FORMAT RowBinary\n"));
+        expected.Write(Encoding.UTF8.GetBytes(
+            $"INSERT INTO `{TestUtilities.TestDatabase}`.`{TestUtilities.BareTableName(FakeEndpointTable)}` (`Id`) FORMAT RowBinary\n"));
 
         // RowBinary encodes UInt64 little-endian on every platform, so the expectation is spelled out
         // explicitly rather than inherited from the host's byte order. The scratch buffer is reused

ClickHouse.Driver.Tests/InsertBinaryQueryPlacementTests.cs

@@ -145,7 +145,8 @@ await stubbedClient.InsertBinaryAsync(
                 options);
         }
 
-        var expectedStatement = $"INSERT INTO {table} (`Id`, `Value`) FORMAT RowBinary";
+        var expectedStatement =
+            $"INSERT INTO `{TestUtilities.TestDatabase}`.`{TestUtilities.BareTableName(table)}` (`Id`, `Value`) FORMAT RowBinary";
         var urlQuery = HttpUtility.ParseQueryString(sentUri.Query).Get("query");
         var body = Decoded(sentBody, compressor);
 

ClickHouse.Driver.Tests/SQL/QuotedIdentifierInsertTests.cs

@@ -0,0 +1,158 @@
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.IO;
+using System.Text;
+using System.Threading.Tasks;
+using ClickHouse.Driver.Tests.Utilities;
+using NUnit.Framework;
+
+namespace ClickHouse.Driver.Tests.SQL;
+
+/// <summary>
+/// Inserts into tables and columns whose names are legal ClickHouse identifiers but are not legal
+/// unquoted ones, so they only reach the server correctly if the insert paths quote them.
+/// </summary>
+[TestFixture]
+public class QuotedIdentifierInsertTests : AbstractConnectionTestFixture
+{
+    /// <summary>Quoted names this fixture created, dropped on teardown.</summary>
+    private readonly ConcurrentQueue<string> quotedTables = new();
+
+    /// <summary>
+    /// Name shapes that require quoting. <c>{0}</c> is replaced with a unique token, so a shape can
+    /// place the offending characters anywhere in the name.
+    /// </summary>
+    private static IEnumerable<TestCaseData> NameShapesRequiringQuoting()
+    {
+        yield return new TestCaseData("{0}-hyphen").SetName("{m}(hyphen)");
+        yield return new TestCaseData("{0} space").SetName("{m}(space)");
+        yield return new TestCaseData("{0}`backtick").SetName("{m}(backtick)");
+    }
+
+    private static string Quote(string identifier) => $"`{identifier.Replace("`", "\\`")}`";
+
+    /// <summary>
+    /// Creates a table whose name matches <paramref name="nameShape"/>.
+    /// </summary>
+    /// <returns>
+    /// The database-qualified name as a caller passes it to the client (unquoted), and the quoted
+    /// form for use in DDL and read-back queries.
+    /// </returns>
+    private async Task<(string table, string quoted)> CreateTableRequiringQuotingAsync(
+        string nameShape, string columnDefinitions = "id UInt64, value String")
+    {
+        var unique = TestUtilities.BareTableName(TestUtilities.CreateTableName("quoted_identifier"));
+        var bare = string.Format(nameShape, unique);
+        var quoted = $"{Quote(TestUtilities.TestDatabase)}.{Quote(bare)}";
+
+        await client.ExecuteNonQueryAsync(
+            $"CREATE TABLE {quoted} ({columnDefinitions}) ENGINE = MergeTree() ORDER BY id");
+        quotedTables.Enqueue(quoted);
+
+        return ($"{TestUtilities.TestDatabase}.{bare}", quoted);
+    }
+
+    [OneTimeTearDown]
+    public void DropQuotedTables()
+    {
+        while (quotedTables.TryDequeue(out var quoted))
+        {
+            try
+            {
+                client.ExecuteNonQueryAsync($"DROP TABLE IF EXISTS {quoted}").GetAwaiter().GetResult();
+            }
+            catch (Exception e)
+            {
+                TestContext.Progress.WriteLine($"Failed to drop test table {quoted}: {e.Message}");
+            }
+        }
+    }
+
+    [TestCaseSource(nameof(NameShapesRequiringQuoting))]
+    public async Task InsertBinaryAsync_TableNameRequiringQuoting_ShouldInsertRows(string nameShape)
+    {
+        var (table, quoted) = await CreateTableRequiringQuotingAsync(nameShape);
+
+        await client.InsertBinaryAsync(table, new[] { "id", "value" }, new[] { new object[] { 1UL, "a" } });
+
+        Assert.That(await client.ExecuteScalarAsync($"SELECT value FROM {quoted}"), Is.EqualTo("a"));
+    }
+
+    /// <summary>
+    /// A caller that worked around the missing quoting by pre-quoting the name must keep working:
+    /// the name is quoted once, not twice.
+    /// </summary>
+    [Test]
+    public async Task InsertBinaryAsync_PreQuotedTableName_ShouldInsertRows()
+    {
+        var (_, quoted) = await CreateTableRequiringQuotingAsync("{0}-hyphen");
+
+        await client.InsertBinaryAsync(quoted, new[] { "id", "value" }, new[] { new object[] { 1UL, "a" } });
+
+        Assert.That(await client.ExecuteScalarAsync($"SELECT value FROM {quoted}"), Is.EqualTo("a"));
+    }
+
+    /// <summary>
+    /// A caller that worked around the missing quoting with the double quotes ClickHouse also accepts
+    /// around an identifier must keep working.
+    /// </summary>
+    [Test]
+    public async Task InsertBinaryAsync_TableNameInDoubleQuotes_ShouldInsertRows()
+    {
+        var (table, quoted) = await CreateTableRequiringQuotingAsync("{0}-hyphen");
+        var bare = TestUtilities.BareTableName(table);
+
+        await client.InsertBinaryAsync(
+            $"\"{TestUtilities.TestDatabase}\".\"{bare}\"",
+            new[] { "id", "value" },
+            new[] { new object[] { 1UL, "a" } });
+
+        Assert.That(await client.ExecuteScalarAsync($"SELECT value FROM {quoted}"), Is.EqualTo("a"));
+    }
+
+    /// <summary>
+    /// With <see cref="InsertOptions.ColumnTypes"/> there is no schema probe, so the INSERT statement
+    /// is the only place the table name is used.
+    /// </summary>
+    [Test]
+    public async Task InsertBinaryAsync_TableNameRequiringQuoting_WithColumnTypes_ShouldInsertRows()
+    {
+        var (table, quoted) = await CreateTableRequiringQuotingAsync("{0}-hyphen");
+        var options = new InsertOptions
+        {
+            ColumnTypes = new Dictionary<string, string> { ["id"] = "UInt64", ["value"] = "String" },
+        };
+
+        await client.InsertBinaryAsync(
+            table, new[] { "id", "value" }, new[] { new object[] { 1UL, "a" } }, options);
+
+        Assert.That(await client.ExecuteScalarAsync($"SELECT value FROM {quoted}"), Is.EqualTo("a"));
+    }
+
+    [Test]
+    public async Task InsertRawStreamAsync_TableNameRequiringQuoting_ShouldInsertRows()
+    {
+        var (table, quoted) = await CreateTableRequiringQuotingAsync("{0}-hyphen");
+
+        using var stream = new MemoryStream(Encoding.UTF8.GetBytes("1\ta\n"));
+        using var response = await client.InsertRawStreamAsync(table, stream, "TSV");
+
+        Assert.That(response.IsSuccessStatusCode, Is.True);
+        Assert.That(await client.ExecuteScalarAsync($"SELECT value FROM {quoted}"), Is.EqualTo("a"));
+    }
+
+    [Test]
+    public async Task InsertRawStreamAsync_ColumnNameRequiringQuoting_ShouldInsertRows()
+    {
+        var (table, quoted) = await CreateTableRequiringQuotingAsync(
+            "{0}-hyphen", "id UInt64, `value-column` String");
+
+        using var stream = new MemoryStream(Encoding.UTF8.GetBytes("1\ta\n"));
+        using var response = await client.InsertRawStreamAsync(
+            table, stream, "TSV", columns: new[] { "id", "value-column" });
+
+        Assert.That(response.IsSuccessStatusCode, Is.True);
+        Assert.That(await client.ExecuteScalarAsync($"SELECT `value-column` FROM {quoted}"), Is.EqualTo("a"));
+    }
+}

ClickHouse.Driver.Tests/Utility/StringExtensionsTests.cs

@@ -0,0 +1,42 @@
+using ClickHouse.Driver.Utility;
+using NUnit.Framework;
+
+namespace ClickHouse.Driver.Tests.Utility;
+
+public class StringExtensionsTests
+{
+    [TestCase("table", ExpectedResult = "`table`")]
+    [TestCase("my-table", ExpectedResult = "`my-table`")]
+    [TestCase("db.table", ExpectedResult = "`db`.`table`")]
+    [TestCase("db.my-table", ExpectedResult = "`db`.`my-table`")]
+    [TestCase("my-db.my-table", ExpectedResult = "`my-db`.`my-table`")]
+    [TestCase("`my-table`", ExpectedResult = "`my-table`")]
+    [TestCase("`db`.`table`", ExpectedResult = "`db`.`table`")]
+    [TestCase("db.`my-table`", ExpectedResult = "`db`.`my-table`")]
+    [TestCase("`my.table`", ExpectedResult = "`my.table`")]
+    [TestCase("`db`.`my.table`", ExpectedResult = "`db`.`my.table`")]
+    [TestCase("db.back`tick", ExpectedResult = "`db`.`back\\`tick`")]
+    [TestCase("`back\\`tick`.table", ExpectedResult = "`back\\`tick`.`table`")]
+    [TestCase("\"my-table\"", ExpectedResult = "\"my-table\"")]
+    [TestCase("db.\"my.table\"", ExpectedResult = "`db`.\"my.table\"")]
+    [TestCase("", ExpectedResult = "")]
+    [TestCase(null, ExpectedResult = null)]
+    public string EncloseQualifiedName_Name_ShouldEncloseEveryPartOnce(string name) =>
+        name.EncloseQualifiedName();
+
+    /// <summary>
+    /// The three insert paths enclose independently, and the schema cache key is built from the same
+    /// name, so enclosing an enclosed name must not add another layer of quoting.
+    /// </summary>
+    [TestCase("table")]
+    [TestCase("my-table")]
+    [TestCase("db.my-table")]
+    [TestCase("`db`.`my.table`")]
+    [TestCase("\"my-table\"")]
+    public void EncloseQualifiedName_AlreadyEnclosedName_ShouldBeIdempotent(string name)
+    {
+        var enclosed = name.EncloseQualifiedName();
+
+        Assert.That(enclosed.EncloseQualifiedName(), Is.EqualTo(enclosed));
+    }
+}

ClickHouse.Driver/ClickHouseClient.cs

@@ -688,7 +688,7 @@ private async Task<InsertPlan> PrepareInsertAsync(
             logger.LogDebug("Metadata loaded for table {Table}. Columns: {Columns}.", table, string.Join(", ", columnNames ?? Array.Empty<string>()));
         }
 
-        var query = $"INSERT INTO {table} ({string.Join(", ", columnNames)}) FORMAT {options.Format.ToString()}";
+        var query = $"INSERT INTO {table.EncloseQualifiedName()} ({string.Join(", ", columnNames)}) FORMAT {options.Format.ToString()}";
         var baseQueryId = options.QueryId ?? Guid.NewGuid().ToString();
 
         return new InsertPlan
@@ -896,8 +896,8 @@ public async Task<HttpResponseMessage> InsertRawStreamAsync(
         if (string.IsNullOrEmpty(format))
             throw new ArgumentException("Format cannot be null or empty", nameof(format));
 
-        var columnList = columns != null ? $"({string.Join(", ", columns)})" : string.Empty;
-        var query = $"INSERT INTO {table} {columnList} FORMAT {format}";
+        var columnList = columns != null ? $"({string.Join(", ", columns.Select(c => c.EncloseColumnName()))})" : string.Empty;
+        var query = $"INSERT INTO {table.EncloseQualifiedName()} {columnList} FORMAT {format}";
 
         // The request message that carries this content disposes it - and, through StreamContent,
         // the supplied stream - once the request completes, on success and on failure alike.

ClickHouse.Driver/Utility/SchemaResolver.cs

@@ -90,7 +90,7 @@ internal SchemaResolver(ClickHouseClient client)
             ? "*"
             : string.Join(",", columns.Select(c => c.EncloseColumnName()));
         using var reader = (ClickHouseDataReader)await client.ExecuteReaderAsync(
-            $"SELECT {columnsExpr} FROM {table} WHERE 1=0", null, options).ConfigureAwait(false);
+            $"SELECT {columnsExpr} FROM {table.EncloseQualifiedName()} WHERE 1=0", null, options).ConfigureAwait(false);
         var types = reader.GetClickHouseColumnTypes();
         var names = reader.GetColumnNames().Select(c => c.EncloseColumnName()).ToArray();
         return (names, types);
@@ -137,7 +137,7 @@ private string BuildCacheKey(string table, InsertOptions options)
         var database = options.Database ?? client.Settings.Database ?? string.Empty;
 
         var enclosedDatabase = database.EncloseColumnName();
-        var enclosedTable = table.EncloseColumnName();
+        var enclosedTable = table.EncloseQualifiedName();
         return $"{enclosedDatabase}.{enclosedTable}";
     }
 

ClickHouse.Driver/Utility/StringExtensions.cs

@@ -44,6 +44,76 @@ public static string EncloseColumnName(this string str)
         return builder.ToString();
     }
 
+    /// <summary>
+    /// Encloses a possibly database-qualified name (<c>database.table</c>) in backticks, one part at a
+    /// time, so that a part which is not a legal unquoted identifier still reaches the server as a
+    /// single identifier. Parts that are already enclosed are left as they are, which keeps a name
+    /// that a caller pre-quoted from being quoted twice.
+    /// </summary>
+    /// <param name="str">Possibly qualified, possibly already enclosed name</param>
+    /// <returns>Name with every part enclosed</returns>
+    public static string EncloseQualifiedName(this string str)
+    {
+        if (string.IsNullOrEmpty(str) || str.IndexOf('.') < 0)
+            return EnclosePart(str);
+
+        var builder = new StringBuilder(str.Length + 4);
+        var position = 0;
+        while (true)
+        {
+            var end = FindPartEnd(str, position);
+            if (builder.Length > 0)
+                builder.Append('.');
+            builder.Append(EnclosePart(str[position..end]));
+            if (end >= str.Length)
+                return builder.ToString();
+            position = end + 1; // Skip the separator
+        }
+    }
+
+    /// <summary>
+    /// Encloses one part of a qualified name. A part the caller already enclosed is left as it is —
+    /// in backticks, or in the double quotes ClickHouse equally accepts around an identifier, which
+    /// is how a caller can have worked around the name not being enclosed at all.
+    /// </summary>
+    private static string EnclosePart(string part) =>
+        part != null && part.Length >= 2 && part[0] == '"' && part[part.Length - 1] == '"'
+            ? part
+            : part.EncloseColumnName();
+
+    /// <summary>
+    /// Returns the index of the separator that ends the part starting at <paramref name="start"/>, or
+    /// the length of <paramref name="str"/> for the last part. A separator inside an enclosed part
+    /// belongs to the name, so it does not end the part.
+    /// </summary>
+    private static int FindPartEnd(string str, int start)
+    {
+        var i = start;
+        if (i < str.Length && (str[i] == '`' || str[i] == '"'))
+        {
+            var quote = str[i];
+            for (i++; i < str.Length; i++)
+            {
+                if (str[i] == '\\')
+                {
+                    i++; // The escaped character cannot close the part
+                    continue;
+                }
+
+                if (str[i] == quote)
+                {
+                    i++;
+                    break;
+                }
+            }
+        }
+
+        while (i < str.Length && str[i] != '.')
+            i++;
+
+        return i;
+    }
+
     /// <summary>
     /// Removes the enclosing backticks (`) from an identifier and unescapes the escape sequences
     /// ClickHouse uses inside a quoted identifier. Does nothing if the identifier is not enclosed.

changelog.d/602-quote-insert-table-name.fixes.md

@@ -0,0 +1,5 @@
+* Fixed inserts into tables whose name is not a legal unquoted identifier (`my-table`, `user events`).
+  `InsertBinaryAsync` and `InsertRawStreamAsync` now enclose the table name — and, for
+  `InsertRawStreamAsync`, the column names — in backticks instead of concatenating them into the
+  statement, so such a table no longer fails with a `SYNTAX_ERROR`. A name a caller already quoted is
+  left as it is. ([#602](https://github.com/ClickHouse/clickhouse-cs/issues/602))

docs/overview.mdx

@@ -439,6 +439,7 @@ var options = new InsertOptions
 * The client automatically fetches table structure via `SELECT * FROM <table> WHERE 1=0` before inserting. Provided values must match the target column types. To skip this query, use [`InsertOptions.ColumnTypes` or `InsertOptions.UseSchemaCache`](#skip-schema-query).
 * When `MaxDegreeOfParallelism > 1`, batches are uploaded in parallel. Sessions are not compatible with parallel insertion; either disable sessions or set `MaxDegreeOfParallelism = 1`.
 * Use `RowBinaryFormat.RowBinaryWithDefaults` in `InsertOptions.Format` if you want the server to apply DEFAULT values for columns not provided.
+* The table name is a name, not a SQL fragment. Each part of `database.table` is enclosed in backticks, so a name that is not a legal unquoted identifier (`my-table`, `user events`) works as it is. A part you enclosed yourself is left as it is.
 </Note>
 
 #### POCO inserts {#poco-insert}
@@ -1122,7 +1123,8 @@ using var response = await client.InsertRawStreamAsync(
 ```
 
 <Note>
-See the [format settings documentation](/reference/settings/formats) for options to control data ingestion behavior.
+* See the [format settings documentation](/reference/settings/formats) for options to control data ingestion behavior.
+* The table and column names are names, not SQL fragments: they are enclosed in backticks, so names that are not legal unquoted identifiers work as they are.
 </Note>
 
 ---