-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix JsonSerializer.Serialize producing invalid JSON for [JsonExtensionData] with JsonObject #122838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…sonObject
When serializing an object containing a property marked with [JsonExtensionData]
attribute of type JsonObject, the serializer was producing invalid JSON like
{"Id":1,{"nested":true}} instead of the correct {"Id":1,"nested":true}.
The fix modifies TryWriteDataExtensionProperty in JsonConverterOfT.cs to write
the JsonObject extension data contents without the wrapping braces, similar to
how dictionary extension data is handled.
Also added tests to verify the fix for JsonObject extension data serialization.
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
src/libraries/System.Text.Json/tests/Common/ExtensionDataTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/tests/Common/ExtensionDataTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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 bug where JsonSerializer.Serialize produced invalid JSON when serializing objects with [JsonExtensionData] properties of type JsonObject. The output was missing property names and directly including object braces (e.g., {"Id":1,{"nested":true}}), which is malformed JSON.
Key Changes:
- Modified
JsonConverterOfT.csto iterate overJsonObjectentries and write each key-value pair directly, ensuring extension data is properly flattened into the parent object - Added comprehensive test coverage for JsonObject extension data serialization scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs | Replaced the TryWrite call for JsonObject extension data with explicit iteration and property-by-property writing to flatten extension data into the parent object |
| src/libraries/System.Text.Json/tests/Common/ExtensionDataTests.cs | Added 4 new tests covering valid JSON output, round-trip serialization, empty JsonObject, and null JsonObject scenarios, plus a test class definition |
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
Outdated
Show resolved
Hide resolved
|
@copilot, please address the feedback. Also please address the failing tests, e.g. |
…WriteContentsTo - Add internal WriteContentsTo method to JsonObject that writes properties without wrapping braces, handling both dictionary-backed and JsonElement-backed cases - Update TryWriteDataExtensionProperty to call the new method, avoiding code duplication - Register ClassWithJsonObjectExtensionDataAndProperty in source generator contexts to fix failing source gen tests - Remove unnecessary using directive Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Fixed in 7f5a477. Added |
…for trimming - Add WriteExtensionDataValue virtual method to JsonConverter (base class) - Override in JsonObjectConverter to call WriteContentsTo - Update TryWriteDataExtensionProperty to use virtual method - Remove System.Text.Json.Nodes using directive from JsonConverterOfT.cs This approach maintains trimming support by avoiding direct type references to JsonObject in JsonConverterOfT.cs, following the same pattern used for ReadElementAndSetProperty. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs
Outdated
Show resolved
Hide resolved
Add back the Debug.Assert(Type == typeof(JsonObject)) assertion as requested. Since debug assertions are removed in release builds, this doesn't affect trimming support. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs
Outdated
Show resolved
Hide resolved
Moved WriteExtensionDataValue from JsonConverter to JsonConverter<T> with a generic T value parameter instead of object value. This removes the need for a downcast in the JsonObjectConverter implementation. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Head branch was pushed to by a user without write access
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
Outdated
Show resolved
Hide resolved
Removed the using directive and fully qualified JsonObject as Nodes.JsonObject in the debug assert to minimize namespace dependencies in JsonConverterOfT.cs. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Description
JsonSerializer.Serializewas producing invalid JSON when serializing objects with[JsonExtensionData]properties of typeJsonObject. The output was missing the property name for extension data entries:Root cause:
TryWriteDataExtensionPropertyinJsonConverterOfT.cscalledTryWriteforJsonObject, which writes the full object including braces. Extension data should write only the contents (key-value pairs) without wrapping braces.Fix:
WriteContentsTomethod toJsonObjectthat writes properties without wrapping braces, handling both dictionary-backed andJsonElement-backed cases with proper optimization.WriteExtensionDataValuevirtual method toJsonConverter<T>with a genericT valueparameter that throws by default, overridden inJsonObjectConverterto callWriteContentsTo. This follows the same pattern asReadElementAndSetPropertyand avoids direct type references toJsonObjectinJsonConverterOfT.csto maintain trimming support.TryWriteDataExtensionPropertyto call the virtual method instead of directly referencingJsonObject.Debug.Assertfor theJsonObjecttype check using a qualifiedNodes.JsonObjectreference (debug assertions are removed in release builds, so this doesn't affect trimming).Customer Impact
Serialization of any class with
[JsonExtensionData]of typeJsonObjectproduces malformed JSON that cannot be parsed.Regression
No. Reproducible from .NET 6 through .NET 8+. Deserialization works correctly; only serialization was affected.
Testing
JsonObject, and nullJsonObjectSystem.Text.Json.TestspassExtensionDataTestsContext_MetadataandExtensionDataTestsContext_Default)Risk
Low. Change is minimal and isolated to the
JsonObjectbranch of extension data serialization. Dictionary extension data path is unchanged. The virtual method pattern maintains trimming support by avoiding direct type references in non-debug code paths.Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.