Fix JsonSerializer throws when serializing null value in a value-type nullable union case#128689
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
There was a problem hiding this comment.
Pull request overview
Fixes #128688 where serializing a union with a value-type nullable case (e.g., union ValueTypeNullablePairUnion(int, int?)) holding a null value would throw, because the deconstructor returns (typeof(int), null) and dispatching null to the int converter fails in UnboxOnWrite<T>. The write path now short-circuits to JSON null when the case type is a value type and the case value is null, mirroring the existing null-handling on the read path.
Changes:
- In
JsonUnionConverter<TUnion>.OnTryWrite, write JSONnulldirectly when the deconstructor returns a value-type case with a null value. - Add
UnionWithValueTypeNullableCase_SerializesNullAsJsonNullandUnionWithValueTypeNullableCase_RoundTripsNullValuetests over the existingValueTypeNullablePairUnion, exercised via both reflection and source-generation contexts.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Union/JsonUnionConverter.cs | Adds the null-value/value-type guard before dispatching to the case converter. |
| src/libraries/System.Text.Json/tests/Common/UnionTests.cs | Adds serialization and roundtrip tests for value-type nullable union cases. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
| // If the deconstructor reports a value-type case holding null, | ||
| // write JSON null directly rather than dispatching to the value-type's converter | ||
| // (which cannot unbox null). | ||
| if (caseValue is null && caseType.IsValueType) { |
| // If the deconstructor reports a value-type case holding null, | ||
| // write JSON null directly rather than dispatching to the value-type's converter | ||
| // (which cannot unbox null). | ||
| if (caseValue is null && caseType.IsValueType) { |
There was a problem hiding this comment.
The failure suggests to me that caseTypeInfo.Converter is JsonConverter<int> rather than JsonConverter<int?> which should handle null values without issue. So I suspect this might be an issue with either the union metadata or the deconstructor returning an incorrect case type.
Are you hitting this using the source generator, reflection, or both?
There was a problem hiding this comment.
I got it in both source gen and reflection. Not sure why it hits this specific converter though. I can take a look later
I've added roundtrip test on existing
union ValueTypeNullablePairUnion(int, int?);to verify that nullable case value works well for valueType. There are existing tests checking the same forstring?.Fixes #128688