Skip to content

Conversation

@eiriktsarpalis
Copy link
Member

Disables fast path serialization for types with properties using custom converters. While technically it should be possible to support fast path serialization for this scenario, it would require making the JsonPropertyInfo.ConverterBase property public. We should consider doing this in .NET 7.

Fixes #58119.

@ghost
Copy link

ghost commented Aug 27, 2021

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

Issue Details

Disables fast path serialization for types with properties using custom converters. While technically it should be possible to support fast path serialization for this scenario, it would require making the JsonPropertyInfo.ConverterBase property public. We should consider doing this in .NET 7.

Fixes #58119.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: 6.0.0

@eiriktsarpalis
Copy link
Member Author

Should be backported to release/6.0

@steveharter
Copy link
Contributor

While technically it should be possible to support fast path serialization for this scenario, it would require making the JsonPropertyInfo.ConverterBase property public

Why is this? I assume we just need to call myconverter.Write(.

@eiriktsarpalis
Copy link
Member Author

While technically it should be possible to support fast path serialization for this scenario, it would require making the JsonPropertyInfo.ConverterBase property public

Why is this? I assume we just need to call myconverter.Write(.

property-level converters need to be instantiated and cached with property-level metadata (otherwise we would have to allocate a new converter on each serialization). We currently do cache a JsonPropertyInfo[], however the converter property in JsonPropertyInfo has not been made public yet (and therefore cannot be consumed by the fast path serializer). If we wanted to get this working without adding new APIs we would probably need to create a separate JsonConverter[] but that would come at the cost of added allocations and redundant code.

@steveharter
Copy link
Contributor

The long-term and more ideal implementation is to use the property-level converter in the fast-path, meaning add the new serializer-gen interop API (it will need to be public, but treated as internal). If we are going to add any other new interop APIs then I think we should use the converter in the fast path, otherwise I think the PR here is fine.

Copy link
Contributor

@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.

At this point, we should disable fast path and use serializer, and fix this in 7.0.

Also this PR needs to get in before the "property factory" support in #58267 is added, since fast path also needs to be disabled for properties that have factory converters.

@steveharter steveharter merged commit 7b32a1a into dotnet:main Sep 2, 2021
@steveharter
Copy link
Contributor

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1195028196

@eiriktsarpalis eiriktsarpalis deleted the sourcegen-propertyconverter-disablefastpath branch September 3, 2021 17:40
@ghost ghost locked as resolved and limited conversation to collaborators Oct 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Text.Json code generator: custom converter ignored?

3 participants