Fix JsonSchemaExporter dropping nullability for nullable floating-point composition schemas#129530
Fix JsonSchemaExporter dropping nullability for nullable floating-point composition schemas#129530Copilot wants to merge 11 commits into
Conversation
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
eiriktsarpalis
left a comment
There was a problem hiding this comment.
@copilot This doesn't look right.
| "anyOf": [ | ||
| { "type": "number" }, | ||
| { "enum": ["NaN", "Infinity", "-Infinity"] }, | ||
| { "type": "null" } |
There was a problem hiding this comment.
This looks invalid. The expected schema should have had the first subschema be { "type" : ["number","null"] }.
There was a problem hiding this comment.
Updated — the first subschema is now { "type": ["number","null"] } and the separate null branch is gone. Fixed in 11a4bcb.
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Agreed — the original approach was wrong. I've reworked it in {"anyOf":[{"type":["number","null"]},{"enum":["NaN","Infinity","-Infinity"]}]}This also handles the empty/type-less |
| return; | ||
| } | ||
|
|
||
| if (firstTypedBranch is null && branch.Type is not JsonSchemaType.Any) |
There was a problem hiding this comment.
@copilot I fail to see how this fix is valid in the general case. If anyOf contained objects, this code would erroneously insert null in the first object in the list which we clearly don't want to do. What we should be fixing here is handling specifically for nullable structs when number handling is specified, not inaccurate post hoc patching like is being attempted here.
There was a problem hiding this comment.
You're right. I reverted the MakeNullable change entirely and moved the fix into the nullable-value-type path (NullableElementConverter block in JsonSchemaExporter), alongside the existing enum handling. This code only runs for Nullable<T>, where the element schema can only be an anyOf when it's an IEEE floating-point type under AllowNamedFloatingPointLiterals — so the null is folded into the numeric branch ({ "type": ["number","null"] }) and never touches object/polymorphic compositions. Fixed in c81c58a.
…, revert MakeNullable Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes JSON Schema export for nullable IEEE floating-point types when JsonNumberHandling.AllowNamedFloatingPointLiterals causes the element schema to be expressed as an anyOf composition, ensuring null remains an accepted value in the exported schema.
Changes:
- Update
JsonSchemaExporter.MapJsonSchemaCoreto preservenullacceptance for nullable-wrapper schemas when the element schema is ananyOfwith anenumbranch (named float literals). - Add regression coverage for
double?,float?, and a POCO with nullable floating-point properties under globalAllowNamedFloatingPointLiterals. - Extend the source-generation test context to include the new nullable float and POCO test types.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Text.Json/src/System/Text/Json/Schema/JsonSchemaExporter.cs | Adds nullable-wrapper handling for anyOf schemas by appending null to the relevant enum branch. |
| src/libraries/System.Text.Json/tests/Common/JsonSchemaExporterTests.TestTypes.cs | Adds regression test vectors for nullable floating-point schemas under AllowNamedFloatingPointLiterals. |
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/JsonSchemaExporterTests.cs | Updates the source-gen context to include the new test types. |
tarekgh
left a comment
There was a problem hiding this comment.
A few issues found while reviewing the nullable floating-point fix.
| { | ||
| if (branch.Enum is { } enumValues) | ||
| { | ||
| enumValues.Add(null); // Append null to the enum array. |
There was a problem hiding this comment.
This emits {"enum":["NaN","Infinity","-Infinity",null]}. For consistency with the default number-handling output ({"type":["number","null"]}), null should be folded into the type keyword of the numeric branch instead, producing {"anyOf":[{"type":["number","null"]},{"enum":["NaN","Infinity","-Infinity"]}]}.
| // named floating-point literals, mirroring the enum handling above. | ||
| foreach (JsonSchema branch in anyOf) | ||
| { | ||
| if (branch.Enum is { } enumValues) |
There was a problem hiding this comment.
If no branch carries an Enum, the loop completes without adding null and the schema is silently left non-nullable. Recommend a Debug.Assert that elementTypeInfo is a numeric type formatted with AllowNamedFloatingPointLiterals, plus a safe fallback so null is still accepted if the element shape ever changes.
| } | ||
| else if (schema.AnyOf is { } anyOf) | ||
| { | ||
| // The element schema is an "anyOf" composition, which for a nullable value type |
There was a problem hiding this comment.
This comment is too absolute. NullableElementConverter is also used by other internal wrappers (F# option/value-option, casting converters), so an anyOf here is not provably only an IEEE float schema. Soften the wording and pair it with the assert/fallback noted below.
tarekgh
left a comment
There was a problem hiding this comment.
Left a few comments, LGTM otherwise.
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
…ullability Co-authored-by: eiriktsarpalis <10833894+eiriktsarpalis@users.noreply.github.com>
| // The element schema is an "anyOf" composition, which for nullable value types | ||
| // should only originate from IEEE floating-point types using | ||
| // AllowNamedFloatingPointLiterals. | ||
| Debug.Assert((effectiveNumberHandling & JsonNumberHandling.AllowNamedFloatingPointLiterals) != 0 && |
There was a problem hiding this comment.
@copilot This check is too brittle and could possibly break with things like nullable C# struct unions. (Do we have tests for these? if not please add a few here!)
What I would recommend instead is introducing a new internal virtual bool IsIeeeFloatingPointConverter => false property on JsonConverter and have that overridden on the relevant built-in converters (assuming such a flag doesn't exist already). This branch should only be entered by checking against that property on elementConverter.
There was a problem hiding this comment.
Introduced a new internal virtual bool IsIeeeFloatingPointConverter => false property on JsonConverter, overridden to true in DoubleConverter, SingleConverter, and HalfConverter. The nullable-wrapper path now checks elementConverter.IsIeeeFloatingPointConverter before folding null into the numeric branch. For non-IEEE converters (e.g., nullable struct unions), the code appends a standalone { "type": "null" } branch to preserve nullability. Added test coverage for nullable struct unions. Fixed in a272333.
| } | ||
| } | ||
|
|
||
| Debug.Assert(foundNumberBranch); |
There was a problem hiding this comment.
@copilot replace the redundant boolean with a debug assert that calls List.Exists before the foreach.
There was a problem hiding this comment.
Replaced the boolean with a Debug.Assert(anyOf.Exists(b => (b.Type & JsonSchemaType.Number) != 0)) before the foreach loop. Fixed in a272333.
Co-authored-by: eiriktsarpalis <10833894+eiriktsarpalis@users.noreply.github.com>
Introduced a virtual property `IsIeeeFloatingPointConverter` on `JsonConverter`, overridden to `true` in `DoubleConverter`, `SingleConverter`, and `HalfConverter`. Updated `JsonSchemaExporter` to check this property instead of performing brittle element type checks, preventing false positives from nullable struct unions. Replaced the boolean foundNumberBranch with a Debug.Assert that checks the list before iteration. Added test coverage for nullable struct unions to ensure nullability is preserved without affecting IEEE floating-point schema generation. Co-authored-by: eiriktsarpalis <16040868+eiriktsarpalis@users.noreply.github.com>
JsonSchemaExporteromitted null-acceptance for CLR-nullable types (double?,float?,Half?) whose schema is emitted as ananyOfcomposition — which happens for IEEE floating-point types underJsonNumberHandling.AllowNamedFloatingPointLiterals. The same property exports correctly as{"type":["number","null"]}under default number handling, so adouble?was advertised to downstream JSON-Schema/OpenAPI consumers as a non-nullable number.Description
JsonSchema.MakeNullableonly addsJsonSchemaType.Nullwhen the schema carries a concretetypekeyword. Composition schemas (anyOf) haveType == Any, so nullable wrappers over floating-pointanyOfschemas could lose nullability.internal virtual bool IsIeeeFloatingPointConverterproperty onJsonConverter, overridden totrueinDoubleConverter,SingleConverter, andHalfConverter. Updated the nullable-value-type path inJsonSchemaExporter.MapJsonSchemaCoreto check this property instead of performing brittle element type checks: when the element schema isanyOfand the converter is an IEEE floating-point converter, null is folded into the numeric branch'stypekeyword ("type": ["number","null"]). For non-IEEE converters (e.g., nullable struct unions), a standalone{ "type": "null" }branch is appended to ensure nullability. Added aDebug.Assertto validate the expected shape for IEEE floating-point types withAllowNamedFloatingPointLiterals.double?,float?,Half?, and a POCO with nullable floating-point properties under globalAllowNamedFloatingPointLiterals, plus nullable struct union test cases, across reflection and source-gen test contexts.Resulting export for a nullable property: