Skip to content

fix: escape embedded backticks in column names (#316) - #679

Open
bjsneto wants to merge 3 commits into
ClickHouse:mainfrom
bjsneto:fix/316-escape-quoted-identifiers
Open

bjsneto wants to merge 3 commits into
ClickHouse:mainfrom
bjsneto:fix/316-escape-quoted-identifiers

Conversation

@bjsneto

@bjsneto bjsneto commented Oct 11, 2026 •

Copy link
Copy Markdown

Summary

Fixes EncloseColumnName returning already-delimited input without escaping embedded backticks.

The method now treats the input as an unquoted column name, escapes backticks inside it, and adds the enclosing backticks. QuoteSingle and QuoteDouble continue to add delimiters without escaping their input, preserving the existing Escape().QuoteSingle() production flow and avoiding double-escaping.

Regression tests were added for embedded and already-delimited backticks, as well as the quote helper behavior.

The existing bulk-copy regression test was corrected to pass each column name as a separate ColumnNames element instead of passing two comma-separated identifiers as one value.

Fixes #316

Checklist

  • Unit tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

Escape quotes and backticks in the string quoting helpers and add regression tests.
Copilot AI balanced review requested due to automatic review settings October 11, 2026 15:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@CLAassistant

CLAassistant commented Oct 11, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 5d6792e. Configure here.

@codecov

codecov Bot commented Oct 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@bjsneto bjsneto changed the title fix: escape delimiters in quoted identifiers (#316) fix: escape embedded backticks in column names (#316) Oct 11, 2026
/// <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.

@polyglotAI-bot

Copy link
Copy Markdown
Collaborator

Summary
The HTTP client now treats column names as raw names and always adds delimiters in the quote helpers. The identifier fix is incomplete for backslash/backtick combinations, and the breaking change to insert column-name handling is not documented. Block pending these changes.

❌ Blockers
  • ClickHouse.Driver/Utility/StringExtensions.cs:38 Escape existing backslashes before backticks so a raw name cannot terminate the generated identifier.
  • ClickHouse.Driver/Utility/StringExtensions.cs:29 Document the changed interpretation of previously quoted column names in the public insert APIs and provide migration guidance.
Tests
  • Add a server round-trip for a column whose raw name starts and ends with backticks and contains a backslash immediately before an interior backtick. Exercise the default schema probe, UseSchemaCache, and explicit ColumnTypes, and assert the inserted value.
Missing context
  • Review was static; this sandbox does not permit builds or server tests. A server round-trip is needed to validate the escaping through the public insert path.
Final verdict

❌ Block. Complete backslash escaping and document the public column-name contract change.

Automated review by Polyglot AI (gpt-6-astra) of d01d8ba. Requested by @alex-clickhouse. Replies on the inline threads are read on the next review. A maintainer can ask for a new review by mentioning @polyglotAI-bot or requesting a review from it.

@polyglotAI-bot polyglotAI-bot left a comment

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.

Review of d01d8ba posted: see the summary comment and the inline threads.

Escape backslashes before backticks in `EncloseColumnName`, add schema-integration coverage for raw names containing `\` before `` ` ``, and clarify insert column name documentation.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Distinguishing tests for already-delimited QuoteSingle and QuoteDouble inputs are still needed.

0 open findings

🧠 Review effort: Lite

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EncloseColumnName / QuoteSingle / QuoteDouble skip escaping when input starts and ends with the quote char

4 participants