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.

Sign up for GitHub

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

Jump to bottom

Align dictionary key converter/metadata retrieval with pattern for collection elements #50074

Merged
2 commits merged into from
Mar 24, 2021
Merged

Align dictionary key converter/metadata retrieval with pattern for collection elements #50074

2 commits merged into from
Mar 24, 2021

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Mar 23, 2021 •
edited
Loading

Alternative approach to #48232. Here we create a KeyClassInfo for dictionary keys, similar to how it is done for dictionary and collection elements/values.

This is needed so incoming STJ source generator can pre-generate metadata for dictionary key types, and pass that directly to the serializer. This is analogous to the design for collection elements e.g. here. It should also help with other scenarios described in #48232.

The breaking change described in #48232 (comment) where custom converters will throw InvalidOperationExeption (now NotSupportedException, following this change) can be mitigated by exposing the functionality to handle dictionary keys: #46520.

@layomia layomia added this to the 6.0.0 milestone Mar 23, 2021
@layomia layomia self-assigned this Mar 23, 2021
@ghost
Copy link

ghost commented Mar 23, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Alternative approach to #48232. Here we create a KeyClassInfo for dictionary keys, similar to how it is done for dictionary and collection elements/values.

This is needed so incoming STJ source generator can pre-generate metadata for dictionary key types, and pass that directly to the serializer. This is analogous to the design for collection elements e.g. here. It should also help with other scenarios described in #48232.

The breaking change described in #48232 (comment) where custom converters will throw InvalidOperationExeption (now NotSupportedException, following this change) can be mitigated by exposing the functionality to handle dictionary keys: #50071.

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json
Milestone: 6.0.0

@steveharter
Copy link
Member

Are we OK with "breaking" this and taking a hard dependency on #46520?

@layomia
Copy link
Contributor Author

layomia commented Mar 23, 2021

Are we OK with "breaking" this and taking a hard dependency on #46520?

Yes, I believe so. It is rare for users to provide custom simple converters (aside from TimeSpan, which will have built-in support in 6.0), especially now that we have the feature to read quoted number values. I think very few folks will hit this, and #46520 will be a good mitigation. If we take this change, I'll prioritize getting that feature in.

steveharter
steveharter reviewed Mar 24, 2021
View reviewed changes
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

internal virtual void WriteWithQuotes(Utf8JsonWriter writer, [DisallowNull] T value, JsonSerializerOptions options, ref WriteStack state)
=> throw new InvalidOperationException();
=> ThrowHelper.ThrowNotSupportedException_DictionaryKeyTypeNotSupported(TypeToConvert, this);

internal sealed override void WriteWithQuotesAsObject(Utf8JsonWriter writer, object value, JsonSerializerOptions options, ref WriteStack state)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we box here?

Copy link
Contributor Author

@layomia layomia Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The only caller of this specific method is from ObjectConverter, when serializing System.Object-typed keys. The values to serialize are boxed at this point. ObjectConverter forwards the (boxed) value to the typed converter for the runtime type of the instance. The typed converter then unboxes the value and serializes it.

It's the same as how TryWriteAsObject works.

steveharter
steveharter approved these changes Mar 24, 2021
View reviewed changes
Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

Looks good; we may need to react if anyone with a primitive custom converter also needs quoted number support.

However, even before this PR that scenario could be considered broken since their custom converter wasn't used for quoted numbers.

@ghost
Copy link

ghost commented Mar 24, 2021

Hello @layomia!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit d73c261 into dotnet:main Mar 24, 2021
@layomia layomia added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Mar 25, 2021
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Mar 25, 2021
@trylek trylek mentioned this pull request Apr 22, 2021
Closed
trylek added a commit to trylek/runtime that referenced this pull request Apr 22, 2021
@trylek
Fix test 49826 to work after the refactoring in dotnet#50074
ec20000 
The change dotnet#50074 made rooting of built-in serializers lazier. By
analyzing the library code I have figured out that I can root the
default serializers by calling Serialize. This fixes the test
failure.

Thanks

Tomas
@trylek trylek mentioned this pull request Apr 22, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 24, 2021
@layomia layomia deleted the DictionaryKey branch April 26, 2021 17:33
@layomia
Copy link
Contributor Author

layomia commented Nov 2, 2021

The breaking change here was reversed in #57138 and #57302.

@layomia layomia added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Nov 2, 2021
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 2, 2021
@layomia layomia removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 2, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@steveharter steveharter steveharter approved these changes

@eiriktsarpalis eiriktsarpalis Awaiting requested review from eiriktsarpalis

@jozkee jozkee Awaiting requested review from jozkee

Assignees

@layomia layomia

Labels
area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@layomia @steveharter