Skip to content

Add JsonSerializerOptions.MakeReadOnly(bool) overload.#90013

Merged
eiriktsarpalis merged 1 commit intodotnet:mainfrom
eiriktsarpalis:add/make-readonly-fallback
Aug 7, 2023
Merged

Add JsonSerializerOptions.MakeReadOnly(bool) overload.#90013
eiriktsarpalis merged 1 commit intodotnet:mainfrom
eiriktsarpalis:add/make-readonly-fallback

Conversation

@eiriktsarpalis
Copy link
Member

@ghost
Copy link

ghost commented Aug 4, 2023

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.

@eiriktsarpalis eiriktsarpalis requested a review from layomia August 4, 2023 11:32
@ghost ghost assigned eiriktsarpalis Aug 4, 2023
@eiriktsarpalis eiriktsarpalis requested a review from tarekgh August 4, 2023 11:32
@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 requested a review from eerhardt August 4, 2023 11:33
@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
@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