JsonSerializerOptions constructor is not copying the context

[AnQueth]

since the JsonSerializerOptions doesn't copy the _context from the old to the new, the json source generator stuff will not work with mvc output formatter. the call that wants to make a copy is below. since Encoder is null, it tries to make a new JsonSerialzierOptions and this constructor ignores _context.

 internal static SystemTextJsonOutputFormatter CreateFormatter(JsonOptions jsonOptions)
        {
            var jsonSerializerOptions = jsonOptions.JsonSerializerOptions;

            if (jsonSerializerOptions.Encoder is null)
            {
                // If the user hasn't explicitly configured the encoder, use the less strict encoder that does not encode all non-ASCII characters.
                jsonSerializerOptions = new JsonSerializerOptions(jsonSerializerOptions)
                {
                    Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
                };
            }

            return new SystemTextJsonOutputFormatter(jsonSerializerOptions);
        }

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

---

since the JsonSerializerOptions doesn't copy the _context from the old to the new, the json source generator stuff will not work with mvc output formatter. the call that wants to make a copy is below. since Encoder is null, it tries to make a new JsonSerialzierOptions and this constructor ignores _context.

 internal static SystemTextJsonOutputFormatter CreateFormatter(JsonOptions jsonOptions)
        {
            var jsonSerializerOptions = jsonOptions.JsonSerializerOptions;

            if (jsonSerializerOptions.Encoder is null)
            {
                // If the user hasn't explicitly configured the encoder, use the less strict encoder that does not encode all non-ASCII characters.
                jsonSerializerOptions = new JsonSerializerOptions(jsonSerializerOptions)
                {
                    Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
                };
            }

            return new SystemTextJsonOutputFormatter(jsonSerializerOptions);
        }

Author:	AnQueth
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[layomia]

This behavior is by-design: the copy constructor does not copy the type/property metadata classes to avoid unnecessary references that potentially hinder garbage collection on the source options (e.g. a group of types are deserialized with source options, but never with destination options). Note that this characteristic was discussed during the initial implementation of the copy ctor (dotnet/runtime#30445, dotnet/runtime#30445). At that time the metadata was generated at runtime with reflection, but now with source-gen it is generated at compile-time. The GC concern applies in both cases.

For your scenario, you would want to manually add your context to the new options instance. Is this feasible in your scenario?

 internal static SystemTextJsonOutputFormatter CreateFormatter(JsonOptions jsonOptions)
{
    var jsonSerializerOptions = jsonOptions.JsonSerializerOptions;

    if (jsonSerializerOptions.Encoder is null)
    {
        // If the user hasn't explicitly configured the encoder, use the less strict encoder that does not encode all non-ASCII characters.
        jsonSerializerOptions = new JsonSerializerOptions(jsonSerializerOptions)
        {
            Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
        };

        jsonSerializerOptions.AddContext<MyContext>();
    }

    return new SystemTextJsonOutputFormatter(jsonSerializerOptions);
}

[ghost]

This issue has been marked needs more info since it may be missing important information needed to assess it. Please refer to our contribution guidelines for tips on how to report issues effectively.

[AnQueth]

i copied that code from your code :) that's asp.net core code. the problem is your source gen will never be used by MVC if you do not copy the context or supply a Encoder.

[ericstj]

FYI @pranavkm -- should this bug be moved to dotnet/aspnetcore?

aspnetcore/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonOutputFormatter.cs

Lines 33 to 47 in a450cb6

	internal static SystemTextJsonOutputFormatter CreateFormatter(JsonOptions jsonOptions)
	{
	var jsonSerializerOptions = jsonOptions.JsonSerializerOptions;
	if (jsonSerializerOptions.Encoder is null)
	{
	// If the user hasn't explicitly configured the encoder, use the less strict encoder that does not encode all non-ASCII characters.
	jsonSerializerOptions = new JsonSerializerOptions(jsonSerializerOptions)
	{
	Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
	};
	}
	return new SystemTextJsonOutputFormatter(jsonSerializerOptions);
	}

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[halter73]

@ericstj @layomia ASP.NET is trying to use the user-supplied JsonSerializerOptions with one modified property (Encoder if it was null) without mutating the user-supplied object. Is there a way to do this without breaking the JSON source generator?

[layomia]

@halter73 @AnQueth thanks for the clarification.

One approach given existing constraints is that if (de)serialization has not occurred on the user's options instance, then the Encoder property can simply be set as desired, without needing to clone the instance. This would retain the source-generated metadata. Can this characteristic be relied on in ASP.NET's scenario? (I suppose not since the approach is not taken today)

If not, then perhaps we can look into new API to clone serialization metadata from one options instance to another. I'd advocate for separate API rather than inlining the logic in the copy ctor so that the choice to share references to metadata caches across options instances would be explicit (cf. #38720 (comment)). cc @eiriktsarpalis @steveharter

Another approach could be for ASP.NET to expose API to receive a callback that adds a context to an options instance.

[halter73]

One approach given existing constraints is that if (de)serialization has not occurred on the user's options instance, then the Encoder property can simply be set as desired, without needing to clone the instance. This would retain the source-generated metadata. Can this characteristic be relied on in ASP.NET's scenario? (I suppose not since the approach is not taken today)

As you say, I don't feel comfortable taking that change after we've gone out of our way not to mutate the user-provided options for so long.

If not, then perhaps we can look into new API to clone serialization metadata from one options instance to another. I'd advocate for separate API rather than inlining the logic in the copy ctor so that the choice to share references to metadata caches across options instances would be explicit (cf. #38720 (comment)).

What's the harm in sharing references to large metadata caches? I get that they're big, but I don't think there's much of a risk of applications cloning options objects and then retaining a reference to the clone longer than necessary. Why would anyone want to clone the options without the JsonSerializerContext if there is one?

Another approach could be for ASP.NET to expose API to receive a callback that adds a context to an options instance.

I'm not sure I understand this completely, but new API doesn't fix existing apps that should be benefitting from the source generator. Based on my current understanding of the issue, I think the copy ctor should copy the JsonSerializerContext reference and I think we should take this as a patch in .NET 6.

[eiriktsarpalis]

What's the harm in sharing references to large metadata caches? I get that they're big, but I don't think there's much of a risk of applications cloning options objects and then retaining a reference to the clone longer than necessary. Why would anyone want to clone the options without the JsonSerializerContext if there is one?

I think the copy constructor should try to copy the context as well, however at this point I'm slightly concerned about the potential for breaking changes by changing the current behavior. There are a few places in the current implementations where we will throw an exception if an options instance is detected to already contain a context:

https://github.com/dotnet/runtime/blob/42224c69786c6386a946dfa47ebbcaf645283f5e/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs#L96-L108

and

https://github.com/dotnet/runtime/blob/42224c69786c6386a946dfa47ebbcaf645283f5e/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs#L722-L729

I'm not entirely certain what the safest approach would be here, but we might want to expose a JsonSerializerContext property in JsonSerializerOptions to set or clear our the context as needed.

[pranavkm]

Bringing this back up - the only reason we use the copy constructor is to use the relaxed encoding which produces terser JSON for non-ASCII characters. Newtonsoft.Json uses a relaxed encoding by default and we'd wanted to keep parity with it when we started off. But given minimal endpoints do not do this, perhaps we can consider changing this behavior for 7.0 and not use the relaxed encoding with MVC.

Note that we'd continue having to use the copy-constructor with the STJ-based IJsonHelper - https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.ViewFeatures/src/Rendering/SystemTextJsonHelper.cs#L36-L39, but that gets a lot less use. Perhaps it might suffice to provide a warning there if we know a JsonSerializerContext is associated with the JsonSerializerOptions.

It might also be worthwhile consider patching this in 6.0 where we do not call the copy constructor if an AppContext switch is present. The current behavior is really buggy and silently limits the usage of the source generator in MVC apps.

[davhdavh]

I wouldn't call it "limit" when the serialized output depends on how the endpoint is defined as the trivial example in the above bug shows.
But it is clearly a breaking change since people might have used an endpoint with one output just to have it change to another because.

[davhdavh]

I can confirm that it works properly if setting the Encoder before setting the context:

services.AddMvc()
        .AddJsonOptions(o => {
            o.JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping;
            o.JsonSerializerOptions.AddContextX().AddContextY().AddContextZ().AddContext<CombinedSerializer>();
         });

[layomia]

Given the impact of this behavior, we should definitely provide a mechanism for copying context metadata from one options instance to another. We could do it in the copy ctor, but we'd have to be careful about breaking changes as stated in #38720 (comment). I'll spend some time prototyping this fix and circle back here.

On a side note, @pranavkm is the actual (de)serialization done with an overload of the serializer that takes a JsonSerializerOptions instance? I'd expect reflection to kick in and serialization to work as expected since those serializer calls eventually call this method:

https://github.com/dotnet/runtime/blob/42224c69786c6386a946dfa47ebbcaf645283f5e/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs#L594-L608

[pranavkm]

@layomia here are the call sites :

• https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs#L73
• https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonOutputFormatter.cs#L82
• https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.ViewFeatures/src/Rendering/SystemTextJsonHelper.cs#L25 // least interesting.

I'd expect reflection to kick in and serialization to work as expected since those serializer calls eventually call this method:

Generally so, but #39628 points to an interesting scenario where source generator uses different settings than JsonSerializerOptions. Either way, it'd be unfortunate if people do the work to enable the source generator but don't end up benefiting from it because of MVC.

[davhdavh]

Could the documentation and configuration of the JsonSerializerOptions also be made a bit clearer? so far I have found 4 different places this needs to be configured, instead of all detecting it from IOptions<JsonSerializerOptions> or something similar...

services.Configure<IOptions<JsonSerializerOptions>>(o => Setup(o.Value));
services.Configure<IOptions<Microsoft.AspNetCore.Http.Json.JsonOptions>>(o => Setup(o.Value.SerializerOptions));
services.AddMvc().AddJsonOptions(o => Setup(o.JsonSerializerOptions));
services.AddSignalR().AddJsonProtocol(o=>Setup(o.PayloadSerializerOptions));

[halter73]

What references IOptions<JsonSerializerOptions> @davhdavh?

[davhdavh]

Nothing but ourselves it would seem...
In lack of a well documented place to have the global options object, It was my goto setting for all the places I needed it to serializing things manually (Thanks, btw, found a couple of places I missed while searching for usages)