Tweak configuration of JsonSerializerOptions #49875

captainsafia · 2023-08-04T18:37:27Z

...DelegateGenerator/Baselines/MapAction_ExplicitBodyParam_ComplexReturn_Snapshot.generated.txt

eiriktsarpalis · 2023-08-04T18:43:37Z

src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs

@@ -506,6 +506,7 @@ namespace Microsoft.AspNetCore.Http.Generated
    {{GeneratedCodeAttribute}}
    file static class GeneratedRouteBuilderExtensionsCore
    {
+        private static readonly JsonOptions FallbackJsonOptions = new();


You should try setting a resolver here, otherwise the singleton itself would be susceptible to races as multiple threads attempt to initialize it.

We shouldn't need to set the resolver here.

The JsonSerializerOptions and JsonTypeInfoResolver get ~~lazy~~ initialized in JsonOptions here:

aspnetcore/src/Http/Http.Extensions/src/JsonOptions.cs

Lines 27 to 34 in 9427854

TypeInfoResolver = JsonSerializer.IsReflectionEnabledByDefault ? CreateDefaultTypeResolver() : null

};

// Use a copy so the defaults are not modified.

/// <summary>

/// Gets the <see cref="JsonSerializerOptions"/>.

/// </summary>

public JsonSerializerOptions SerializerOptions { get; } = new JsonSerializerOptions(DefaultSerializerOptions);

when IsReflectionEnabledByDeafult=false, the Resolver may be null. We should fix the thread safety concerns all up (RDG, RDF, and MVC) with #49849.

It would depend on how DefaultSerializerOptions is defined. If it contains a resolver it would get picked up by the copy constructor.

Apropos, does the singleton need to be JsonOptions here. Seems it might as well be a JsonSerializerOptions.

It would depend on how DefaultSerializerOptions is defined. If it contains a resolver it would get picked up by the copy constructor.

DefaultSerializerOptions doesn't initialize a resolver AFAICT (it's just new JsonSerlizerOptions(JsonDefaults.Web) so I think being explicit here helps.

Apropos, does the singleton need to be JsonOptions here. Seems it might as well be a JsonSerializerOptions.

True.

It would depend on how DefaultSerializerOptions is defined.

It is part of the above linked code. I missed a couple lines.

aspnetcore/src/Http/Http.Extensions/src/JsonOptions.cs

Lines 16 to 34 in 9427854

internal static readonly JsonSerializerOptions DefaultSerializerOptions = new JsonSerializerOptions(JsonSerializerDefaults.Web)

{

// Web defaults don't use the relaxed JSON escaping encoder.

//

// Because these options are for producing content that is written directly to the request

// (and not embedded in an HTML page for example), we can use UnsafeRelaxedJsonEscaping.

Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,

// The JsonSerializerOptions.GetTypeInfo method is called directly and needs a defined resolver

// setting the default resolver (reflection-based) but the user can overwrite it directly or by modifying

// the TypeInfoResolverChain

TypeInfoResolver = JsonSerializer.IsReflectionEnabledByDefault ? CreateDefaultTypeResolver() : null

};

// Use a copy so the defaults are not modified.

/// <summary>

/// Gets the <see cref="JsonSerializerOptions"/>.

/// </summary>

public JsonSerializerOptions SerializerOptions { get; } = new JsonSerializerOptions(DefaultSerializerOptions);

Apropos, does the singleton need to be JsonOptions here. Seems it might as well be a JsonSerializerOptions.

It should be using the DefaultSerializerOptions that is in JsonOptions because this is where our default JsonSerializerOptions is defined. We shouldn't be defining a new one here in generated code.

eiriktsarpalis · 2023-08-04T18:58:25Z

...t/RequestDelegateGenerator/Baselines/HandlesEndpointsWithAndWithoutDiagnostics.generated.txt

+                var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? FallbackJsonOptions;
+                var jsonSerializerOptions = jsonOptions.SerializerOptions;
+                jsonSerializerOptions.TypeInfoResolver ??= JsonTypeInfoResolver.Combine();


Assuming the singleton is fully configured, couldn't this be written without any mutations as follows?

Suggested change

var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? FallbackJsonOptions;

var jsonSerializerOptions = jsonOptions.SerializerOptions;

jsonSerializerOptions.TypeInfoResolver ??= JsonTypeInfoResolver.Combine();

var jsonSerializerOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value?.SerializerOptions;

if (jsonSerializerOptions is null or { TypeInfoResolver: null })

{

// FallbackJsonOptions configured with the empty resolver.

jsonSerializerOptions = FallbackJsonOptions.Value.SerializerOptions;

}

Note that JsonOptions FallbackJsonOptions = new(); does NOT have an empty resolver when JsonSerializer.IsReflectionEnabledByDefault == false.

aspnetcore/src/Http/Http.Extensions/src/JsonOptions.cs

Line 27 in 9427854

TypeInfoResolver = JsonSerializer.IsReflectionEnabledByDefault ? CreateDefaultTypeResolver() : null

That would be great...wouldn't we run the risk of overriding scenarios where the user set the serializer options without an explicit type info resolver but other properties set?

builder.Services.ConfigureHttpJsonOptions(o => o.SerializerOptions.AllowTrailingCommas = true);

Note that JsonOptions FallbackJsonOptions = new(); does NOT have an empty resolver when JsonSerializer.IsReflectionEnabledByDefault == false.

Correct! One implication that this has for our tests is the fact that our RDG tests are implicitly using the reflection-based type resolver for scenarios where we don't define JsonOptions. I realized this after trying to initialize a JsonSerializerOptions from generated code and discovering the implications of not using the one defined in JsonOptions.

Note that JsonOptions FallbackJsonOptions = new(); does NOT have an empty resolver when JsonSerializer.IsReflectionEnabledByDefault == false.

That's true for JsonOptions FallbackJsonOptions = new();, but that's not necessarily true for serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value.

Someone could call JsonOptions.SerializerOptions.TypeInfoResolver = null, but I see we've added a test for this. Now that we no longer assign to JsonOptions.SerializerOptions.TypeInfoResolver when its null like the code originally referenced in this comment, I think we're fine.

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointJsonPreparationEmitter.cs

eerhardt

This LGTM. I'm slightly nervous about changing the TypeInfoResolver to be non-null so late, but I think this is the best solution to this problem.

Note that this change also fixes #49849. That should be closed when this is merged.

eerhardt · 2023-08-06T00:39:28Z

src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonOutputFormatterTest.cs

@@ -262,8 +262,8 @@ public void WriteResponseBodyAsync_Works_WhenTypeResolverIsNull()
        var jsonOptions = new JsonOptions();
        jsonOptions.JsonSerializerOptions.TypeInfoResolver = null;

-        var stjOutputFormatter = SystemTextJsonOutputFormatter.CreateFormatter(jsonOptions);
-        Assert.IsAssignableFrom<IJsonTypeInfoResolver>(stjOutputFormatter.SerializerOptions.TypeInfoResolver);
+        var exception = Assert.Throws<InvalidOperationException>(() => SystemTextJsonOutputFormatter.CreateFormatter(jsonOptions));


(nit) the test name should probably be updated since this scenario no longer "works".

eiriktsarpalis · 2023-08-07T10:40:12Z

src/Http/Http.Extensions/src/JsonOptions.cs

-        // the TypeInfoResolverChain
-        TypeInfoResolver = JsonSerializer.IsReflectionEnabledByDefault ? CreateDefaultTypeResolver() : null
+        // the TypeInfoResolverChain. Use JsonTypeInfoResolver.Combine() to produce an empty TypeInfoResolver.
+        TypeInfoResolver = JsonSerializer.IsReflectionEnabledByDefault ? CreateDefaultTypeResolver() : JsonTypeInfoResolver.Combine()


When trying to serialize with PublishTrimmed == true this will throw a "No metadata for type 'Foo'" error message. Consider authoring a custom resolver that throws a desirable error message. You could also have STJ do it for you by setting the value to JsonSerializerOptions.Default.TypeInfoResolver (although that would require a suppresion).

Tweak configuration of JsonSerializerOptions

3e8e463

captainsafia requested review from Tratcher, BrennanConroy and halter73 as code owners August 4, 2023 18:37

ghost added the area-runtime label Aug 4, 2023

eerhardt reviewed Aug 4, 2023

View reviewed changes

...DelegateGenerator/Baselines/MapAction_ExplicitBodyParam_ComplexReturn_Snapshot.generated.txt Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Aug 4, 2023

View reviewed changes

Set EmptyTypeInfoResolver in JsonOptions

7bc7393

captainsafia requested a review from a team as a code owner August 4, 2023 20:51

eerhardt reviewed Aug 4, 2023

View reviewed changes

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointJsonPreparationEmitter.cs Outdated Show resolved Hide resolved

eerhardt approved these changes Aug 4, 2023

View reviewed changes

Fix whitespace

9fbc1a0

captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Aug 4, 2023

Update MVC tests

fb3bd51

captainsafia merged commit cf12d96 into main Aug 5, 2023

captainsafia deleted the safia/rdg-jsontyperesolver branch August 5, 2023 17:09

ghost added this to the 8.0-rc1 milestone Aug 5, 2023

eerhardt reviewed Aug 6, 2023

View reviewed changes

eiriktsarpalis reviewed Aug 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak configuration of JsonSerializerOptions #49875

Tweak configuration of JsonSerializerOptions #49875

captainsafia commented Aug 4, 2023 •

edited

Loading

eiriktsarpalis Aug 4, 2023

eerhardt Aug 4, 2023 •

edited

Loading

eiriktsarpalis Aug 4, 2023

captainsafia Aug 4, 2023

eerhardt Aug 4, 2023

eiriktsarpalis Aug 4, 2023

eerhardt Aug 4, 2023 •

edited

Loading

captainsafia Aug 4, 2023

captainsafia Aug 4, 2023

halter73 Aug 7, 2023

eerhardt left a comment

eerhardt Aug 6, 2023

eiriktsarpalis Aug 7, 2023 •

edited

Loading

	TypeInfoResolver = JsonSerializer.IsReflectionEnabledByDefault ? CreateDefaultTypeResolver() : null
	};

	// Use a copy so the defaults are not modified.
	/// <summary>
	/// Gets the <see cref="JsonSerializerOptions"/>.
	/// </summary>
	public JsonSerializerOptions SerializerOptions { get; } = new JsonSerializerOptions(DefaultSerializerOptions);

	internal static readonly JsonSerializerOptions DefaultSerializerOptions = new JsonSerializerOptions(JsonSerializerDefaults.Web)
	{
	// Web defaults don't use the relaxed JSON escaping encoder.
	//
	// Because these options are for producing content that is written directly to the request
	// (and not embedded in an HTML page for example), we can use UnsafeRelaxedJsonEscaping.
	Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,

	// The JsonSerializerOptions.GetTypeInfo method is called directly and needs a defined resolver
	// setting the default resolver (reflection-based) but the user can overwrite it directly or by modifying
	// the TypeInfoResolverChain
	TypeInfoResolver = JsonSerializer.IsReflectionEnabledByDefault ? CreateDefaultTypeResolver() : null
	};

	// Use a copy so the defaults are not modified.
	/// <summary>
	/// Gets the <see cref="JsonSerializerOptions"/>.
	/// </summary>
	public JsonSerializerOptions SerializerOptions { get; } = new JsonSerializerOptions(DefaultSerializerOptions);

-                var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? FallbackJsonOptions;
-                var jsonSerializerOptions = jsonOptions.SerializerOptions;
-                jsonSerializerOptions.TypeInfoResolver ??= JsonTypeInfoResolver.Combine();
+                var jsonSerializerOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value?.SerializerOptions;
+                if (jsonSerializerOptions is null or { TypeInfoResolver: null })
+                {
+                     // FallbackJsonOptions configured with the empty resolver.
+                     jsonSerializerOptions = FallbackJsonOptions.Value.SerializerOptions;
+                }

Tweak configuration of JsonSerializerOptions #49875

Tweak configuration of JsonSerializerOptions #49875

Conversation

captainsafia commented Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

eerhardt Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiriktsarpalis Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

captainsafia commented Aug 4, 2023 •

edited

Loading

eerhardt Aug 4, 2023 •

edited

Loading

eerhardt Aug 4, 2023 •

edited

Loading

eiriktsarpalis Aug 7, 2023 •

edited

Loading