Skip to content

Tweak configuration of JsonSerializerOptions #49875

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 5, 2023
Merged

Tweak configuration of JsonSerializerOptions #49875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e8e463
Tweak configuration of JsonSerializerOptions
captainsafia Aug 4, 2023
7bc7393
Set EmptyTypeInfoResolver in JsonOptions
captainsafia Aug 4, 2023
9fbc1a0
Fix whitespace
captainsafia Aug 4, 2023
fb3bd51
Update MVC tests
captainsafia Aug 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ private static Func<HttpContext, StringValues> ResolveFromRouteOrQuery(string pa
""";

public static string ResolveJsonBodyOrServiceMethod => """
private static Func<HttpContext, bool, ValueTask<(bool, T?)>> ResolveJsonBodyOrService<T>(LogOrThrowExceptionHelper logOrThrowExceptionHelper, string parameterTypeName, string parameterName, JsonOptions jsonOptions, IServiceProviderIsService? serviceProviderIsService = null)
private static Func<HttpContext, bool, ValueTask<(bool, T?)>> ResolveJsonBodyOrService<T>(LogOrThrowExceptionHelper logOrThrowExceptionHelper, string parameterTypeName, string parameterName, JsonSerializerOptions jsonSerializerOptions, IServiceProviderIsService? serviceProviderIsService = null)
{
if (serviceProviderIsService is not null)
{
Expand All @@ -197,7 +197,7 @@ private static Func<HttpContext, StringValues> ResolveFromRouteOrQuery(string pa
return static (httpContext, isOptional) => new ValueTask<(bool, T?)>((true, httpContext.RequestServices.GetService<T>()));
}
}
var jsonTypeInfo = (JsonTypeInfo<T>)jsonOptions.SerializerOptions.GetTypeInfo(typeof(T));
var jsonTypeInfo = (JsonTypeInfo<T>)jsonSerializerOptions.GetTypeInfo(typeof(T));
return (httpContext, isOptional) => TryResolveBodyAsync<T>(httpContext, logOrThrowExceptionHelper, isOptional, parameterTypeName, parameterName, jsonTypeInfo, isInferred: true);
}
""";
Expand Down Expand Up @@ -506,6 +506,7 @@ namespace Microsoft.AspNetCore.Http.Generated
{{GeneratedCodeAttribute}}
file static class GeneratedRouteBuilderExtensionsCore
{
private static readonly JsonOptions FallbackJsonOptions = new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@eerhardt eerhardt Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{GetVerbs(verbs)}}
{{endpoints}}

Expand Down
2 changes: 1 addition & 1 deletion src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointEmitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ static void ProcessParameter(EndpointParameter parameter, CodeWriter codeWriter,
}
codeWriter.Write($@"var {parameter.SymbolName}_JsonBodyOrServiceResolver = ");
var shortParameterTypeName = parameter.Type.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat);
codeWriter.WriteLine($"ResolveJsonBodyOrService<{parameter.Type.ToDisplayString(EmitterConstants.DisplayFormat)}>(logOrThrowExceptionHelper, {SymbolDisplay.FormatLiteral(shortParameterTypeName, true)}, {SymbolDisplay.FormatLiteral(parameter.SymbolName, true)}, jsonOptions, serviceProviderIsService);");
codeWriter.WriteLine($"ResolveJsonBodyOrService<{parameter.Type.ToDisplayString(EmitterConstants.DisplayFormat)}>(logOrThrowExceptionHelper, {SymbolDisplay.FormatLiteral(shortParameterTypeName, true)}, {SymbolDisplay.FormatLiteral(parameter.SymbolName, true)}, jsonSerializerOptions, serviceProviderIsService);");
}
}
}
Expand Down
10 changes: 6 additions & 4 deletions ...tp/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointJsonPreparationEmitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ internal static class EndpointJsonPreparationEmitter
{
internal static void EmitJsonPreparation(this Endpoint endpoint, CodeWriter codeWriter)
{
codeWriter.WriteLine("var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? new JsonOptions();");
codeWriter.WriteLine($"var objectJsonTypeInfo = (JsonTypeInfo<object?>)jsonOptions.SerializerOptions.GetTypeInfo(typeof(object));");
codeWriter.WriteLine("var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? FallbackJsonOptions;");
codeWriter.WriteLine("var jsonSerializerOptions = jsonOptions.SerializerOptions;");
codeWriter.WriteLine("jsonSerializerOptions.MakeReadOnly();");
codeWriter.WriteLine($"var objectJsonTypeInfo = (JsonTypeInfo<object?>)jsonSerializerOptions.GetTypeInfo(typeof(object));");

if (endpoint.Response?.IsSerializableJsonResponse(out var responseType) == true)
{
var typeName = responseType.ToDisplayString(EmitterConstants.DisplayFormatWithoutNullability);
codeWriter.WriteLine($"var responseJsonTypeInfo = (JsonTypeInfo<{responseType.ToDisplayString(NullableFlowState.MaybeNull, EmitterConstants.DisplayFormat)}>)jsonOptions.SerializerOptions.GetTypeInfo(typeof({typeName}));");
codeWriter.WriteLine($"var responseJsonTypeInfo = (JsonTypeInfo<{responseType.ToDisplayString(NullableFlowState.MaybeNull, EmitterConstants.DisplayFormat)}>)jsonSerializerOptions.GetTypeInfo(typeof({typeName}));");
}

foreach (var parameter in endpoint.Parameters)
Expand All @@ -36,7 +38,7 @@ static void ProcessParameter(EndpointParameter parameter, CodeWriter codeWriter)
return;
}
var typeName = parameter.Type.ToDisplayString(EmitterConstants.DisplayFormat);
codeWriter.WriteLine($"var {parameter.SymbolName}_JsonTypeInfo = (JsonTypeInfo<{typeName}>)jsonOptions.SerializerOptions.GetTypeInfo(typeof({parameter.Type.ToDisplayString(EmitterConstants.DisplayFormatWithoutNullability)}));");
codeWriter.WriteLine($"var {parameter.SymbolName}_JsonTypeInfo = (JsonTypeInfo<{typeName}>)jsonSerializerOptions.GetTypeInfo(typeof({parameter.Type.ToDisplayString(EmitterConstants.DisplayFormatWithoutNullability)}));");
}

}
Expand Down
4 changes: 2 additions & 2 deletions src/Http/Http.Extensions/src/JsonOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ public class JsonOptions

// 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
// the TypeInfoResolverChain. Use JsonTypeInfoResolver.Combine() to produce an empty TypeInfoResolver.
TypeInfoResolver = JsonSerializer.IsReflectionEnabledByDefault ? CreateDefaultTypeResolver() : JsonTypeInfoResolver.Combine()
Copy link
Member

@eiriktsarpalis eiriktsarpalis Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

};

// Use a copy so the defaults are not modified.
Expand Down
5 changes: 3 additions & 2 deletions src/Http/Http.Extensions/test/JsonOptionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public class JsonOptionsTests
{
[ConditionalFact]
[RemoteExecutionSupported]
public void DefaultSerializerOptions_SetsTypeInfoResolverNull_WhenJsonIsReflectionEnabledByDefaultFalse()
public void DefaultSerializerOptions_SetsTypeInfoResolverEmptyResolver_WhenJsonIsReflectionEnabledByDefaultFalse()
{
var options = new RemoteInvokeOptions();
options.RuntimeConfigurationOptions.Add("System.Text.Json.JsonSerializer.IsReflectionEnabledByDefault", false.ToString());
Expand All @@ -23,7 +23,8 @@ public void DefaultSerializerOptions_SetsTypeInfoResolverNull_WhenJsonIsReflecti
var options = JsonOptions.DefaultSerializerOptions;

// Assert
Assert.Null(options.TypeInfoResolver);
Assert.NotNull(options.TypeInfoResolver);
Assert.IsAssignableFrom<IJsonTypeInfoResolver>(options.TypeInfoResolver);
}, options);
}

Expand Down
7 changes: 5 additions & 2 deletions ...equestDelegateGenerator/Baselines/HandlesEndpointsWithAndWithoutDiagnostics.generated.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ namespace Microsoft.AspNetCore.Http.Generated
%GENERATEDCODEATTRIBUTE%
file static class GeneratedRouteBuilderExtensionsCore
{
private static readonly JsonOptions FallbackJsonOptions = new();
private static readonly string[] GetVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Get };

[InterceptsLocation(@"TestMapActions.cs", 9, 13)]
Expand All @@ -80,8 +81,10 @@ namespace Microsoft.AspNetCore.Http.Generated
var handler = Cast(del, global::System.String () => throw null!);
EndpointFilterDelegate? filteredInvocation = null;
var serviceProvider = options.ServiceProvider ?? options.EndpointBuilder.ApplicationServices;
var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? new JsonOptions();
var objectJsonTypeInfo = (JsonTypeInfo<object?>)jsonOptions.SerializerOptions.GetTypeInfo(typeof(object));
var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? FallbackJsonOptions;
var jsonSerializerOptions = jsonOptions.SerializerOptions;
jsonSerializerOptions.MakeReadOnly();
var objectJsonTypeInfo = (JsonTypeInfo<object?>)jsonSerializerOptions.GetTypeInfo(typeof(object));

if (options.EndpointBuilder.FilterFactories.Count > 0)
{
Expand Down
13 changes: 9 additions & 4 deletions .../test/RequestDelegateGenerator/Baselines/MapAction_BindAsync_NullableReturn.generated.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ namespace Microsoft.AspNetCore.Http.Generated
%GENERATEDCODEATTRIBUTE%
file static class GeneratedRouteBuilderExtensionsCore
{
private static readonly JsonOptions FallbackJsonOptions = new();
private static readonly string[] GetVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Get };

[InterceptsLocation(@"TestMapActions.cs", 25, 13)]
Expand Down Expand Up @@ -82,8 +83,10 @@ namespace Microsoft.AspNetCore.Http.Generated
EndpointFilterDelegate? filteredInvocation = null;
var serviceProvider = options.ServiceProvider ?? options.EndpointBuilder.ApplicationServices;
var logOrThrowExceptionHelper = new LogOrThrowExceptionHelper(serviceProvider, options);
var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? new JsonOptions();
var objectJsonTypeInfo = (JsonTypeInfo<object?>)jsonOptions.SerializerOptions.GetTypeInfo(typeof(object));
var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? FallbackJsonOptions;
var jsonSerializerOptions = jsonOptions.SerializerOptions;
jsonSerializerOptions.MakeReadOnly();
var objectJsonTypeInfo = (JsonTypeInfo<object?>)jsonSerializerOptions.GetTypeInfo(typeof(object));
var parameters = del.Method.GetParameters();

if (options.EndpointBuilder.FilterFactories.Count > 0)
Expand Down Expand Up @@ -188,8 +191,10 @@ namespace Microsoft.AspNetCore.Http.Generated
EndpointFilterDelegate? filteredInvocation = null;
var serviceProvider = options.ServiceProvider ?? options.EndpointBuilder.ApplicationServices;
var logOrThrowExceptionHelper = new LogOrThrowExceptionHelper(serviceProvider, options);
var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? new JsonOptions();
var objectJsonTypeInfo = (JsonTypeInfo<object?>)jsonOptions.SerializerOptions.GetTypeInfo(typeof(object));
var jsonOptions = serviceProvider?.GetService<IOptions<JsonOptions>>()?.Value ?? FallbackJsonOptions;
var jsonSerializerOptions = jsonOptions.SerializerOptions;
jsonSerializerOptions.MakeReadOnly();
var objectJsonTypeInfo = (JsonTypeInfo<object?>)jsonSerializerOptions.GetTypeInfo(typeof(object));
var parameters = del.Method.GetParameters();

if (options.EndpointBuilder.FilterFactories.Count > 0)
Expand Down
Loading