-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Support IReadOnlyDictionary<string, object/JsonElement> as JsonExtensionData #120636
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
Co-authored-by: stephentoub <[email protected]>
…ression Co-authored-by: stephentoub <[email protected]>
@copilot, there are test failures:
|
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 extends JsonExtensionData support to include IReadOnlyDictionary<string, object>
and IReadOnlyDictionary<string, JsonElement>
as valid extension data property types, alongside the existing IDictionary
support.
- Adds validation logic to accept IReadOnlyDictionary interface types when Dictionary can be assigned to them
- Updates instantiation logic to create Dictionary instances for IReadOnlyDictionary extension data properties
- Enhanced documentation to reflect the new supported types
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
JsonTypeInfo.cs | Added validation logic for IReadOnlyDictionary extension data properties |
JsonSerializer.Read.HandlePropertyName.cs | Added instantiation logic to create Dictionary instances for IReadOnlyDictionary properties |
JsonExtensionDataAttribute.cs | Updated XML documentation to include IReadOnlyDictionary support |
ExtensionDataTests.cs | Added comprehensive tests for IReadOnlyDictionary extension data functionality |
DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs | Added test cases for IReadOnlyDictionary types in extension data validation |
Fixed in commit 3f9c7dc. The source generator's |
…ion data Co-authored-by: stephentoub <[email protected]>
@copilot, please ensure you build and fully test all changes. There are build failures:
|
Co-authored-by: stephentoub <[email protected]>
... Fixed in commit 8f4a3e0. The |
// IReadOnlyDictionary is supported only if a Dictionary can be assigned to it (e.g., the interface itself) | ||
(typeof(IReadOnlyDictionary<string, object>).IsAssignableFrom(propertyType) && | ||
propertyType.IsAssignableFrom(typeof(Dictionary<string, object>))) || |
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 is the same as writing
// IReadOnlyDictionary is supported only if a Dictionary can be assigned to it (e.g., the interface itself) | |
(typeof(IReadOnlyDictionary<string, object>).IsAssignableFrom(propertyType) && | |
propertyType.IsAssignableFrom(typeof(Dictionary<string, object>))) || | |
propertyType == typeof(IReadOnlyDictionary<string, object>) || |
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.
Simplified in commit 0e3f7a1. The logic now uses direct type equality checks instead of the more complex assignability checks.
// Also check for IReadOnlyDictionary<string, object> or IReadOnlyDictionary<string, JsonElement> | ||
// but only if Dictionary can be assigned to it (to exclude ImmutableDictionary and similar types) |
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.
Practically, this means we're extending support for only two types: IReadOnlyDictionary<> and Dictionary<> (which we already support). So we only need to check for an exact match of the former.
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.
More importantly though, the premise of the JsonExtensionData
feature is appending unbound data to mutable dictionary like types. Assuming we wanted to extend this to read-only types, we want to additionally check if the property has a setter. Today we're only checking on the basis of the property type, so this would need to be refactored quite a bit.
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.
Co-authored-by: stephentoub <[email protected]>
Original prompt
Fixes #31645
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.