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.

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

Stop Calling JSON GetTypeInfo with the runtime type #47859

Merged
merged 6 commits into from
Apr 25, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
Stop Calling JSON GetTypeInfo with the runtime type
Calling GetTypeInfo with the object's runtime type doesn't work when using unspeakable types and using JSON source generation. There is no way to mark the unspeakable type (like a C# compiler implemented IAsyncEnumerable iterator) as JsonSerializable.

Instead, when the "declared type"'s JsonTypeInfo isn't compatible with the runtime type w.r.t. polymorphism, serialize the value "as object", letting System.Text.Json's serialization take over to serialize the value.

Fix #47548
  • Loading branch information
eerhardt committed Apr 24, 2023
commit 2b3789ac4e5587cfeeea985982d0b7fc6c073587
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ private static Func<HttpContext, StringValues> ResolveFromRouteOrQuery(string pa
""";

public static string WriteToResponseAsyncMethod => """
[UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode",
Justification = "<Pending>")]
[UnconditionalSuppressMessage("AOT", "IL3050:RequiresDynamicCode", Justification = "See above.")]
private static Task WriteToResponseAsync<T>(T? value, HttpContext httpContext, JsonTypeInfo<T> jsonTypeInfo, JsonSerializerOptions options)
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
{
var runtimeType = value?.GetType();
Expand All @@ -157,7 +160,7 @@ private static Task WriteToResponseAsync<T>(T? value, HttpContext httpContext, J
return httpContext.Response.WriteAsJsonAsync(value!, jsonTypeInfo);
}

return httpContext.Response.WriteAsJsonAsync(value!, options.GetTypeInfo(runtimeType));
return httpContext.Response.WriteAsJsonAsync<object?>(value, options);
}
""";

Expand Down
14 changes: 6 additions & 8 deletions src/Http/Http.Extensions/src/HttpResponseJsonExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,16 @@ public static partial class HttpResponseJsonExtensions
/// <param name="value">The value to write as JSON.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> used to cancel the operation.</param>
/// <returns>The task object representing the asynchronous operation.</returns>
[RequiresUnreferencedCode(RequiresUnreferencedCodeMessage)]
[RequiresDynamicCode(RequiresDynamicCodeMessage)]
public static Task WriteAsJsonAsync<TValue>(
this HttpResponse response,
TValue value,
CancellationToken cancellationToken = default)
{
ArgumentNullException.ThrowIfNull(response);

var options = ResolveSerializerOptions(response.HttpContext);
return response.WriteAsJsonAsync(value, jsonTypeInfo: (JsonTypeInfo<TValue>)options.GetTypeInfo(typeof(TValue)), contentType: null, cancellationToken);
return response.WriteAsJsonAsync(value, options: null, contentType: null, cancellationToken);
}

/// <summary>
Expand Down Expand Up @@ -108,9 +109,7 @@ public static Task WriteAsJsonAsync<TValue>(
/// <param name="contentType">The content-type to set on the response.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> used to cancel the operation.</param>
/// <returns>The task object representing the asynchronous operation.</returns>
#pragma warning disable RS0026 // Do not add multiple public overloads with optional parameters
Copy link
Member

Choose a reason for hiding this comment

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

Why do these no longer apply?

Copy link
Member Author

Choose a reason for hiding this comment

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

IDK. It builds without them... 🤷

public static Task WriteAsJsonAsync<TValue>(
#pragma warning restore RS0026 // Do not add multiple public overloads with optional parameters
this HttpResponse response,
TValue value,
JsonTypeInfo<TValue> jsonTypeInfo,
Expand Down Expand Up @@ -204,6 +203,8 @@ private static async Task WriteAsJsonAsyncSlow<TValue>(
/// <param name="type">The type of object to write.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> used to cancel the operation.</param>
/// <returns>The task object representing the asynchronous operation.</returns>
[RequiresUnreferencedCode(RequiresUnreferencedCodeMessage)]
[RequiresDynamicCode(RequiresDynamicCodeMessage)]
public static Task WriteAsJsonAsync(
this HttpResponse response,
object? value,
Expand All @@ -212,8 +213,7 @@ public static Task WriteAsJsonAsync(
{
ArgumentNullException.ThrowIfNull(response);

var options = ResolveSerializerOptions(response.HttpContext);
return response.WriteAsJsonAsync(value, jsonTypeInfo: options.GetTypeInfo(type), contentType: null, cancellationToken);
return response.WriteAsJsonAsync(value, type, options: null, contentType: null, cancellationToken);
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
}

/// <summary>
Expand Down Expand Up @@ -302,9 +302,7 @@ private static async Task WriteAsJsonAsyncSlow(
/// <param name="contentType">The content-type to set on the response.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> used to cancel the operation.</param>
/// <returns>The task object representing the asynchronous operation.</returns>
#pragma warning disable RS0026 // Do not add multiple public overloads with optional parameters
public static Task WriteAsJsonAsync(
#pragma warning restore RS0026 // Do not add multiple public overloads with optional parameters
this HttpResponse response,
object? value,
Type type,
Expand Down
62 changes: 61 additions & 1 deletion src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2043,6 +2043,7 @@ public static IEnumerable<object[]> JsonContextActions

[JsonSerializable(typeof(Todo))]
[JsonSerializable(typeof(TodoChild))]
[JsonSerializable(typeof(IAsyncEnumerable<JsonTodo>))]
private partial class TestJsonContext : JsonSerializerContext
{ }

Expand All @@ -2059,7 +2060,8 @@ public async Task RequestDelegateWritesAsJsonResponseBody_WithJsonSerializerCont
var responseBodyStream = new MemoryStream();
httpContext.Response.Body = responseBodyStream;

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

await requestDelegate(httpContext);
Expand All @@ -2073,6 +2075,64 @@ public async Task RequestDelegateWritesAsJsonResponseBody_WithJsonSerializerCont
Assert.Equal("Write even more tests!", deserializedResponseBody!.Name);
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public async Task RequestDelegateWritesAsJsonResponseBody_UnspeakableType(bool useJsonContext)
{
var httpContext = CreateHttpContext();
httpContext.RequestServices = new ServiceCollection()
.AddSingleton(LoggerFactory)
.ConfigureHttpJsonOptions(o => o.SerializerOptions.TypeInfoResolver = useJsonContext ? TestJsonContext.Default : o.SerializerOptions.TypeInfoResolver)
.BuildServiceProvider();

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

Delegate @delegate = IAsyncEnumerable<JsonTodo> () => GetTodosAsync();

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

await requestDelegate(httpContext);

var body = JsonSerializer.Deserialize<JsonTodo[]>(responseBodyStream.ToArray(), new JsonSerializerOptions
{
PropertyNameCaseInsensitive = true
});

Assert.NotNull(body);
Assert.Equal(3, body.Length);

var one = Assert.IsType<JsonTodo>(body[0]);
Assert.Equal(1, one.Id);
Assert.True(one.IsComplete);
Assert.Equal("One", one.Name);

var two = Assert.IsType<JsonTodo>(body[1]);
Assert.Equal(2, two.Id);
Assert.False(two.IsComplete);
Assert.Equal("Two", two.Name);

var three = Assert.IsType<JsonTodoChild>(body[2]);
Assert.Equal(3, three.Id);
Assert.True(three.IsComplete);
Assert.Equal("Three", three.Name);
Assert.Equal("ThreeChild", three.Child);
}

private static async IAsyncEnumerable<JsonTodo> GetTodosAsync()
{
yield return new JsonTodo() { Id = 1, IsComplete = true, Name = "One" };

// ensure this is async
await Task.Yield();

yield return new JsonTodo() { Id = 2, IsComplete = false, Name = "Two" };
yield return new JsonTodoChild() { Id = 3, IsComplete = true, Name = "Three", Child = "ThreeChild" };
}

[Fact]
public void CreateDelegateThrows_WhenGetJsonTypeInfoFail()
{
Expand Down
16 changes: 10 additions & 6 deletions src/Http/Http.Results/src/HttpResultsHelper.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Text;
using System.Text.Json;
using System.Text.Json.Serialization.Metadata;
Expand All @@ -19,6 +20,9 @@ internal static partial class HttpResultsHelper
internal const string DefaultContentType = "text/plain; charset=utf-8";
private static readonly Encoding DefaultEncoding = Encoding.UTF8;

[UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode",
Justification = "<Pending>")]
[UnconditionalSuppressMessage("AOT", "IL3050:RequiresDynamicCode", Justification = "See above.")]
public static Task WriteResultAsJsonAsync<TValue>(
HttpContext httpContext,
ILogger logger,
Expand All @@ -34,8 +38,8 @@ public static Task WriteResultAsJsonAsync<TValue>(
jsonSerializerOptions ??= ResolveJsonOptions(httpContext).SerializerOptions;
var jsonTypeInfo = (JsonTypeInfo<TValue>)jsonSerializerOptions.GetTypeInfo(typeof(TValue));

Type? runtimeType;
if (jsonTypeInfo.IsValid(runtimeType = value.GetType()))
Type? runtimeType = value.GetType();
if (jsonTypeInfo.IsValid(runtimeType))
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
{
Log.WritingResultAsJson(logger, jsonTypeInfo.Type.Name);
return httpContext.Response.WriteAsJsonAsync(
Expand All @@ -46,14 +50,14 @@ public static Task WriteResultAsJsonAsync<TValue>(

Log.WritingResultAsJson(logger, runtimeType.Name);
// Since we don't know the type's polymorphic characteristics
// our best option is use the runtime type, so,
// call WriteAsJsonAsync() with the runtime type to serialize the runtime type rather than the declared type
// our best option is to serialize the value as 'object'.
// call WriteAsJsonAsync<object>() rather than the declared type
// and avoid source generators issues.
// https://github.com/dotnet/aspnetcore/issues/43894
// https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-polymorphism
return httpContext.Response.WriteAsJsonAsync(
return httpContext.Response.WriteAsJsonAsync<object>(
value,
jsonSerializerOptions.GetTypeInfo(runtimeType),
jsonSerializerOptions,
contentType: contentType);
}

Expand Down
56 changes: 55 additions & 1 deletion src/Http/Http.Results/test/HttpResultsHelperTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,58 @@ public async Task WriteResultAsJsonAsync_Works_UsingBaseType_ForChildTypes_WithJ
Assert.Equal("With type hierarchies!", body!.Child);
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public async Task WriteResultAsJsonAsync_Works_UsingUnspeakableType(bool useJsonContext)
{
// Arrange
var value = GetTodosAsync();
var responseBodyStream = new MemoryStream();
var httpContext = CreateHttpContext(responseBodyStream);
var serializerOptions = new JsonOptions().SerializerOptions;

if (useJsonContext)
{
serializerOptions.TypeInfoResolver = TestJsonContext.Default;
}

// Act
await HttpResultsHelper.WriteResultAsJsonAsync(httpContext, NullLogger.Instance, value, jsonSerializerOptions: serializerOptions);

// Assert
var body = JsonSerializer.Deserialize<JsonTodo[]>(responseBodyStream.ToArray(), serializerOptions);

Assert.Equal(3, body.Length);

var one = Assert.IsType<JsonTodo>(body[0]);
Assert.Equal(1, one.Id);
Assert.True(one.IsComplete);
Assert.Equal("One", one.Name);

var two = Assert.IsType<JsonTodo>(body[1]);
Assert.Equal(2, two.Id);
Assert.False(two.IsComplete);
Assert.Equal("Two", two.Name);

var three = Assert.IsType<TodoJsonChild>(body[2]);
Assert.Equal(3, three.Id);
Assert.True(three.IsComplete);
Assert.Equal("Three", three.Name);
Assert.Equal("ThreeChild", three.Child);
}

private static async IAsyncEnumerable<JsonTodo> GetTodosAsync()
{
yield return new JsonTodo() { Id = 1, IsComplete = true, Name = "One" };

// ensure this is async
await Task.Yield();

yield return new JsonTodo() { Id = 2, IsComplete = false, Name = "Two" };
yield return new TodoJsonChild() { Id = 3, IsComplete = true, Name = "Three", Child = "ThreeChild" };
}

private static DefaultHttpContext CreateHttpContext(Stream stream)
=> new()
{
Expand All @@ -198,6 +250,8 @@ private static IServiceProvider CreateServices()
[JsonSerializable(typeof(TodoChild))]
[JsonSerializable(typeof(JsonTodo))]
[JsonSerializable(typeof(TodoStruct))]
[JsonSerializable(typeof(IAsyncEnumerable<JsonTodo>))]
[JsonSerializable(typeof(JsonTodo[]))]
private partial class TestJsonContext : JsonSerializerContext
{ }

Expand All @@ -224,7 +278,7 @@ private class TodoChild : Todo
public string Child { get; set; }
}

[JsonDerivedType(typeof(TodoJsonChild))]
[JsonDerivedType(typeof(TodoJsonChild), nameof(TodoJsonChild))]
private class JsonTodo : Todo
{
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,27 +65,38 @@ public sealed override async Task WriteResponseBodyAsync(OutputFormatterWriteCon

var httpContext = context.HttpContext;

var runtimeType = context.Object?.GetType();
// 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.
// To enable this our best option is to check if the JsonTypeInfo for the declared type is valid,
// if it is use it. If it isn't, serialize the value as 'object' and let JsonSerializer serialize it as necessary.
JsonTypeInfo? jsonTypeInfo = null;

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

var runtimeType = context.Object?.GetType();
if (declaredTypeJsonInfo.IsValid(runtimeType))
{
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, jsonTypeInfo, httpContext.RequestAborted);
if (jsonTypeInfo is not null)
{
await JsonSerializer.SerializeAsync(responseStream, context.Object, jsonTypeInfo, httpContext.RequestAborted);
}
else
{
await JsonSerializer.SerializeAsync(responseStream, context.Object, SerializerOptions, httpContext.RequestAborted);
}

await responseStream.FlushAsync(httpContext.RequestAborted);
}
catch (OperationCanceledException) when (context.HttpContext.RequestAborted.IsCancellationRequested) { }
Expand All @@ -99,7 +110,15 @@ public sealed override async Task WriteResponseBodyAsync(OutputFormatterWriteCon
ExceptionDispatchInfo? exceptionDispatchInfo = null;
try
{
await JsonSerializer.SerializeAsync(transcodingStream, context.Object, jsonTypeInfo);
if (jsonTypeInfo is not null)
{
await JsonSerializer.SerializeAsync(transcodingStream, context.Object, jsonTypeInfo);
}
else
{
await JsonSerializer.SerializeAsync(transcodingStream, context.Object, SerializerOptions);
}

await transcodingStream.FlushAsync();
}
catch (Exception ex)
Expand Down
Loading