-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix duplicate serialization of virtual properties with JsonPropertyName on base class #122842
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
…me on base class When a virtual property with [JsonPropertyName] on the base class is overridden in a derived class without the attribute, the property was being serialized twice with different JSON names (once from base class's attribute, once from derived class). The fix tracks virtual properties by their CLR member name during property resolution, so that when processing base class properties, we can detect if they have already been overridden by a derived class property - even when the JSON property names differ. This affects both the runtime (DefaultJsonTypeInfoResolver) and the source generator (JsonSourceGenerator). Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
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 virtual properties with [JsonPropertyName] on a base class were serialized twice when overridden in a derived class without the attribute. The root cause was that property conflict resolution only tracked properties by JSON name, so when the derived override lacked the attribute, the different JSON names ("Id" vs "test") prevented collision detection.
Key Changes
- Added
OverriddenVirtualPropertiesdictionary to track virtual properties by CLR member name - Added early-exit check in
AddPropertyWithConflictResolutionto detect virtual property overrides even when JSON names differ - Applied the same fix to both runtime (JsonTypeInfo.cs) and source generator (JsonSourceGenerator.Parser.cs)
- Set
IsVirtualproperty inCreatePropertyInfoCorefor source-generated metadata - Added comprehensive tests covering the bug scenario and related edge cases
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs | Added OverriddenVirtualProperties dictionary to PropertyHierarchyResolutionState and implemented virtual property override detection logic in AddPropertyWithConflictResolution |
| src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs | Mirrored the runtime changes in the source generator: added OverriddenVirtualMembers dictionary and override detection logic |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Helpers.cs | Set IsVirtual property in CreatePropertyInfoCore to ensure source-generated metadata has this flag set correctly |
| src/libraries/System.Text.Json/tests/Common/PropertyNameTests.cs | Added three test methods covering virtual property scenarios and test class definitions |
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PropertyNameTests.cs | Registered new test classes in both Metadata and Default source generation contexts |
| [Fact] | ||
| public async Task VirtualPropertyWithJsonPropertyNameOnBaseClass_SerializedOnce() | ||
| { | ||
| // Regression test for https://github.com/dotnet/runtime/issues/96998 |
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.
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.
@copilot, you can remove the comment about this being a regression test.
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 in 7954786.
| [Fact] | ||
| public async Task VirtualPropertyWithJsonPropertyNameOnBaseClass_SerializedOnce() | ||
| { | ||
| // Regression test for https://github.com/dotnet/runtime/issues/96998 |
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.
@copilot, you can remove the comment about this being a regression test.
|
|
||
| propertyInfo.MemberName = propertyInfoValues.PropertyName; | ||
| propertyInfo.MemberType = propertyInfoValues.IsProperty ? MemberTypes.Property : MemberTypes.Field; | ||
| propertyInfo.IsVirtual = propertyInfoValues.IsVirtual; |
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.
@copilot, was this a bug on its own? Are there other issues that stemmed from this not being copied, or was it otherwise benign? Are there other properties that should be copied but aren't?
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.
Yes, this was a latent bug - IsVirtual was populated by the source generator into JsonPropertyInfoValues<T>.IsVirtual but never copied to JsonPropertyInfo.IsVirtual. This was benign before because IsVirtual on JsonPropertyInfo was only checked when there was a JSON name collision (handled by the existing conflict resolution logic). My fix introduced a new check that uses IsVirtual even when JSON names differ, which exposed the missing copy.
I verified all other properties in JsonPropertyInfoValues<T> are being copied: IsProperty, IsPublic, DeclaringType, PropertyTypeInfo, Converter, Getter, Setter, IgnoreCondition, HasJsonInclude, IsExtensionData, NumberHandling, PropertyName, JsonPropertyName, and AttributeProviderFactory are all handled.
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.
If memory serves IsVirtual is a no-op setting that was added in the public source generator models and never ended up being used.
…meOnBaseClass_SerializedOnce Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
| // regardless of the JSON property name they use. This handles the case where a derived class | ||
| // overrides a virtual property but uses a different JSON property name (e.g., due to the base | ||
| // class having [JsonPropertyName] that the derived class doesn't inherit). | ||
| if (jsonPropertyInfo.IsVirtual && |
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.
@copilot this check isn't necessary, this class of bugs also impacts shadowed members that aren't necessarily virtual.
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.
@eiriktsarpalis, if a base and a derived type have the same named property but two different json names, they should be different, right?
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 the IsVirtual check in 171bc5a. The fix now tracks all properties by member name and applies to both virtual overrides and new shadowed members.
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.
if a base and a derived type have the same named property but two different json names, they should be different, right?
From the perspective of C# the property on the base type is rendered inaccessible on the derived type unless a cast is performed. Shouldn't this convention also apply to the JSON contract of the derived type? The presence of a JsonPropertyName shouldn't have any impact on that.
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.
From the perspective of C# the property on the base type is rendered inaccessible on the derived type unless a cast is performed.
They're two completely different properties, with two completely different JSON names; the only happen to have the same C# name, and for that to happen, the developer of the derived type explicitly said they were different by adding new to it. I don't see why the JSON should then pretend that they're the same.
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.
Why? Those are two completely distinct properties, both in expressed developer intent (new) and in JsonPropertyName, which is "bar" in the base and "MyProp" by default in the derived. When it's virtual/override, it's a single property, so you need to factor in relationship between attributes, but when not virtual, they're entirely distinct. The only relationship would be that when there's no attribute, they'd default to conflicting names.
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 only relationship would be that when there's no attribute, they'd default to conflicting names.
STJ will give out errors when you have conflicting JSON property names. This doesn't impact shadowed members because they are always being ignored. In other words, STJ has historically treated shadowing members as if they were overrides.
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.
STJ has historically treated shadowing members as if they were overrides.
Well that's unfortunate. We're happy with that? Or we don't like it but it'd be too breaking to change now?
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.
Given that, I'm unclear as to your original comment in this thread. What bugs does removing the IsVirtual check fix?
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.
Well that's unfortunate. We're happy with that? Or we don't like it but it'd be too breaking to change now?
It's not great, but yeah it would be a major breaking change.
What bugs does removing the IsVirtual check fix?
I haven't looked, but given that we haven't used IsVirtual as a distinguishing factor elsewhere in the algorithm I'm concerned about doing it here.
eiriktsarpalis
left a comment
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.
I'm a bit concerned with targeted fixes touching this logic, as it is particularly prone to introducing breaking changes in the generated serialization contracts. I've created a parent issue tracking all bugs related to this, I think we want to take a more principled approach about what fixes we should be making and how.
Per review feedback, the IsVirtual check was unnecessary since the same bug affects shadowed members using 'new' keyword, not just virtual overrides. The IsOverriddenOrShadowedBy check already handles both cases. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
PropertyHierarchyResolutionStateto track properties by CLR member name for override/shadow detectionAddPropertyWithConflictResolutionto skip base class properties that have been overridden or shadowedJsonSourceGenerator.Parser.csCreatePropertyInfoCoreinJsonMetadataServices.Helpers.csto setIsVirtualpropertyOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.