Skip to content

Support STJ Polymorphism #45405

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.

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 31 commits into from
Jan 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
7cc182d
Initial GetTypeInfo usage
brunolins16 Nov 29, 2022
69ecc80
Merging 45359
brunolins16 Nov 30, 2022
fb1fabb
Using TypeInfoAPI
brunolins16 Dec 1, 2022
a2473bb
Merge branch 'main' into brunolins16/issues/44852
brunolins16 Dec 1, 2022
2eac02f
Updating comments
brunolins16 Dec 1, 2022
59e9b6e
Remove using
brunolins16 Dec 1, 2022
1e05325
Updating MVC support
brunolins16 Dec 2, 2022
044d219
Adding fastpath
brunolins16 Dec 2, 2022
0b7940f
Trying to fix mvc
brunolins16 Dec 2, 2022
32d7cf4
Updating mvc support
brunolins16 Dec 2, 2022
16f37f9
Simplify MVC
brunolins16 Dec 2, 2022
784d364
Moving more to fastpath
brunolins16 Dec 3, 2022
62edd21
Adding JsonOptions setup
brunolins16 Dec 9, 2022
5586708
Moving DefaultJsonTypeInfoResolver instance creation
brunolins16 Dec 9, 2022
d37a9c6
Merge branch 'main' into brunolins16/issues/44852
brunolins16 Dec 9, 2022
19b54b6
Fix bad merge
brunolins16 Dec 9, 2022
5f76c1b
Adding RDF unit tests
brunolins16 Dec 10, 2022
7657b25
Adding more RDF tests
brunolins16 Dec 10, 2022
c3f98bf
Adding MVC unit tests
brunolins16 Dec 12, 2022
e4eb4bd
Updating IL2026 warnings
brunolins16 Dec 12, 2022
84b7eb5
Scoping trimming warning
brunolins16 Dec 12, 2022
ca3809f
Updates
brunolins16 Dec 15, 2022
2c64c14
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Dec 15, 2022
6e87830
Changing to always JsonTypeInfo
brunolins16 Dec 16, 2022
7c6ef7c
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 3, 2023
3e136b4
Adding JsonSerializerExtensions
brunolins16 Jan 3, 2023
bc162e6
Fixing warnings
brunolins16 Jan 3, 2023
0bdfb96
Apply suggestions from code review
brunolins16 Jan 5, 2023
71bb5b6
Updating suppression
brunolins16 Jan 5, 2023
534c62c
Adding IL3050
brunolins16 Jan 5, 2023
45bbd69
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 9, 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
13 changes: 13 additions & 0 deletions src/Http/Http.Extensions/src/JsonOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Text.Encodings.Web;
using System.Text.Json;
using System.Text.Json.Serialization.Metadata;

#nullable enable

Expand All @@ -21,11 +22,23 @@ public class JsonOptions
// 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 calling
Copy link
Member

@eiriktsarpalis eiriktsarpalis Dec 15, 2022

Choose a reason for hiding this comment

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

Given that this is a global instance, I would recommend making it read-only from the get-go using the JsonSerializerOptions.MakeReadOnly() method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that here is too early to do this. Users still have a chance to chance the options at this point

Copy link
Member

Choose a reason for hiding this comment

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

This will makes the code susceptible to races, in which multiple threads try to set potentially conflicting configuration to the same singleton.

// .AddContext<TContext>()
TypeInfoResolver = CreateDefaultTypeResolver()
};

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

#pragma warning disable IL2026 // Suppressed in Microsoft.AspNetCore.Http.Extensions.WarningSuppressions.xml
#pragma warning disable IL3050 // Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.
private static IJsonTypeInfoResolver CreateDefaultTypeResolver()
=> new DefaultJsonTypeInfoResolver();
#pragma warning restore IL3050 // Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.
#pragma warning restore IL2026 // Suppressed in Microsoft.AspNetCore.Http.Extensions.WarningSuppressions.xml
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?xml version="1.0" encoding="utf-8"?>
<linker>
<assembly fullname="Microsoft.AspNetCore.Http.Extensions, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60">
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2026</argument>
<property name="Scope">member</property>
<property name="Target">M:Microsoft.AspNetCore.Http.Json.JsonOptions.CreateDefaultTypeResolver</property>
<property name="Justification">This warning is left in the product so developers get an ILLink warning when trimming an app, in future, only when Microsoft.AspNetCore.EnsureJsonTrimmability=false.</property>
</attribute>
</assembly>
</linker>
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
<Compile Include="$(SharedSourceRoot)RoutingMetadata\AcceptsMetadata.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)TypeNameHelper\TypeNameHelper.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)ProblemDetails\ProblemDetailsDefaults.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ValueStringBuilder\**\*.cs" LingBase="Shared"/>
<Compile Include="$(SharedSourceRoot)ValueStringBuilder\**\*.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)Json\JsonSerializerExtensions.cs" LinkBase="Shared"/>
</ItemGroup>

<ItemGroup>
Expand Down
182 changes: 138 additions & 44 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs

Large diffs are not rendered by default.

142 changes: 142 additions & 0 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
using System.Security.Cryptography.X509Certificates;
using System.Text;
using System.Text.Json;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
Expand All @@ -31,6 +33,7 @@
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Primitives;
using Moq;
using Xunit.Abstractions;

namespace Microsoft.AspNetCore.Routing.Internal;

Expand Down Expand Up @@ -3070,6 +3073,122 @@ public async Task RequestDelegateWritesMembersFromChildTypesToJsonResponseBody(D
Assert.Equal("With type hierarchies!", deserializedResponseBody!.Child);
}

public static IEnumerable<object[]> PolymorphicResult
{
get
{
JsonTodoChild originalTodo = new()
{
Name = "Write even more tests!",
Child = "With type hierarchies!",
};

JsonTodo TestAction() => originalTodo;

Task<JsonTodo> TaskTestAction() => Task.FromResult<JsonTodo>(originalTodo);
async Task<JsonTodo> TaskTestActionAwaited()
{
await Task.Yield();
return originalTodo;
}

ValueTask<JsonTodo> ValueTaskTestAction() => ValueTask.FromResult<JsonTodo>(originalTodo);
async ValueTask<JsonTodo> ValueTaskTestActionAwaited()
{
await Task.Yield();
return originalTodo;
}

return new List<object[]>
{
new object[] { (Func<JsonTodo>)TestAction },
new object[] { (Func<Task<JsonTodo>>)TaskTestAction},
new object[] { (Func<Task<JsonTodo>>)TaskTestActionAwaited},
new object[] { (Func<ValueTask<JsonTodo>>)ValueTaskTestAction},
new object[] { (Func<ValueTask<JsonTodo>>)ValueTaskTestActionAwaited},
};
}
}

[Theory]
[MemberData(nameof(PolymorphicResult))]
public async Task RequestDelegateWritesMembersFromChildTypesToJsonResponseBody_WithJsonPolymorphicOptions(Delegate @delegate)
{
var httpContext = CreateHttpContext();
httpContext.RequestServices = new ServiceCollection()
.AddSingleton(LoggerFactory)
.AddSingleton(Options.Create(new JsonOptions()))
.BuildServiceProvider();
var responseBodyStream = new MemoryStream();
httpContext.Response.Body = responseBodyStream;

var factoryResult = RequestDelegateFactory.Create(@delegate);
var requestDelegate = factoryResult.RequestDelegate;

await requestDelegate(httpContext);

var deserializedResponseBody = JsonSerializer.Deserialize<JsonTodoChild>(responseBodyStream.ToArray(), new JsonSerializerOptions
{
PropertyNameCaseInsensitive = true
});

Assert.NotNull(deserializedResponseBody);
Assert.Equal("Write even more tests!", deserializedResponseBody!.Name);
Assert.Equal("With type hierarchies!", deserializedResponseBody!.Child);
}

[Theory]
[MemberData(nameof(PolymorphicResult))]
public async Task RequestDelegateWritesMembersFromChildTypesToJsonResponseBody_WithJsonPolymorphicOptionsAndConfiguredJsonOptions(Delegate @delegate)
{
var httpContext = CreateHttpContext();
httpContext.RequestServices = new ServiceCollection()
.AddSingleton(LoggerFactory)
.AddSingleton(Options.Create(new JsonOptions()))
.BuildServiceProvider();
var responseBodyStream = new MemoryStream();
httpContext.Response.Body = responseBodyStream;

var factoryResult = RequestDelegateFactory.Create(@delegate, new RequestDelegateFactoryOptions { ServiceProvider = httpContext.RequestServices });
var requestDelegate = factoryResult.RequestDelegate;

await requestDelegate(httpContext);

var deserializedResponseBody = JsonSerializer.Deserialize<JsonTodoChild>(responseBodyStream.ToArray(), new JsonSerializerOptions
{
PropertyNameCaseInsensitive = true
});

Assert.NotNull(deserializedResponseBody);
Assert.Equal("Write even more tests!", deserializedResponseBody!.Name);
Assert.Equal("With type hierarchies!", deserializedResponseBody!.Child);
}

[Theory]
[MemberData(nameof(PolymorphicResult))]
public async Task RequestDelegateWritesJsonTypeDiscriminatorToJsonResponseBody_WithJsonPolymorphicOptions(Delegate @delegate)
{
var httpContext = CreateHttpContext();
httpContext.RequestServices = new ServiceCollection()
.AddSingleton(LoggerFactory)
.AddSingleton(Options.Create(new JsonOptions()))
.BuildServiceProvider();

var responseBodyStream = new MemoryStream();
httpContext.Response.Body = responseBodyStream;

var factoryResult = RequestDelegateFactory.Create(@delegate);
var requestDelegate = factoryResult.RequestDelegate;

await requestDelegate(httpContext);

var deserializedResponseBody = JsonNode.Parse(responseBodyStream.ToArray());

Assert.NotNull(deserializedResponseBody);
Assert.NotNull(deserializedResponseBody["$type"]);
Assert.Equal(nameof(JsonTodoChild), deserializedResponseBody["$type"]!.GetValue<string>());
}

public static IEnumerable<object[]> JsonContextActions
{
get
Expand Down Expand Up @@ -3110,6 +3229,22 @@ public async Task RequestDelegateWritesAsJsonResponseBody_WithJsonSerializerCont
Assert.Equal("Write even more tests!", deserializedResponseBody!.Name);
}

[Fact]
public void CreateDelegateThrows_WhenGetJsonTypeInfoFail()
{
var httpContext = CreateHttpContext();
httpContext.RequestServices = new ServiceCollection()
.AddSingleton(LoggerFactory)
.ConfigureHttpJsonOptions(o => o.SerializerOptions.AddContext<TestJsonContext>())
.BuildServiceProvider();

var responseBodyStream = new MemoryStream();
httpContext.Response.Body = responseBodyStream;

TodoStruct TestAction() => new TodoStruct(42, "Bob", true);
Assert.Throws<NotSupportedException>(() => RequestDelegateFactory.Create(TestAction, new() { ServiceProvider = httpContext.RequestServices }));
}

public static IEnumerable<object[]> CustomResults
{
get
Expand Down Expand Up @@ -7526,6 +7661,11 @@ private class TodoChild : Todo
public string? Child { get; set; }
}

private class JsonTodoChild : JsonTodo
{
public string? Child { get; set; }
}

private class CustomTodo : Todo
{
public static async ValueTask<CustomTodo?> BindAsync(HttpContext context, ParameterInfo parameter)
Expand All @@ -7539,6 +7679,8 @@ private class CustomTodo : Todo
}
}

[JsonPolymorphic]
[JsonDerivedType(typeof(JsonTodoChild), nameof(JsonTodoChild))]
private class JsonTodo : Todo
{
public static async ValueTask<JsonTodo?> BindAsync(HttpContext context, ParameterInfo parameter)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
using System.Text;
using System.Text.Encodings.Web;
using System.Text.Json;
using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Http;

namespace Microsoft.AspNetCore.Mvc.Formatters;

Expand All @@ -21,6 +23,8 @@ public SystemTextJsonOutputFormatter(JsonSerializerOptions jsonSerializerOptions
{
SerializerOptions = jsonSerializerOptions;

jsonSerializerOptions.MakeReadOnly();

SupportedEncodings.Add(Encoding.UTF8);
SupportedEncodings.Add(Encoding.Unicode);
SupportedMediaTypes.Add(MediaTypeHeaderValues.ApplicationJson);
Expand Down Expand Up @@ -68,18 +72,27 @@ public sealed override async Task WriteResponseBodyAsync(OutputFormatterWriteCon

var httpContext = context.HttpContext;

// context.ObjectType reflects the declared model type when specified.
// For polymorphic scenarios where the user declares a return type, but returns a derived type,
// we want to serialize all the properties on the derived type. This keeps parity with
// the behavior you get when the user does not declare the return type and with Json.Net at least at the top level.
var objectType = context.Object?.GetType() ?? context.ObjectType ?? typeof(object);
var runtimeType = context.Object?.GetType();
JsonTypeInfo? jsonTypeInfo = null;

if (context.ObjectType is not null)
{
var declaredTypeJsonInfo = SerializerOptions.GetTypeInfo(context.ObjectType);

if (declaredTypeJsonInfo.IsPolymorphicSafe() || context.Object is null || runtimeType == declaredTypeJsonInfo.Type)
{
jsonTypeInfo = declaredTypeJsonInfo;
}
}

jsonTypeInfo ??= SerializerOptions.GetTypeInfo(runtimeType ?? typeof(object));

var responseStream = httpContext.Response.Body;
if (selectedEncoding.CodePage == Encoding.UTF8.CodePage)
{
try
{
await JsonSerializer.SerializeAsync(responseStream, context.Object, objectType, SerializerOptions, httpContext.RequestAborted);
await JsonSerializer.SerializeAsync(responseStream, context.Object, jsonTypeInfo, httpContext.RequestAborted);
await responseStream.FlushAsync(httpContext.RequestAborted);
}
catch (OperationCanceledException) when (context.HttpContext.RequestAborted.IsCancellationRequested) { }
Expand All @@ -93,7 +106,7 @@ public sealed override async Task WriteResponseBodyAsync(OutputFormatterWriteCon
ExceptionDispatchInfo? exceptionDispatchInfo = null;
try
{
await JsonSerializer.SerializeAsync(transcodingStream, context.Object, objectType, SerializerOptions);
await JsonSerializer.SerializeAsync(transcodingStream, context.Object, jsonTypeInfo);
await transcodingStream.FlushAsync();
}
catch (Exception ex)
Expand Down
9 changes: 9 additions & 0 deletions src/Mvc/Mvc.Core/src/JsonOptions.cs
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;
using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Mvc.Formatters;
using Microsoft.AspNetCore.Mvc.ModelBinding;

Expand Down Expand Up @@ -37,5 +38,13 @@ public class JsonOptions
// from deserialization errors that might occur from deeply nested objects.
// This value is the same for model binding and Json.Net's serialization.
MaxDepth = MvcOptions.DefaultMaxModelBindingRecursionDepth,

// 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 calling
// .AddContext<TContext>()
TypeInfoResolver = CreateDefaultTypeResolver()
};

private static IJsonTypeInfoResolver CreateDefaultTypeResolver()
=> new DefaultJsonTypeInfoResolver();
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Microsoft.AspNetCore.Mvc.RouteAttribute</Description>
<Compile Include="$(SharedSourceRoot)MediaType\ReadOnlyMediaTypeHeaderValue.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)MediaType\HttpTokenParsingRule.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)RoutingMetadata\AcceptsMetadata.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)Json\JsonSerializerExtensions.cs" LinkBase="Shared"/>
</ItemGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public async Task WriteResponseBodyAsync_Encodes()
var outputFormatterContext = new OutputFormatterWriteContext(
actionContext.HttpContext,
new TestHttpResponseStreamWriterFactory().CreateWriter,
typeof(string),
typeof(object),
content)
{
ContentType = new StringSegment(mediaType.ToString()),
Expand Down
Loading