Skip to content

fix(types): render valid Enum type declarations - #493

Merged
alex-clickhouse merged 1 commit into
ClickHouse:mainfrom
fallintoplace:fix/enum-type-tostring
Aug 4, 2026
Merged

fix(types): render valid Enum type declarations#493
alex-clickhouse merged 1 commit into
ClickHouse:mainfrom
fallintoplace:fix/enum-type-tostring

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

EnumType.ToString() currently omits the closing parenthesis and renders labels without ClickHouse quoting or escaping. This produces malformed type names through APIs such as GetDataTypeName().

Render enum declarations using the existing ClickHouse string escaping helpers, invariant integer formatting, and canonical spacing.

Tests

Added a parser-renderer-parser regression covering escaped quotes and labels containing =.

Copilot AI review requested due to automatic review settings July 31, 2026 17:00

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.

Pull request overview

This PR fixes EnumType.ToString() so it renders valid ClickHouse enum type declarations by properly quoting/escaping labels, using invariant integer formatting, and including the closing parenthesis—preventing malformed type names from surfacing via APIs like GetDataTypeName().

Changes:

  • Fix enum declaration rendering to include proper quoting/escaping and correct closing syntax.
  • Add a parser→renderer→parser regression test covering escaped quotes and = inside labels.
  • Document the user-visible fix in the changelog and release notes.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
ClickHouse.Driver/Types/EnumType.cs Updates ToString() to emit valid, escaped, invariant ClickHouse enum declarations.
ClickHouse.Driver.Tests/Types/EnumTypeTests.cs Adds a regression test ensuring rendered enum declarations are parseable and preserve tricky labels.
CHANGELOG.md Notes the bug fix in the Unreleased “Bug Fixes” section.
RELEASENOTES.md Notes the bug fix in the Unreleased “Bug Fixes” section.

Comment on lines +61 to +62
public override string ToString() =>
$"{Name}({string.Join(", ", Values.Select(kvp => $"{kvp.Key.Escape().QuoteSingle()} = {kvp.Value.ToString(CultureInfo.InvariantCulture)}"))})";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Values is populated from the parsed declaration, and preserving that insertion order keeps the rendered type consistent with the source declaration.

The previous implementation also enumerated Values directly, so this change does not introduce a new ordering dependency. Sorting by numeric value would instead change the declared member order, so I’d prefer to keep the current behavior.

@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@alex-clickhouse

alex-clickhouse commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Hi, thanks, the basic concept is good, a couple of issues though:

  1. Let's have a proper integration test that roundtrips through the server, not just the parser.
  2. Enum16 has a ToString() override that shadows the fix. Also add Enum16 tests.

@fallintoplace
fallintoplace force-pushed the fix/enum-type-tostring branch from b2dcb9f to bc0f193 Compare August 3, 2026 17:29
@alex-clickhouse
alex-clickhouse merged commit 8b1e5f6 into ClickHouse:main Aug 4, 2026
18 checks passed
alex-clickhouse added a commit that referenced this pull request Aug 5, 2026
main added 22 new Unreleased entries since this branch was cut. Each is now
its own changelog.d/ fragment, extracted verbatim by line number rather than
retyped, so the assembled Unreleased section reproduces main's exactly (as a
set of lines; sorting by PR number reorders entries within their sections).

New fragments, one per (PR, category):

  #390 improvements   multidim blittable inserts
  #472 improvements   per-scalar Span<byte> reads
  #484 fixes          byte[]/TimeOnly HTTP parameters
  #485 fixes          JSON strings under ReadStringsAsByteArrays
  #490 breaking       raw results return compressed bytes
  #490 features       AcceptEncoding response compression
  #490 improvements   lz4 by default, HttpClient, errors, deflate
  #492 fixes          HTTP response disposal
  #493 fixes          Enum type declarations
  #494 fixes          raw-stream double dispose
  #497 fixes          GetSchema("Columns") restrictions
  #498 fixes          JSON paths starting with setting names
  #503 fixes          quoted JSON typed paths
  #504 fixes          quoted Tuple/Nested element names
  #509 fixes          {name:Type} scanner vs server lexer
  #511 fixes          {name:Type} hints after a non-hint brace
  #513 fixes          @name placeholders, heredocs, $ in names

#390's entry was appended to the *released* v1.3.0 section on main (v1.3.0
shipped 2026-06-29), so it would have documented an unreleased change under a
shipped version and never appeared in 1.4.0's notes. It moves to Unreleased as
a fragment; the rest of v1.3.0 is byte-identical.

RELEASENOTES.md regenerated with --sync-notes. `--check` passes, the solution
builds, and the packed .nupkg's releaseNotes open on v1.3.0 with no Unreleased
stub and no #390 bullet.
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.

3 participants