diff --git a/ClickHouse.Driver.Tests/Copy/InsertBinaryRequestBufferingTests.cs b/ClickHouse.Driver.Tests/Copy/InsertBinaryRequestBufferingTests.cs index 87b44205d..5c0de82c3 100644 --- a/ClickHouse.Driver.Tests/Copy/InsertBinaryRequestBufferingTests.cs +++ b/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 diff --git a/ClickHouse.Driver.Tests/InsertBinaryQueryPlacementTests.cs b/ClickHouse.Driver.Tests/InsertBinaryQueryPlacementTests.cs index a1f14eeef..b10212c93 100644 --- a/ClickHouse.Driver.Tests/InsertBinaryQueryPlacementTests.cs +++ b/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); diff --git a/ClickHouse.Driver.Tests/SQL/QuotedIdentifierInsertTests.cs b/ClickHouse.Driver.Tests/SQL/QuotedIdentifierInsertTests.cs new file mode 100644 index 000000000..db3205619 --- /dev/null +++ b/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; + +/// +/// 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. +/// +[TestFixture] +public class QuotedIdentifierInsertTests : AbstractConnectionTestFixture +{ + /// Quoted names this fixture created, dropped on teardown. + private readonly ConcurrentQueue quotedTables = new(); + + /// + /// Name shapes that require quoting. {0} is replaced with a unique token, so a shape can + /// place the offending characters anywhere in the name. + /// + private static IEnumerable 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("`", "\\`")}`"; + + /// + /// Creates a table whose name matches . + /// + /// + /// 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. + /// + 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")); + } + + /// + /// A caller that worked around the missing quoting by pre-quoting the name must keep working: + /// the name is quoted once, not twice. + /// + [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")); + } + + /// + /// A caller that worked around the missing quoting with the double quotes ClickHouse also accepts + /// around an identifier must keep working. + /// + [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")); + } + + /// + /// With there is no schema probe, so the INSERT statement + /// is the only place the table name is used. + /// + [Test] + public async Task InsertBinaryAsync_TableNameRequiringQuoting_WithColumnTypes_ShouldInsertRows() + { + var (table, quoted) = await CreateTableRequiringQuotingAsync("{0}-hyphen"); + var options = new InsertOptions + { + ColumnTypes = new Dictionary { ["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")); + } +} diff --git a/ClickHouse.Driver.Tests/Utility/StringExtensionsTests.cs b/ClickHouse.Driver.Tests/Utility/StringExtensionsTests.cs new file mode 100644 index 000000000..10bdeccad --- /dev/null +++ b/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(); + + /// + /// 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. + /// + [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)); + } +} diff --git a/ClickHouse.Driver/ClickHouseClient.cs b/ClickHouse.Driver/ClickHouseClient.cs index e3591eecf..17e5bf71c 100644 --- a/ClickHouse.Driver/ClickHouseClient.cs +++ b/ClickHouse.Driver/ClickHouseClient.cs @@ -688,7 +688,7 @@ private async Task PrepareInsertAsync( logger.LogDebug("Metadata loaded for table {Table}. Columns: {Columns}.", table, string.Join(", ", columnNames ?? Array.Empty())); } - 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 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. diff --git a/ClickHouse.Driver/Utility/SchemaResolver.cs b/ClickHouse.Driver/Utility/SchemaResolver.cs index fa9445ef2..811f6bd48 100644 --- a/ClickHouse.Driver/Utility/SchemaResolver.cs +++ b/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}"; } diff --git a/ClickHouse.Driver/Utility/StringExtensions.cs b/ClickHouse.Driver/Utility/StringExtensions.cs index 7d4514e04..4e602310d 100644 --- a/ClickHouse.Driver/Utility/StringExtensions.cs +++ b/ClickHouse.Driver/Utility/StringExtensions.cs @@ -44,6 +44,76 @@ public static string EncloseColumnName(this string str) return builder.ToString(); } + /// + /// Encloses a possibly database-qualified name (database.table) 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. + /// + /// Possibly qualified, possibly already enclosed name + /// Name with every part enclosed + 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 + } + } + + /// + /// 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. + /// + private static string EnclosePart(string part) => + part != null && part.Length >= 2 && part[0] == '"' && part[part.Length - 1] == '"' + ? part + : part.EncloseColumnName(); + + /// + /// Returns the index of the separator that ends the part starting at , or + /// the length of for the last part. A separator inside an enclosed part + /// belongs to the name, so it does not end the part. + /// + 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; + } + /// /// 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. diff --git a/changelog.d/602-quote-insert-table-name.fixes.md b/changelog.d/602-quote-insert-table-name.fixes.md new file mode 100644 index 000000000..1bd186c9b --- /dev/null +++ b/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)) diff --git a/docs/http.mdx b/docs/http.mdx index 72b0d8b6d..f6cca2115 100644 --- a/docs/http.mdx +++ b/docs/http.mdx @@ -439,6 +439,7 @@ var options = new InsertOptions * The client automatically fetches table structure via `SELECT * FROM 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. #### POCO inserts {#poco-insert} @@ -1269,7 +1270,8 @@ Ownership passes only once the arguments are accepted: if the call throws `Argum -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. ---