Skip to content
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

Add support for serializing properties in interface hierarchies #78788

Merged

Conversation

eiriktsarpalis
Copy link
Member

@ghost
Copy link

ghost commented Nov 23, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #41749.

cc @captainsafia

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Nov 23, 2022
@eiriktsarpalis eiriktsarpalis added the enhancement Product code improvement that does NOT require public API changes/additions label Nov 23, 2022
@tarekgh
Copy link
Member

tarekgh commented Nov 23, 2022

/_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs(64,13): Trim analysis error IL2072: System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo<T>.LateAddProperties(): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.Interfaces' in call to 'System.Text.Json.Reflection.ReflectionExtensions.GetSortedInterfaceHierarchy(Type)'. The return value of method 'System.Text.Json.Serialization.Metadata.JsonTypeInfo.Type.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [/__w/1/s/src/libraries/sfx.proj]

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modulo the analyzer failure, LGTM.

@eiriktsarpalis
Copy link
Member Author

/_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs(64,13): Trim analysis error IL2072: System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo<T>.LateAddProperties(): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.Interfaces' in call to 'System.Text.Json.Reflection.ReflectionExtensions.GetSortedInterfaceHierarchy(Type)'. The return value of method 'System.Text.Json.Serialization.Metadata.JsonTypeInfo.Type.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [/__w/1/s/src/libraries/sfx.proj]

The changes seem to have triggered false positive warnings being emitted by the trimmer. After a lot of experimentation, I was able to isolate the issue to the one for loop that was changed to a foreach loop in ReflectionJsonTypeInfoOfT.cs. I refactored the foreach body into a separate method to avoid hitting the issue.

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Nov 25, 2022

Wasm test failures are unrelated:

eng/testing/wasm-provisioning.targets(102,5): error : Could not find a chrome snapshot folder under https://storage.googleapis.com/chromium-browser-snapshots/Linux_x64, for branch positions 1202 to 1232, for version 107.0.5304.121.

@eiriktsarpalis eiriktsarpalis merged commit 58f59db into dotnet:main Nov 25, 2022
@eiriktsarpalis eiriktsarpalis deleted the feature-interface-inheritance branch November 25, 2022 14:35
@angelaki
Copy link

Just since I'm curious / fast checking haven't found it: where / how are duplicate property named handled?

@eiriktsarpalis
Copy link
Member Author

Same as how they are handled in regular class hierarchies: we throw an exception:

ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(Type, jsonPropertyInfo.Name);

@angelaki
Copy link

Ah sure, Attributes made it possible before, too! Thank you very much.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 25, 2022
@jeffhandley jeffhandley added the blog-candidate Completed PRs that are candidate topics for blog post coverage label Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json blog-candidate Completed PRs that are candidate topics for blog post coverage enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serialization of properties from inherited interfaces vs abstract base classes
4 participants