Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions ClickHouse.Driver.Tests/BulkCopy/BulkCopyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -322,16 +322,30 @@ public async Task ShouldExecuteInsertWithBacktickedColumns()
{
var targetTable = CreateTableName();

await connection.ExecuteStatementAsync($"CREATE TABLE {targetTable} (`field.id` Nullable(UInt8), `@value` Nullable(UInt8)) ENGINE Memory");
await connection.ExecuteStatementAsync(
$"CREATE TABLE {targetTable} (`field.id` Nullable(UInt8), `@value` Nullable(UInt8)) ENGINE Memory");

using var bulkCopy = new ClickHouseBulkCopy(connection)
{
DestinationTableName = targetTable,
ColumnNames = new[] { "`field.id`, `@value`" }
ColumnNames = new[] { "field.id", "@value" }
};
await bulkCopy.WriteToServerAsync(Enumerable.Repeat(new object[] { 5, 5 }, 5));

using var reader = await connection.ExecuteReaderAsync($"SELECT * FROM {targetTable}");
await bulkCopy.WriteToServerAsync(
Enumerable.Repeat(new object[] { 5, 5 }, 5));

using var reader = await connection.ExecuteReaderAsync(
$"SELECT `field.id`, `@value` FROM {targetTable}");

var rows = 0;
while (await reader.ReadAsync())
{
Assert.That(reader.GetValue(0), Is.EqualTo((byte)5));
Assert.That(reader.GetValue(1), Is.EqualTo((byte)5));
rows++;
}

Assert.That(rows, Is.EqualTo(5));
}

[Test]
Expand Down
71 changes: 71 additions & 0 deletions ClickHouse.Driver.Tests/Utility/QuotedStringTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
using ClickHouse.Driver.Utility;

namespace ClickHouse.Driver.Tests.Utility;

[TestFixture]
public class QuotedStringTests
{
[Test]
public void EncloseColumnName_EscapesInteriorBackticks()
{
var result = "weird`name".EncloseColumnName();

Assert.That(result, Is.EqualTo("`weird\\`name`"));
}

[Test]
public void EncloseColumnName_AlreadyEnclosedButNeedsEscape_EscapesInterior()
{
var result = "`col`name`".EncloseColumnName();

Assert.That(result, Is.EqualTo("`\\`col\\`name\\``"));
}

[Test]
public void EncloseColumnName_HandlesNullOrEmpty()
{
Assert.That(StringExtensions.EncloseColumnName(null), Is.Null);
Assert.That(StringExtensions.EncloseColumnName(string.Empty), Is.EqualTo(string.Empty));
}

[Test]
public void QuoteSingle_AddsOuterSingleQuotes()
{
var input = "O\\'Reilly";
var result = input.QuoteSingle();

Assert.That(result, Is.EqualTo("'O\\'Reilly'"));
}

[Test]
public void QuoteSingle_EmptyString_ReturnsQuotedEmptyString()
{
Assert.That(string.Empty.QuoteSingle(), Is.EqualTo("''"));
}

[Test]
public void QuoteDouble_AddsOuterDoubleQuotes()
{
var input = "Product \\\"Items\\\"";
var result = input.QuoteDouble();

Assert.That(result, Is.EqualTo("\"Product \\\"Items\\\"\""));
}

[Test]
public void QuoteDouble_EmptyString_ReturnsQuotedEmptyString()
{
Assert.That(string.Empty.QuoteDouble(), Is.EqualTo("\"\""));
}

[TestCase("O\\'Reilly", "'O\\'Reilly'")]
[TestCase("Product \\\"Items\\\"", "\"Product \\\"Items\\\"\"")]
public void Quote_AddsOuterQuotes(string input, string expected)
{
var result = input.Contains('\'')
? input.QuoteSingle()
: input.QuoteDouble();

Assert.That(result, Is.EqualTo(expected));
}
}
8 changes: 3 additions & 5 deletions ClickHouse.Driver/Utility/StringExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ internal static class StringExtensions
public static string Escape(this string str) => str.Replace("\\", "\\\\").Replace("\'", "\\\'").Replace("\n", "\\n").Replace("\t", "\\t");

[SuppressMessage("Performance", "CA1865:Use char overload", Justification = "Not available in net462")]
public static string QuoteSingle(this string str) => str.StartsWith("'", StringComparison.InvariantCulture) && str.EndsWith("'", StringComparison.InvariantCulture) ? str : $"'{str}'";
public static string QuoteSingle(this string str) => $"'{str}'";

[SuppressMessage("Performance", "CA1865:Use char overload", Justification = "Not available in net462")]
public static string QuoteDouble(this string str) => str.StartsWith("\"", StringComparison.InvariantCulture) && str.EndsWith("\"", StringComparison.InvariantCulture) ? str : $"\"{str}\"";
public static string QuoteDouble(this string str) => $"\"{str}\"";

/// <summary>
/// Appends a value prefixed with its length, so that a concatenation of appended values maps back to
Expand All @@ -26,16 +26,14 @@ public static StringBuilder AppendLengthPrefixed(this StringBuilder builder, str

/// <summary>
/// Encloses column name in backticks (`). Escapes ` symbol if met inside name
/// Does nothing if column is already enclosed
/// The input is always treated as an unquoted column name.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Blocker: Document the breaking column-name contract

This changes the public InsertBinaryAsync(..., columns, ...) and ClickHouseBulkCopy.ColumnNames contract even though the helper is internal. An element containing the SQL spelling `id` previously selected column id; it now selects a name containing literal backticks. Existing callers therefore fail, or target a different column if both names exist. docs/http.mdx has no update, and the changelog fragment only describes the internal helper. Document the raw-name requirement in the bulk-insert section, show callers replacing previously quoted names with raw names, and record the compatibility change in the changelog. Also state that each collection element is one name.

A reply here is read on the next review: an explanation that holds up drops the finding.

/// </summary>
/// <param name="str">Column name</param>
/// <returns>Backticked column name</returns>
public static string EncloseColumnName(this string str)
{
if (string.IsNullOrEmpty(str))
return str;
if (str[0] == '`' && str[str.Length - 1] == '`')
return str; // Early return if already enclosed

var builder = new StringBuilder();
Comment thread
bjsneto marked this conversation as resolved.
Resolved
builder.Append('`');
Expand Down
1 change: 1 addition & 0 deletions changelog.d/316-quoted-identifiers.fixes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Fixed `EncloseColumnName` returning already-delimited input without escaping embedded backticks ([#316](https://github.com/ClickHouse/clickhouse-cs/issues/316)).
Loading