Skip to content
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

Remove JsonSerializerOptions copy in ProblemDetailsJsonOptionsSetup #46716

Merged
merged 3 commits into from
Mar 1, 2023
Merged

Remove JsonSerializerOptions copy in ProblemDetailsJsonOptionsSetup #46716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39e72c2
Remove JsonSerializerOptions copy in ProblemDetailsJsonOptionsSetup
brunolins16 Feb 16, 2023
9dc1b3e
Adding comments
brunolins16 Feb 17, 2023
463593b
Update src/Http/Http.Extensions/test/ProblemDetailsServiceCollectionE…
brunolins16 Feb 21, 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
24 changes: 16 additions & 8 deletions src/Http/Http.Extensions/src/ProblemDetailsJsonOptionsSetup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,23 @@ internal sealed class ProblemDetailsJsonOptionsSetup : IPostConfigureOptions<Jso
{
public void PostConfigure(string? name, JsonOptions options)
{
if (options.SerializerOptions.TypeInfoResolver is not null)
switch (options.SerializerOptions.TypeInfoResolver)
{
if (options.SerializerOptions.IsReadOnly)
{
options.SerializerOptions = new(options.SerializerOptions);
}

// Combine the current resolver with our internal problem details context
options.SerializerOptions.TypeInfoResolver = JsonTypeInfoResolver.Combine(options.SerializerOptions.TypeInfoResolver!, ProblemDetailsJsonContext.Default);
case DefaultJsonTypeInfoResolver:
brunolins16 marked this conversation as resolved.
Show resolved Hide resolved
// In this case, the current configuration is using a reflection-based resolver
// and we are prepending our internal problem details context to be evaluated
// first.
options.SerializerOptions.TypeInfoResolver = JsonTypeInfoResolver.Combine(ProblemDetailsJsonContext.Default, options.SerializerOptions.TypeInfoResolver);
break;
case not null:
// Combine the current resolver with our internal problem details context (adding last)
options.SerializerOptions.AddContext<ProblemDetailsJsonContext>();
Copy link
Member

Choose a reason for hiding this comment

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

This is broken when someone calls AddContext before us right? So it'll be:
custom context -> default context -> problem details context

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean because of https://github.com/brunolins16/aspnetcore/blob/9bb6ab75a30e65e4c530d647a7798c1a96b3e92e/src/Http/Http.Extensions/src/JsonOptions.cs#L37, right? If so, it will actually be:

default context -> custom context -> problem details context

Unless someone called the Combine api, JsonTypeInfoResolver.Combine(MyCustomContext.Default, options.SerializerOptions.TypeInfoResolver);, to prepend instead of append

This is kind of the issue we are discussing with S.T.J team and I am trying to fix here #46490

break;
default:
// Not adding our source gen context when TypeInfoResolver == null
// since adding it will skip the reflection-based resolver and potentially
// cause unexpected serialization problems
break;
}
}
}
30 changes: 25 additions & 5 deletions src/Http/Http.Extensions/test/ProblemDetailsServiceCollectionExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Http.Json;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;
Expand Down Expand Up @@ -103,7 +104,7 @@ public void AddProblemDetails_CombinesProblemDetailsContext()
}

[Fact]
public void AddProblemDetails_CombinesProblemDetailsContext_ForReadOnlyJsonOptions()
public void AddProblemDetails_Throws_ForReadOnlyJsonOptions()
{
// Arrange
var collection = new ServiceCollection();
Expand All @@ -120,10 +121,7 @@ public void AddProblemDetails_CombinesProblemDetailsContext_ForReadOnlyJsonOptio
var services = collection.BuildServiceProvider();
var jsonOptions = services.GetService<IOptions<JsonOptions>>();

Assert.NotNull(jsonOptions.Value);
Assert.NotNull(jsonOptions.Value.SerializerOptions.TypeInfoResolver);
Assert.NotNull(jsonOptions.Value.SerializerOptions.TypeInfoResolver.GetTypeInfo(typeof(ProblemDetails), jsonOptions.Value.SerializerOptions));
Assert.NotNull(jsonOptions.Value.SerializerOptions.TypeInfoResolver.GetTypeInfo(typeof(TypeA), jsonOptions.Value.SerializerOptions));
Assert.Throws<InvalidOperationException>(() => jsonOptions.Value);
}

[Fact]
Expand Down Expand Up @@ -166,6 +164,28 @@ public void AddProblemDetails_DoesNotCombineProblemDetailsContext_WhenNullTypeIn
Assert.Null(jsonOptions.Value.SerializerOptions.TypeInfoResolver);
}

[Fact]
public void AddProblemDetails_CombineProblemDetailsContext_WhenDefaultTypeInfoResolver()
{
// Arrange
var collection = new ServiceCollection();
collection.AddOptions<JsonOptions>();
collection.ConfigureAll<JsonOptions>(options => options.SerializerOptions.TypeInfoResolver = new DefaultJsonTypeInfoResolver());

// Act
collection.AddProblemDetails();

// Assert
var services = collection.BuildServiceProvider();
var jsonOptions = services.GetService<IOptions<JsonOptions>>();

Assert.NotNull(jsonOptions.Value);
Assert.NotNull(jsonOptions.Value.SerializerOptions.TypeInfoResolver);
Assert.IsNotType<DefaultJsonTypeInfoResolver>(jsonOptions.Value.SerializerOptions.TypeInfoResolver);
Assert.NotNull(jsonOptions.Value.SerializerOptions.TypeInfoResolver.GetTypeInfo(typeof(ProblemDetails), jsonOptions.Value.SerializerOptions));
Assert.NotNull(jsonOptions.Value.SerializerOptions.TypeInfoResolver.GetTypeInfo(typeof(TypeA), jsonOptions.Value.SerializerOptions));
}

[JsonSerializable(typeof(TypeA))]
internal partial class TestExtensionsJsonContext : JsonSerializerContext
{ }
Expand Down