Skip to content

fix(types): preserve JSON paths starting with setting names - #498

Merged
alex-clickhouse merged 2 commits into
ClickHouse:mainfrom
fallintoplace:fix/json-setting-path-prefix
Aug 3, 2026
Merged

fix(types): preserve JSON paths starting with setting names#498
alex-clickhouse merged 2 commits into
ClickHouse:mainfrom
fallintoplace:fix/json-setting-path-prefix

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

JSON type parsing filtered max_dynamic_paths and max_dynamic_types using prefix matching. This caused valid typed paths such as max_dynamic_paths_used UInt64 to be discarded as settings.

Recognize these entries as settings only when followed by an assignment, while leaving SKIP handling unchanged.

Tests

  • Added real-server coverage for exact and prefixed path names
  • Covered both affected setting names and preserved spaced assignment parsing
  • dotnet test ClickHouse.Driver.Tests --framework net10.0 --property WarningLevel=0 --filter "Name~ShouldSelectDataWithComplexHintedJsonType|Name~PathStartingWithMaxDynamic|Name~PathNamedMaxDynamicPaths"
  • dotnet build ClickHouse.Driver.sln --framework net10.0 --property WarningLevel=0

Checklist

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

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

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 a JSON type-hint parsing bug in JsonType where typed JSON paths whose names start with max_dynamic_paths / max_dynamic_types were incorrectly treated as JSON settings and therefore ignored, causing values to be decoded as dynamic instead of using the intended hinted type. It also adds real-server test coverage and documents the fix in release notes and the changelog.

Changes:

  • Tightened JSON “setting” detection to only treat max_dynamic_paths / max_dynamic_types as settings when they use assignment syntax (...=...), while keeping SKIP handling intact.
  • Added real-server tests covering typed paths that start with or equal the affected setting names, plus spaced assignment parsing (max_dynamic_paths = 10).
  • Documented the bug fix in both CHANGELOG.md and RELEASENOTES.md.

Reviewed changes

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

File Description
ClickHouse.Driver/Types/JsonType.cs Adjusts filtering logic so typed-path hints aren’t dropped when their names start with JSON setting names.
ClickHouse.Driver.Tests/Types/JsonTypeTests.cs Adds real-server coverage for affected path names and spaced assignment settings syntax.
CHANGELOG.md Adds a user-facing bug-fix entry for the JSON typed-path parsing correction.
RELEASENOTES.md Adds a release-notes bug-fix entry describing the JSON typed-path parsing correction.

Comment thread ClickHouse.Driver/Types/JsonType.cs
@fallintoplace fallintoplace changed the title Fix JSON type hints whose paths start with setting names fix(types): preserve JSON paths starting with setting names Jul 31, 2026
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
ClickHouse.Driver/Types/JsonType.cs 87.50% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@alex-clickhouse

Copy link
Copy Markdown
Collaborator

Hi, thanks for this. Looks good, just one small thing with the tests: let's add some cases with a variety of spaces around the assignment, eg something along the lines of:

[Test]
[TestCase("JSON(max_dynamic_paths = 10, a String)")]
[TestCase("JSON(max_dynamic_paths  =  10, a String)")]
[TestCase("JSON(max_dynamic_types =3, a String)")]
public void ParseShouldExcludeSettingsFromHintedPaths(string typeString)
{
   var type = (JsonType)TypeConverter.ParseClickHouseType(typeString, TypeSettings.Default);
   Assert.That(type.HintedTypes.Keys, Is.EquivalentTo(new[] { "a" }));
}

@fallintoplace
fallintoplace force-pushed the fix/json-setting-path-prefix branch from 5cfe1dd to 146f9c2 Compare August 3, 2026 13:05
@alex-clickhouse

Copy link
Copy Markdown
Collaborator

LGTM, thank you for your contribution!

@alex-clickhouse
alex-clickhouse merged commit f05ba58 into ClickHouse:main Aug 3, 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