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

Provide an API for configuring JsonSerializerOptions instances for reflection-based serialization. #89934

Closed
eiriktsarpalis opened this issue Aug 3, 2023 · 3 comments · Fixed by #90013
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 3, 2023

Background & Motivation

The System.Text.Json reflection-based serializer makes it possible for users to supply JsonSerializerOptions instances that don't have a TypeInfoResolver property explicitly configured. It does this by silently populating the TypeInfoResolver property with a reflection-based DefaultJsonTypeInfoResolver value and then locking the instance for further modification.

Using publicly available APIs, this semantic can be emulated as follows:

void ConfigureForReflection(JsonSerializerOptions options)
{
    options.TypeInfoResolver ??= new DefaultJsonTypeInfoResolver();
    options.MakeReadOnly();
}

The problem with this approach is that it isn't thread safe when applied to the same options instance. I encountered this issue when attempting to implement reflection-based initialization in System.Net.Http.Json (cf. #89830), which can only access public STJ APIs. At the moment, the only viable workaround for libraries like System.Net.Http.Json is to do the following:

if (options.TypeInfoResolver is null)
{
    // Run a basic serialization operation to force thread-safe initialization of the options instance.
    JsonSerializer.Deserialize<int>("0"u8, options);
}

While the above works, it is kind of wasteful in that it forces an unnecessary serialization operation. We need access to the API that simply primes the options instance for reflection-based usage.

API Proposal

Add a method to JsonSerializerOptions for configuring using reflection-based defaults:

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    // Throws IOE if TypeInfoResolver is null
    public void MakeReadOnly();

+   // Populates TypeInfoResolver with DefaultJsonTypeInfoResolver if null
+   // (or the empty resolver if the IsReflectionEnabledDefault switch is disabled)
+   [RequiresUnreferencedCode, RequiresDynamicCode]
+   public void MakeReadOnlyWithReflectionDefaults();
}

cc @eerhardt

@eiriktsarpalis eiriktsarpalis added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 3, 2023
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Aug 3, 2023
@eiriktsarpalis eiriktsarpalis self-assigned this Aug 3, 2023
@ghost
Copy link

ghost commented Aug 3, 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

Background & Motivation

The System.Text.Json reflection-based serializer makes it possible for users to supply JsonSerializerOptions instances that don't have a TypeInfoResolver property explicitly configured. It does this by silently populating the TypeInfoResolver property with a reflection-based DefaultJsonTypeInfoResolver value and then locking the instance for further modification.

Using publicly available APIs, this semantic can be emulated as follows:

void ConfigureForReflection(JsonSerializerOptions options)
{
    options.TypeInfoResolver ??= new DefaultJsonTypeInfoResolver();
    options.MakeReadOnly();
}

The problem with this approach is that it isn't thread safe when applied to the same options instance. I encountered this issue when attempting to implement reflection-based initialization in System.Net.Http.Json (cf. #89830), which can only access public STJ APIs. At the moment, the only viable workaround for libraries like System.Net.Http.Json is to do the following:

if (options.TypeInfoResolver is null)
{
    // Run a basic serialization operation to force thread-safe initialization of the options instance.
    JsonSerializer.Deserialize<int>("0"u8, options);
}

While the above works, it is kind of wasteful in that it forces an unnecessary serialization operation. We need access to the API that simply primes the options instance for reflection-based usage.

API Proposal

Add a method to JsonSerializerOptions for configuring using reflection-based defaults:

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    // Throws IOE if TypeInfoResolver is null
    public void MakeReadOnly();

+   // Populates TypeInfoResolver to DefaultJsonTypeInfoResolver if null
+   // (or the empty resolver if the IsReflectionEnabledDefault switch is disabled)
+   public void MakeReadOnlyWithReflectionDefaults();
}

cc @eerhardt

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json, blocking, api-ready-for-review

Milestone: 8.0.0

@bartonjs
Copy link
Member

bartonjs commented Aug 3, 2023

Video

Rather than a distinct name (largely because we couldn't come up with one), we added a bool parameter, where false matches the existing (AOT-friendly) behavior, and true means "just make it work".

If it feels "natural", consider [ConstantExpected(Min=true,Max=true)].

namespace System.Text.Json
{
    public partial class JsonSerializerOptions
    {
        // Existing
        // Throws IOE if TypeInfoResolver is null
        public void MakeReadOnly();

        [RequiresUnreferencedCode, RequiresDynamicCode]
        public void MakeReadOnly(bool populateMissingResolver);
    }  
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 3, 2023
@eiriktsarpalis
Copy link
Member Author

One hiccup that I found with ConstantExpectedAttribute is that it isn't available in older TFMs that STJ supports. What's the recommended course of action in such situations? Only include the attribute in TFMs that support it? Polyfill the attribute?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 7, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants