Skip to content

Honor JsonNumberHandlingAttribute declarations on C# unions - #133673

Merged
eiriktsarpalis merged 13 commits into
mainfrom
copilot/fix-union-int-string-json
Sep 15, 2026
Merged

eiriktsarpalis merged 13 commits into
mainfrom
copilot/fix-union-int-string-json

Conversation

Copilot AI commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Honor a union type's JsonNumberHandling when building its configuration-time case map. This lets [JsonNumberHandling(JsonNumberHandling.Strict)] on IntOrString(int, string) opt out of quoted-number ambiguity under JsonSerializerDefaults.Web.

Adds regression coverage through reflection and source-generated metadata.

Fixes #133668.

Note

This description was generated by GitHub Copilot.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Copilot AI and others added 2 commits September 11, 2026 10:04
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix union (int, string) rejection in JsonSerializerDefaults.Web Honor JsonNumberHandling overrides on C# unions Sep 11, 2026

@eiriktsarpalis eiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot I don't understand how the changes are relevant to the task at hand. Note that this should be a targeted fix.

Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>

Copilot AI commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

@copilot I don't understand how the changes are relevant to the task at hand. Note that this should be a targeted fix.

Narrowed in af64b38. The dispatch-map and hot-path case-resolution changes were removed; the production fix now only marks unions as number-handling-aware and propagates the selected case type’s explicit setting through the existing converter path.

Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Comment thread src/libraries/System.Text.Json/tests/Common/UnionTests.cs Outdated
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 14, 2026 20:18

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.

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/libraries/System.Text.Json/src/System/Text/Json/Schema/JsonSchemaExporter.cs:374

  • This drops the caller's customNumberHandling whenever the union itself has no type-level override. MapJsonSchemaCore has already computed an effective value from the parent property/collection, union metadata, and options, but each case is given only typeInfo.NumberHandling; a union nested under a container/type-info override therefore falls back to the global options and can advertise a different numeric shape than the container uses. Preserve the parent override when typeInfo.NumberHandling is null.
                            JsonSchema caseSchema = MapJsonSchemaCore(ref state, caseTypeInfo, customNumberHandling: typeInfo.NumberHandling, cacheResult: false);

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs:1178

  • The default map still ignores number-handling metadata on a nullable numeric case's element type. Nullable<T>.Converter.GetSupportedJsonValueTypes forwards the supplied handling to its element converter, and the structural classifier explicitly unwraps nullable cases to preserve elementTypeInfo.NumberHandling; this path only checks caseTypeInfo.NumberHandling. Consequently, an int type-info modifier of AllowReadingFromString on NullableNumericStringUnion(int?, string) under strict options produces a different dispatch map from the structural path and schema. Include the nullable element override when computing the effective handling.
                    target.NumberHandling ?? caseTypeInfo.NumberHandling ?? options.NumberHandling;
  • Files reviewed: 14/14 changed files
  • Comments generated: 1
  • Review effort level: Lite

Set the read/write frame handling from the union override or the selected case metadata. Cover numeric and nullable cases, collection continuations, precedence, and POCO member independence.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 14, 2026 21:35
@eiriktsarpalis eiriktsarpalis changed the title Honor JsonNumberHandling overrides on C# unions Honor JsonNumberHandlingAttribute declarations on C# unions Sep 14, 2026

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

The changes require final human review because they are too complex or risky for automated approval.

Review details
  • Files reviewed: 15/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@tannergooding

Copy link
Copy Markdown
Member

Rest of it looks good to me. Astra reports one more case where it can break though and gave a minimal repro

Carry the originating JsonTypeInfo in an internal context property so structural classification honors directly supplied contract overrides without resolving another contract through options.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 15, 2026 07:41

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.

🟡 Changes recommended

Three unresolved moderate findings remain around named floating-point literals and classifier invalidation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs:1801

  • The compile-time approximation still treats numeric cases as string-shaped only for AllowReadingFromString. Once union-level attributes are honored here, [JsonNumberHandling(AllowNamedFloatingPointLiterals)] must also be reflected in ambiguity diagnostics; otherwise source generation silently omits a warning for a union whose floating-point case accepts named JSON strings, diverging from the runtime contract. Update the approximation and add a named-literal union diagnostic case.
                JsonNumberHandling? unionNumberHandling = GetNumberHandling(unionType);

                foreach (ITypeSymbol caseType in caseTypes)
                {
                    string caseTypeName = caseType.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat);
                    JsonNumberHandling effectiveNumberHandling = unionNumberHandling ?? GetNumberHandling(caseType) ?? _contextNumberHandling;
                    JsonValueType valueTypes = GetSupportedJsonValueTypes(caseType, effectiveNumberHandling);

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs:1179

  • AllowNamedFloatingPointLiterals is also a string-consuming number-handling mode for the built-in floating-point converters ("NaN", "Infinity", and "-Infinity"), but their GetSupportedJsonValueTypes currently reports String only for AllowReadingFromString. As a result, a union such as DoubleOrString(double, string) with this union-level attribute still builds a number-only dispatch map and either rejects named literals or routes them to the string case. Include named-literal handling in the value-shape calculation (and keep the structural/source-generator paths aligned) with a regression test.
                JsonNumberHandling effectiveNumberHandling =
                    target.NumberHandling ?? caseTypeInfo.NumberHandling ?? options.NumberHandling;
                JsonValueType valueTypes = converter.GetSupportedJsonValueTypes(effectiveNumberHandling);
  • Files reviewed: 17/17 changed files
  • Comments generated: 1
  • Review effort level: Lite

@eiriktsarpalis

Copy link
Copy Markdown
Member

/ba-g test failures are unrelated

@eiriktsarpalis
eiriktsarpalis merged commit 30f6d1e into main Sep 15, 2026
80 of 82 checks passed
@eiriktsarpalis
eiriktsarpalis deleted the copilot/fix-union-int-string-json branch September 15, 2026 14:18
@eiriktsarpalis

Copy link
Copy Markdown
Member

/backport to release/11.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/11.0 (link to workflow run)

artl93 pushed a commit that referenced this pull request Sep 15, 2026
…nions (#133951)

Backport of #133673 to release/11.0

/cc @eiriktsarpalis @Copilot

## Customer Impact

- [x] Customer reported
- [ ] Found internally

Fixes a customer reported functionality gap between two newly introduced
features in .NET 11: union support and `JsonNumberHandlingAttribute`.

## Regression

- [ ] Yes
- [x] No

## Testing

Added tests covering the impacted scenaria.

## Risk

Low. Makes straightforward additions to product code addressing the
missing functionality.

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Sep 16, 2026
jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 18, 2026
…33673)

Honor a union type's `JsonNumberHandling` when building its
configuration-time case map. This lets
`[JsonNumberHandling(JsonNumberHandling.Strict)]` on `IntOrString(int,
string)` opt out of quoted-number ambiguity under
`JsonSerializerDefaults.Web`.

Adds regression coverage through reflection and source-generated
metadata.

Fixes dotnet#133668.

> [!NOTE]
> This description was generated by GitHub Copilot.

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Text.Json: [JsonNumberHandling] on a union type is not applied to its cases

4 participants