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

Adding STJ Polymorphism to Result Types #46008

Merged
merged 33 commits into from
Feb 1, 2023
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
2143f21
Adding STJ Polymorphism to Result Types
brunolins16 Jan 9, 2023
0c87d13
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 10, 2023
1e048d3
Renaming unittest
brunolins16 Jan 10, 2023
7485673
Adding unit tests
brunolins16 Jan 10, 2023
af1eba8
Adding more unit tests
brunolins16 Jan 10, 2023
84e1c77
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 12, 2023
4a63010
Setting DefaultTypeInfoResolver
brunolins16 Jan 12, 2023
543c338
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 12, 2023
cfbe2fd
Removing ISTrimmable
brunolins16 Jan 12, 2023
8c10b8c
Removing cache
brunolins16 Jan 12, 2023
cabe9c5
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 13, 2023
a4229b3
Clean up
brunolins16 Jan 13, 2023
2be89fc
Avoiding multiple GetTypeInfo calls
brunolins16 Jan 17, 2023
c9ddbea
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 17, 2023
8c09b97
Fixing JsonResult
brunolins16 Jan 18, 2023
52c3b9d
Clean up
brunolins16 Jan 19, 2023
3d7a9f9
clean up
brunolins16 Jan 20, 2023
891d909
Adding Json apis proposal
brunolins16 Jan 20, 2023
1fb0405
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 20, 2023
e854667
Removing name change
brunolins16 Jan 25, 2023
990498c
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 25, 2023
a2a972a
Fixing bad merge
brunolins16 Jan 25, 2023
ae9c2a4
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Jan 27, 2023
c2768f4
Fix build
brunolins16 Jan 27, 2023
610c7b1
PR review
brunolins16 Jan 27, 2023
1930364
PR Feedback
brunolins16 Jan 27, 2023
3b54e1a
Update for the approved API
brunolins16 Jan 30, 2023
205c6c6
PR review
brunolins16 Jan 31, 2023
fba0039
Update TypedResultsTests.cs
brunolins16 Jan 31, 2023
e39d168
Changing IsPolymorphicSafe
brunolins16 Feb 1, 2023
baf9036
Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…
brunolins16 Feb 1, 2023
9df633e
Fixing notnull annotation
brunolins16 Feb 1, 2023
a070575
Remove blank line
brunolins16 Feb 1, 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
6 changes: 3 additions & 3 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1055,7 +1055,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
{
var jsonTypeInfo = factoryContext.JsonSerializerOptions.GetReadOnlyTypeInfo(typeArg);

if (jsonTypeInfo.IsPolymorphicSafe() == true)
if (jsonTypeInfo.HasKnownPolymorphism())
{
return Expression.Call(
ExecuteTaskOfTFastMethod.MakeGenericMethod(typeArg),
Expand Down Expand Up @@ -1096,7 +1096,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
{
var jsonTypeInfo = factoryContext.JsonSerializerOptions.GetReadOnlyTypeInfo(typeArg);

if (jsonTypeInfo.IsPolymorphicSafe() == true)
if (jsonTypeInfo.HasKnownPolymorphism())
{
return Expression.Call(
ExecuteValueTaskOfTFastMethod.MakeGenericMethod(typeArg),
Expand Down Expand Up @@ -1140,7 +1140,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
{
var jsonTypeInfo = factoryContext.JsonSerializerOptions.GetReadOnlyTypeInfo(returnType);

if (jsonTypeInfo.IsPolymorphicSafe() == true)
if (jsonTypeInfo.HasKnownPolymorphism())
{
return Expression.Call(
JsonResultWriteResponseOfTFastAsyncMethod.MakeGenericMethod(returnType),
Expand Down
50 changes: 27 additions & 23 deletions src/Http/Http.Results/src/HttpResultsHelper.cs
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
// 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;
using Microsoft.AspNetCore.Http.Json;
using Microsoft.AspNetCore.Internal;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Microsoft.Net.Http.Headers;

namespace Microsoft.AspNetCore.Http;
Expand All @@ -16,13 +19,10 @@ internal static partial class HttpResultsHelper
internal const string DefaultContentType = "text/plain; charset=utf-8";
private static readonly Encoding DefaultEncoding = Encoding.UTF8;

// Remove once https://github.com/dotnet/aspnetcore/pull/46008 is done.
[UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", Justification = "<Pending>")]
[UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "<Pending>")]
public static Task WriteResultAsJsonAsync<T>(
public static Task WriteResultAsJsonAsync<TValue>(
HttpContext httpContext,
ILogger logger,
T? value,
TValue? value,
string? contentType = null,
JsonSerializerOptions? jsonSerializerOptions = null)
{
Expand All @@ -31,32 +31,30 @@ public static Task WriteResultAsJsonAsync<T>(
return Task.CompletedTask;
}

var declaredType = typeof(T);
if (declaredType.IsValueType)
{
Log.WritingResultAsJson(logger, declaredType.Name);
jsonSerializerOptions ??= ResolveJsonOptions(httpContext).SerializerOptions;
var jsonTypeInfo = (JsonTypeInfo<TValue>)jsonSerializerOptions.GetTypeInfo(typeof(TValue));

// In this case the polymorphism is not
// relevant and we don't need to box.
Type? runtimeType;
if (jsonTypeInfo.IsValid(runtimeType = value.GetType()))
{
Log.WritingResultAsJson(logger, jsonTypeInfo.Type.Name);
return httpContext.Response.WriteAsJsonAsync(
value,
options: jsonSerializerOptions,
contentType: contentType);
value,
jsonTypeInfo,
contentType: contentType);
}

var runtimeType = value.GetType();

Log.WritingResultAsJson(logger, runtimeType.Name);

// Call WriteAsJsonAsync() with the runtime type to serialize the runtime type rather than the declared type
// 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
// 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(
value,
runtimeType,
options: jsonSerializerOptions,
contentType: contentType);
value,
jsonSerializerOptions.GetTypeInfo(runtimeType),
contentType: contentType);
}

public static Task WriteResultAsContentAsync(
Expand Down Expand Up @@ -146,6 +144,12 @@ public static void ApplyProblemDetailsDefaultsIfNeeded(object? value, int? statu
}
}

private static JsonOptions ResolveJsonOptions(HttpContext httpContext)
{
// Attempt to resolve options from DI then fallback to default options
return httpContext.RequestServices.GetService<IOptions<JsonOptions>>()?.Value ?? new JsonOptions();
}

internal static partial class Log
{
[LoggerMessage(1, LogLevel.Information,
Expand Down
77 changes: 54 additions & 23 deletions src/Http/Http.Results/src/JsonHttpResultOfT.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// 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.Json;
using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
Expand All @@ -18,33 +20,33 @@ public sealed partial class JsonHttpResult<TValue> : IResult, IStatusCodeHttpRes
/// </summary>
/// <param name="value">The value to format in the entity body.</param>
/// <param name="jsonSerializerOptions">The serializer settings.</param>
internal JsonHttpResult(TValue? value, JsonSerializerOptions? jsonSerializerOptions)
: this(value, statusCode: null, contentType: null, jsonSerializerOptions: jsonSerializerOptions)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="Json"/> class with the values.
/// </summary>
/// <param name="value">The value to format in the entity body.</param>
/// <param name="statusCode">The HTTP status code of the response.</param>
/// <param name="jsonSerializerOptions">The serializer settings.</param>
internal JsonHttpResult(TValue? value, int? statusCode, JsonSerializerOptions? jsonSerializerOptions)
: this(value, statusCode: statusCode, contentType: null, jsonSerializerOptions: jsonSerializerOptions)
/// <param name="contentType">The value for the <c>Content-Type</c> header</param>
[RequiresDynamicCode(JsonHttpResultTrimmerWarning.SerializationRequiresDynamicCodeMessage)]
[RequiresUnreferencedCode(JsonHttpResultTrimmerWarning.SerializationUnreferencedCodeMessage)]
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

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

Remind me why this is unsafe after the other work we did to make the underlying JSON APIs that these call into safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is unsafe because it is the scenario where the user provides the options, could be resolved by DI or not, so, we need to check and initialize for reflection.

internal JsonHttpResult(TValue? value, JsonSerializerOptions? jsonSerializerOptions, int? statusCode = null, string? contentType = null)
{
Value = value;
ContentType = contentType;

if (value is ProblemDetails problemDetails)
{
ProblemDetailsDefaults.Apply(problemDetails, statusCode);
statusCode ??= problemDetails.Status;
}
StatusCode = statusCode;

if (jsonSerializerOptions is not null && !jsonSerializerOptions.IsReadOnly)
{
jsonSerializerOptions.TypeInfoResolver ??= new DefaultJsonTypeInfoResolver();
}

JsonSerializerOptions = jsonSerializerOptions;
}

/// <summary>
/// Initializes a new instance of the <see cref="Json"/> class with the values.
/// </summary>
/// <param name="value">The value to format in the entity body.</param>
/// <param name="statusCode">The HTTP status code of the response.</param>
/// <param name="jsonSerializerOptions">The serializer settings.</param>
/// <param name="contentType">The value for the <c>Content-Type</c> header</param>
internal JsonHttpResult(TValue? value, int? statusCode, string? contentType, JsonSerializerOptions? jsonSerializerOptions)
internal JsonHttpResult(TValue? value, int? statusCode = null, string? contentType = null)
{
Value = value;
JsonSerializerOptions = jsonSerializerOptions;
ContentType = contentType;

if (value is ProblemDetails problemDetails)
Expand All @@ -59,7 +61,12 @@ internal JsonHttpResult(TValue? value, int? statusCode, string? contentType, Jso
/// <summary>
/// Gets or sets the serializer settings.
/// </summary>
public JsonSerializerOptions? JsonSerializerOptions { get; internal init; }
public JsonSerializerOptions? JsonSerializerOptions { get; }

/// <summary>
/// Gets or sets the serializer settings.
/// </summary>
internal JsonTypeInfo? JsonTypeInfo { get; init; }

/// <summary>
/// Gets the object result.
Expand All @@ -71,7 +78,7 @@ internal JsonHttpResult(TValue? value, int? statusCode, string? contentType, Jso
/// <summary>
/// Gets the value for the <c>Content-Type</c> header.
/// </summary>
public string? ContentType { get; internal set; }
public string? ContentType { get; }

/// <summary>
/// Gets the HTTP status code.
Expand All @@ -93,6 +100,30 @@ public Task ExecuteAsync(HttpContext httpContext)
httpContext.Response.StatusCode = statusCode;
}

if (Value is null)
{
return Task.CompletedTask;
}

if (JsonTypeInfo != null)
{
HttpResultsHelper.Log.WritingResultAsJson(logger, JsonTypeInfo.Type.Name);

if (JsonTypeInfo is JsonTypeInfo<TValue> typedJsonTypeInfo)
{
// We don't need to box here.
return httpContext.Response.WriteAsJsonAsync(
Value,
typedJsonTypeInfo,
contentType: ContentType);

}
return httpContext.Response.WriteAsJsonAsync(
Value,
JsonTypeInfo,
contentType: ContentType);
}

return HttpResultsHelper.WriteResultAsJsonAsync(
httpContext,
logger,
Expand Down
11 changes: 11 additions & 0 deletions src/Http/Http.Results/src/JsonHttpResultTrimmerWarning.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Http;

internal class JsonHttpResultTrimmerWarning
{
public const string SerializationUnreferencedCodeMessage = "JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext.";
public const string SerializationRequiresDynamicCodeMessage = "JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use the overload that takes a JsonTypeInfo or JsonSerializerContext.";

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
<Compile Include="$(SharedSourceRoot)ProblemDetails\ProblemDetailsDefaults.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ApiExplorerTypes\*.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)RoutingMetadata\AcceptsMetadata.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)Json\JsonSerializerExtensions.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)RouteValueDictionaryTrimmerWarning.cs" LinkBase="Shared" />
</ItemGroup>

Expand Down
8 changes: 7 additions & 1 deletion src/Http/Http.Results/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ static Microsoft.AspNetCore.Http.Results.Created(string? uri, object? value) ->
static Microsoft.AspNetCore.Http.Results.Created(System.Uri? uri, object? value) -> Microsoft.AspNetCore.Http.IResult!
static Microsoft.AspNetCore.Http.Results.Created<TValue>(string? uri, TValue? value) -> Microsoft.AspNetCore.Http.IResult!
static Microsoft.AspNetCore.Http.Results.Created<TValue>(System.Uri? uri, TValue? value) -> Microsoft.AspNetCore.Http.IResult!
static Microsoft.AspNetCore.Http.Results.Json(object? data, System.Text.Json.Serialization.Metadata.JsonTypeInfo! jsonTypeInfo, string? contentType = null, int? statusCode = null) -> Microsoft.AspNetCore.Http.IResult!
static Microsoft.AspNetCore.Http.Results.Json(object? data, System.Type! type, System.Text.Json.Serialization.JsonSerializerContext! context, string? contentType = null, int? statusCode = null) -> Microsoft.AspNetCore.Http.IResult!
static Microsoft.AspNetCore.Http.Results.Json<TValue>(TValue? data, System.Text.Json.Serialization.JsonSerializerContext! context, string? contentType = null, int? statusCode = null) -> Microsoft.AspNetCore.Http.IResult!
static Microsoft.AspNetCore.Http.Results.Json<TValue>(TValue? data, System.Text.Json.Serialization.Metadata.JsonTypeInfo<TValue>! jsonTypeInfo, string? contentType = null, int? statusCode = null) -> Microsoft.AspNetCore.Http.IResult!
static Microsoft.AspNetCore.Http.TypedResults.Created() -> Microsoft.AspNetCore.Http.HttpResults.Created!
static Microsoft.AspNetCore.Http.TypedResults.Created(string? uri) -> Microsoft.AspNetCore.Http.HttpResults.Created!
static Microsoft.AspNetCore.Http.TypedResults.Created(System.Uri? uri) -> Microsoft.AspNetCore.Http.HttpResults.Created!
Expand All @@ -22,4 +26,6 @@ static Microsoft.AspNetCore.Http.TypedResults.Created<TValue>(System.Uri? uri, T
*REMOVED*static Microsoft.AspNetCore.Http.TypedResults.Created(System.Uri! uri) -> Microsoft.AspNetCore.Http.HttpResults.Created!
*REMOVED*static Microsoft.AspNetCore.Http.TypedResults.Created(string! uri) -> Microsoft.AspNetCore.Http.HttpResults.Created!
*REMOVED*static Microsoft.AspNetCore.Http.TypedResults.Created<TValue>(System.Uri! uri, TValue? value) -> Microsoft.AspNetCore.Http.HttpResults.Created<TValue>!
*REMOVED*static Microsoft.AspNetCore.Http.TypedResults.Created<TValue>(string! uri, TValue? value) -> Microsoft.AspNetCore.Http.HttpResults.Created<TValue>!
*REMOVED*static Microsoft.AspNetCore.Http.TypedResults.Created<TValue>(string! uri, TValue? value) -> Microsoft.AspNetCore.Http.HttpResults.Created<TValue>!
static Microsoft.AspNetCore.Http.TypedResults.Json<TValue>(TValue? data, System.Text.Json.Serialization.JsonSerializerContext! context, string? contentType = null, int? statusCode = null) -> Microsoft.AspNetCore.Http.HttpResults.JsonHttpResult<TValue>!
static Microsoft.AspNetCore.Http.TypedResults.Json<TValue>(TValue? data, System.Text.Json.Serialization.Metadata.JsonTypeInfo<TValue>! jsonTypeInfo, string? contentType = null, int? statusCode = null) -> Microsoft.AspNetCore.Http.HttpResults.JsonHttpResult<TValue>!
Loading