Provide a public api to "lock" a JsonSerializerOptions

[Alxandr]

Background and Motivation

We have a set of contract libraries that we create, all of which depend on a "core" contract library, which defines the json serialization options as a static property. Currently, this property is internal, and we use InternalsVisibleTo to give access to this property to the actual contracts projects, but it would be preferable to just have this be a public property.

The reason we don't just have it as public is that we don't want consumers of our contracts to modify it (as it's a shared static instance). However, reading through the source of JsonSerializerOptions I noticed that it contains a VerifyMutable method, meaning that there are cases where a JsonSerializerOptions is not mutable. I suggest enabling a simple public API that can take a given JsonSerializerOptions and "lock" it, or "make it immutable", so that it's safe to share in library code. This would be a no-op if the options is already immutable.

I do not know what name would be best for such a method, so the one below is just a suggestion.

Proposed API

namespace System.Text.Json
{
    public partial class JsonSerializerOptions 
    {
+       public bool IsImmutable { get; }
+       public void EnsureImmutable(); 
    }

Usage Examples

var options = new JsonSerializerOptions { TypeInfoResolver = new MyFancyResolver(), WriteIndented = true };

Console.WriteLine(options.IsLocked); // False
var converter = options.GetConverter(typeof(MyPoco)); // Returns an uncached converter instance

options.Lock(); // lock the instance and initialize the metadata cache

Console.WriteLine(options.IsLocked); // True
converter = options.GetConverter(typeof(MyPoco)); // Returns a cached converter instance

Alternative Designs

We might want to consider exposing equivalent functionality to JsonTypeInfo and JsonPropertyInfo, whose design uses a similar locking design. We're also not certain about naming, the obvious alternative being IsLocked/Lock() but there are concerns that this connotes with exclusive locking.

Risks

I don't see any risk with this API. It would also help codify the fact that a JsonSerializerOptions can be "locked" (today it just happens implicitly).

[ghost]

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

Issue Details

---

Background and Motivation

We have a set of contract libraries that we create, all of which depend on a "core" contract library, which defines the json serialization options as a static property. Currently, this property is internal, and we use InternalsVisibleTo to give access to this property to the actual contracts projects, but it would be preferable to just have this be a public property.

The reason we don't just have it as public is that we don't want consumers of our contracts to modify it (as it's a shared static instance). However, reading through the source of JsonSerializerOptions I noticed that it contains a VerifyMutable method, meaning that there are cases where a JsonSerializerOptions is not mutable. I suggest enabling a simple public API that can take a given JsonSerializerOptions and "lock" it, or "make it immutable", so that it's safe to share in library code. This would be a no-op if the options is already immutable.

I do not know what name would be best for such a method, so the one below is just a suggestion.

Proposed API

namespace System.Text.Json
{
    public class JsonSerializerOptions 
    {
+       public void EnsureImmutable(); 

Usage Examples

    public static class JsonContracts
    {
        static JsonContracts()
        {
            SerializerOptions = new JsonSerializerOptions
            {
                PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
                PropertyNameCaseInsensitive = true,
            };

            SerializerOptions.ConfigureForNodaTime(DateTimeZoneProviders.Tzdb);

			SerializerOptions.EnsureImmutable();
        }

        public static JsonSerializerOptions SerializerOptions { get; }
    }

Alternative Designs

Keeping it internal works, but requires me to configure InternalsVisibleTo for each contract project. Duplicating the code also works, but leads to code-duplication. Creating a new options each time also works.

Risks

I don't see any risk with this API. It would also help codify the fact that a JsonSerializerOptions can be "locked" (today it just happens implicitly).

Author:	Alxandr
Assignees:	-
Labels:	api-suggestion, area-System.Text.Json, untriaged
Milestone:	-

[svick]

As far as I can tell, you can already do this by using the options instance to serialize or deserialize something:

var options = new JsonSerializerOptions();

options.WriteIndented = true; // ok

JsonSerializer.Serialize<object>(null, options);

options.WriteIndented = true; // exception

That doesn't necessarily mean this API shouldn't be added, just that it's technically not necessary. (I'm also assuming that this behavior is guaranteed, but I'm not sure it actually is.)

Also, the pattern where you can make a mutable object immutable is often called "freezable" and the method to do it Freeze, so I think that's the name that should be used here.

[Alxandr]

@svick Yeah, that's the trick I'm currently experimenting with. But it's highly unintuitive, likely to be removed if you don't add a comment that explains why you're doing this, etc. And again, it's implicit "freezing". Similarly, I could also do this using reflection and setting private members, but that's opening myself up to non-breaking changes breaking me.

[eiriktsarpalis]

Sounds reasonable, but wouldn't we also need to expose a boolean property indicating that the instance is locked? cc @layomia @steveharter

[Alxandr]

I purposefully didn't add one to this suggestion, because I don't think it's strictly necessary. Exceptions on attempts to modify (as today), and an EnsureImmutable that just does nothing if it's already immutable (so you can call it multiple times) would cover all the cases I can imagine. But, having a property would definitely make asserting that the options you give out in a public API is actually locked (say - in a unit test for instance).

[layomia]

Sounds reasonable, but wouldn't we also need to expose a boolean property indicating that the instance is locked? cc @layomia @steveharter

Yeah we should probably add this as well.

[terrajobst]

Video

• We'd prefer the read-only terminology because we don't really have the notion of mutable types that can become immutable, we do, however, have the notion of types becoming read only.

namespace System.Text.Json;

public partial class JsonSerializerOptions 
{
    public bool IsReadOnly { get; }
    public void MakeReadOnly();
}

[Alxandr]

MakeReadOnly would still be a no-op if it already was readonly I assume?

[Ne4to]

Looks like it requires a more general approach.
The proposed change is the single scenario specific.
There are a lot of mutable types and it's possible that similar scenarios are required.

One of the approaches could be introducing a special interface and implementing it by JsonSerializerOptions. Or at least it could be added later.

interface ICantThinkOfAName
{
    bool IsReadOnly { get; }
    void MakeReadOnly();
}

[terrajobst]

@Ne4to

We have a design rules that says "if you can't think of a method accepting the interface the interface is useless". That's the case here; it would just work as way to codify a convention, which isn't all that useful. (the fact that you can't think of a good name is another indicator in this regard, as this isn't really a concept)

[terrajobst]

@Alxandr

MakeReadOnly would still be a no-op if it already was readonly I assume?

Yes, that would be the intention.

[krwq]

would it make sense to add identical APIs to JsonTypeInfo, JsonPropertyInfo, DefaultJsonTypeInfoResolver?

[eiriktsarpalis]

would it make sense to add identical APIs to JsonTypeInfo, JsonPropertyInfo, DefaultJsonTypeInfoResolver?

I think it probably does for JsonTypeInfo, it's common enough for this to be useful.

[eiriktsarpalis]

ould it make sense to add identical APIs to JsonTypeInfo, JsonPropertyInfo, DefaultJsonTypeInfoResolver?

I think it probably does for JsonTypeInfo, it's common enough for this to be useful.

Any publicly accessible "lock" method in JsonTypeInfo should probably not configure the type, since it might create conditions for stack overflows in recursive metadata when the instance has not been stored in a cache yet.