-
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
base: main
Are you sure you want to change the base?
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 |
| Debug.Assert(Type == typeof(JsonObject)); | ||
|
|
||
| // Write the JsonObject extension data contents without the wrapping braces. | ||
| // This is necessary because extension data properties should be flattened | ||
| // into the parent object, not nested as a separate object. | ||
| JsonObject jsonObject = (JsonObject)(object)value!; | ||
| foreach (KeyValuePair<string, JsonNode?> entry in jsonObject) | ||
| { | ||
| writer.WritePropertyName(entry.Key); | ||
|
|
||
| if (entry.Value is null) | ||
| { | ||
| writer.WriteNullValue(); | ||
| } | ||
| else | ||
| { | ||
| entry.Value.WriteTo(writer, options); | ||
| } | ||
| } | ||
|
|
||
| return true; |
Copilot
AI
Jan 4, 2026
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.
The depth validation is missing for the JsonObject extension data path. While WriteTo on JsonNode will eventually check depth through the Utf8JsonWriter, it's inconsistent with how the dictionary converter path handles depth checking (lines 486-488). Consider adding an explicit depth check before iterating, similar to the dictionary path, to fail fast and provide consistent error handling.
| Debug.Assert(Type == typeof(JsonObject)); | ||
|
|
||
| // Write the JsonObject extension data contents without the wrapping braces. | ||
| // This is necessary because extension data properties should be flattened | ||
| // into the parent object, not nested as a separate object. | ||
| JsonObject jsonObject = (JsonObject)(object)value!; | ||
| foreach (KeyValuePair<string, JsonNode?> entry in jsonObject) | ||
| { | ||
| writer.WritePropertyName(entry.Key); | ||
|
|
||
| if (entry.Value is null) | ||
| { | ||
| writer.WriteNullValue(); | ||
| } | ||
| else | ||
| { | ||
| entry.Value.WriteTo(writer, options); | ||
| } | ||
| } | ||
|
|
||
| return true; |
Copilot
AI
Jan 4, 2026
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.
The JsonObject extension data path doesn't maintain the WriteStack state by calling state.Push() and state.Pop() like the dictionary path does (lines 494, 512). This inconsistency could lead to issues with state tracking, particularly for continuation scenarios or complex nested structures. Consider adding proper state management to match the dictionary path's behavior.
| if (dictionaryConverter == null) | ||
| { | ||
| // If not JsonDictionaryConverter<T> then we are JsonObject. | ||
| // Avoid a type reference to JsonObject and its converter to support trimming. |
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.
// Avoid a type reference to JsonObject and its converter to support trimming.
Is this not true anymore?
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.
Removed that comment since we now use a direct type reference to JsonObject and call the new WriteContentsTo method. The previous comment was outdated - we already had a Debug.Assert(Type == typeof(JsonObject)) so there was already a type reference.
| // This is necessary because extension data properties should be flattened | ||
| // into the parent object, not nested as a separate object. | ||
| JsonObject jsonObject = (JsonObject)(object)value!; | ||
| foreach (KeyValuePair<string, JsonNode?> entry in jsonObject) |
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.
This looks like it's copied from
runtime/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.cs
Lines 176 to 188 in 11e43c9
| foreach (KeyValuePair<string, JsonNode?> entry in Dictionary) | |
| { | |
| writer.WritePropertyName(entry.Key); | |
| if (entry.Value is null) | |
| { | |
| writer.WriteNullValue(); | |
| } | |
| else | |
| { | |
| entry.Value.WriteTo(writer, options); | |
| } | |
| } |
JsonElement backed JsonObject. This might be fine, but I think it's probably better to have this code in JsonObject and avoid duplicating it if possible.
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.
Added JsonObject.WriteContentsTo() internal method in 7f5a477. It handles both the JsonElement-backed case (using EnumerateObject()) and the dictionary-backed case, avoiding code duplication with WriteTo().
|
@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 |
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: Added a new internal
WriteContentsTomethod toJsonObjectthat writes properties without wrapping braces, handling both dictionary-backed andJsonElement-backed cases with proper optimization. UpdatedTryWriteDataExtensionPropertyto call this method, avoiding code duplication and maintaining consistency with how dictionary extension data is handled.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.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.