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 JsonSerializerOptions.MakeReadOnly(bool) overload. #90013

Merged

Conversation

eiriktsarpalis
Copy link
Member

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Aug 4, 2023

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 #89934.

cc @halter73 @eerhardt

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json, new-api-needs-documentation

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Aug 4, 2023
{
if (!_isConfiguredForJsonSerializer)
{
ConfigureForJsonSerializer();
Copy link
Member

Choose a reason for hiding this comment

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

ConfigureForJsonSerializer() will throw an exception when JsonSerializer.IsReflectionEnabledByDefault == false && TypeInfoResolver == null saying:

Reflection-based serialization has been disabled for this application. Either use the source generator APIs or explicitly configure the 'JsonSerializerOptions.TypeInfoResolver' property.

I don't think ASP.NET Core will be able to use this new API because of this. There are scenarios where we need to call MakeReadOnly() on a JsonSerializerOptions that doesn't have its TypeInfoResolver set yet. One example is in MVC:

dotnet/aspnetcore#49360

MVC always creates a SystemTextJsonOutputFormatter up front without knowing if the app is actually going to use JSON serialization or not. When the app doesn't touch JSON, we shouldn't be throwing an exception at startup.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like aspnetcore was able to work around #89830 since it owns its JsonSerializerOptions instances, so we don't need new APIs to unblock that case.

The semantics of the new overload is oriented towards components accepting JsonSerializerOptions instances they don't own but still need to emulate the behavior of the JsonSerializer methods.

@eiriktsarpalis eiriktsarpalis merged commit c0967dc into dotnet:main Aug 7, 2023
103 checks passed
@eiriktsarpalis eiriktsarpalis deleted the add/make-readonly-fallback branch August 7, 2023 10:57
@ghost ghost locked as resolved and limited conversation to collaborators Oct 6, 2023
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.

Provide an API for configuring JsonSerializerOptions instances for reflection-based serialization.
5 participants