[release/7.0] Ensure source generated metadata properties are read-only. (#76540)

[eiriktsarpalis]

Backport of #76540 to release/7.0

Customer Impact

The inclusion of the contract customization feature exposes a number of APIs that makes it possible for users to modify aspects of the pre-existing JsonTypeInfo contract metadata model. However, it seems that we neglected to freeze modifications for instances instantiated and cached by the source generator:

JsonTypeInfo<MyPoco> metadata = MyContext.Default.MyPoco;

// Can modify metadata on the static `Default` context instance.
metadata.CreateObject = null;
metadata.Properties.Clear();

[JsonSerializable(typeof(MyPoco))]
public partial class MyContext : JsonSerializerContext { }

public class MyPoco 
{ 
    public int Id { get; set; }
}

At first glance this problem might be considered benign, however it has the potential to create a couple of issues:

1. The source generator is aggressively caching metadata instances for performance, so this is effectively introducing global mutable state. Changes in one component can cause unforeseen changes in an unrelated context:

   // Mutate the result of a GetTypeInfo call
   JsonTypeInfo metadata = MySerializerContext.Default.GetTypeInfo(typeof(MyClass));
   metadata.Properties.Clear();
   
   // Change leaks to the static property:
   Console.WriteLine(MySerializerContext.Default.MyClass.Properties.Count); // 0

   or might be the cause of races when multiple threads are independently attempting to modify contracts:

   Parallel.For(0, 100, i =>
   {
        // Simulates multiple threads attempting to independently modify metadata:
        JsonTypeInfo typeInfo = MySerializerContext.Default.GetTypeInfo(typeof(MyPoco));
        typeInfo.Properties[0].Name = typeInfo.Properties[0].Name + ".suffix";
   });
   
   // Changes on `GetTypeInfo` results mutate the static instance
   // Id.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix
   Console.WriteLine(MySerializerContext.Default.MyPoco.Properties[0].Name);

2. Direct mutation of source gen metadata breaks the fast-path invalidation logic:

   MyContext.Default.MyPoco.Properties[0].Name = "alternative_name";
   
   // Modification ignored because the serializer still calls into the fast path that cannot be modified.
   JsonSerializer.Serialize(new MyPoco { Id = 42 }, MyContext.Default.MyPoco); // { "Id" : 42 }

Testing

Added testing validating that cached source generated metadata is read-only.

Risk

Moderate. Even though the fix is fairly straightforward, it necessitates the introduction of a new public method that can be used by the source generator for locking the metadata it is producing. Adding new APIs at such a late stage carries inherent risks, but we've explored the options thoroughly and landed on an API that should also help customers discover that the metadata can be locked and understand when that occurs.

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

While we don't expect user code will directly invoke either of these APIs, their presence is beneficial. When the metadata is locked, mutation members throw exceptions, and customers can anticipate that possibility by seeing IsReadOnly and inspecting its value and when it changes. There is a possibility user code will begin referencing MakeReadOnly in scenarios we don't anticipate, and/or that we'll want to introduce scoped or validated read-only states in the future. If those scenarios arise, it's possible the shape introduced here won't be ideal. But we have mitigation approaches if needed.

[dotnet-issue-labeler]

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, to 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]

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

---

Backport of #76540 to release/7.0

Customer Impact

The inclusion of the contract customization feature exposes a number of APIs that makes it possible for users to modify aspects of the pre-existing JsonTypeInfo contract metadata model. However, it seems that we neglected to freeze modifications for instances instantiated and cached by the source generator:

JsonTypeInfo<MyPoco> metadata = MyContext.Default.MyPoco;

// Can modify metadata on the static `Default` context instance.
metadata.CreateObject = null;
metadata.Properties.Clear();

[JsonSerializable(typeof(MyPoco))]
public partial class MyContext : JsonSerializerContext { }

public class MyPoco 
{ 
    public int Id { get; set; }
}

At first glance this problem might be considered benign, however it has the potential to create a couple of issues:

1. The source generator is aggressively caching metadata instances for performance, so this is effectively introducing global mutable state. Changes in one component can cause unforeseen changes in an unrelated context:

   // Mutate the result of a GetTypeInfo call
   JsonTypeInfo metadata = MySerializerContext.Default.GetTypeInfo(typeof(MyClass));
   metadata .Properties.Clear();
   
   // Change leaks to the static property:
   Console.WriteLine(MySerializerContext.Default.MyClass.Properties.Count); // 0

   or might be the cause of races when multiple threads are independently attempting to modify contracts:

   Parallel.For(0, 100, i =>
   {
        // Simulates multiple threads attempting to independently modify metadata:
        JsonTypeInfo typeInfo = MySerializerContext.Default.GetTypeInfo(typeof(MyPoco));
        typeInfo.Properties[0].Name = typeInfo.Properties[0].Name + ".suffix";
   });
   
   // Changes on `GetTypeInfo` results mutate the static instance
   // Id.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix
   Console.WriteLine(MySerializerContext.Default.MyPoco.Properties[0].Name);

2. Direct mutation of source gen metadata breaks the fast-path invalidation logic:

   MyContext.Default.MyPoco.Properties[0].Name = "alternative_name";
   
   // Modification ignored because the serializer still calls into the fast path that cannot be modified.
   JsonSerializer.Serialize(new MyPoco { Id = 42 }, MyContext.Default.MyPoco); // { "Id" : 42 }

Testing

Added testing validating that cached source generated metadata is read-only.

Risk

Moderate. Even though the fix is fairly straightforward, it necessitates the introduction of a single public method that can be used by the source generator for locking the metadata it is producing. Adding new APIs at such a late stage carries inherent risks, however this particular method is part of the JsonMetadataServices class which is solely intended for use by the source generator and is marked EditorBrowsableState.Never. The method implementation itself is trivial, and as with all other JsonMetadataServices methods we wouldn't support it for uses other than the source generator.

Author:	eiriktsarpalis
Assignees:	eiriktsarpalis
Labels:	area-System.Text.Json, new-api-needs-documentation
Milestone:	7.0.0

[carlossanlop]

Approved by Tactics via email.

[jeffhandley]

We're still deliberating this. We don't have consensus on adding this API in this form to address the issue. We'll hold off on merging this...

[jeffhandley]

The description and code have been updated to reflect the outcomes from the API review discussions. The APIs have been moved to JsonTypeInfo. Tactics has been informed of the change in the fix.

@carlossanlop -- please let us know how this will affect IntelliSense. Will 7.0 be missing IntelliSense for these APIs in GA? Does that ever get serviced, or would IntelliSense be missing forever in 7.0? Even if that's the case, we can tolerate the gap, but it'll be helpful to know if that's the case.

[jeffhandley]

Once CI is green, this can be merged.

[carlossanlop]

please let us know how this will affect IntelliSense

We won't have intellisense for these new APIs in GA. Here is what we will have to do to fix that:

1. We need to wait for the GA release to be published.
2. Feed the GA ref assemblies to the dotnet-api-docs build system. This will ensure the new APIs show up in MS Docs, undocumented.
3. Add the missing docs directly in the new dotnet-api-docs xmls.
4. Generate the intellisense package for GA.
5. Update Versions.props in the release/7.0 branch to consume the new package.
6. The first servicing release of 7.0 (December release?) will contain the new documentation showing up in intellisense.

[jeffhandley]

Excellent; thanks, @carlossanlop!

[carlossanlop]

CI is green. no-merge label removed. Approved and signed off. Merging now