Skip to content

Making JsonOptions AOT/Trimmer-safe with EnsureJsonTrimmability switch #45886

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 11 commits into from
Jan 20, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
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
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// 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.Serialization;
using Microsoft.AspNetCore.Http;

Expand Down Expand Up @@ -64,8 +63,6 @@ public class ProblemDetails
[JsonExtensionData]
public IDictionary<string, object?> Extensions
{
[RequiresUnreferencedCode("JSON serialization and deserialization of ProblemDetails.Extensions might require types that cannot be statically analyzed.")]
[RequiresDynamicCode("JSON serialization and deserialization of ProblemDetails.Extensions might require types that cannot be statically analyzed.")]
get => _extensions;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Text;
using System.Text.Json;
using System.Text.Json.Nodes;
using Microsoft.AspNetCore.Http.Json;
using Microsoft.AspNetCore.Mvc;

Expand Down Expand Up @@ -92,6 +93,39 @@ public void Read_UsingJsonSerializerWorks()
});
}

[Fact]
public void Read_WithUnknownTypeHandling_Works()
{
// Arrange
var type = "https://tools.ietf.org/html/rfc9110#section-15.5.5";
var title = "Not found";
var status = 404;
var detail = "Product not found";
var instance = "http://example.com/products/14";
var traceId = "|37dd3dd5-4a9619f953c40a16.";
var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"detail\":\"{detail}\", \"instance\":\"{instance}\",\"traceId\":\"{traceId}\"}}";
var serializerOptions = new JsonSerializerOptions(JsonSerializerOptions) { UnknownTypeHandling = System.Text.Json.Serialization.JsonUnknownTypeHandling.JsonNode };

// Act
var problemDetails = JsonSerializer.Deserialize<ProblemDetails>(json, serializerOptions);

// Assert
Assert.NotNull(problemDetails);
Assert.Equal(type, problemDetails!.Type);
Assert.Equal(title, problemDetails.Title);
Assert.Equal(status, problemDetails.Status);
Assert.Equal(instance, problemDetails.Instance);
Assert.Equal(detail, problemDetails.Detail);
Assert.Collection(
problemDetails.Extensions,
kvp =>
{
Assert.Equal("traceId", kvp.Key);
Assert.IsAssignableFrom<JsonNode>(kvp.Value!);
Assert.Equal(traceId, kvp.Value?.ToString());
});
}

[Fact]
public void Read_WithSomeMissingValues_Works()
{
Expand Down Expand Up @@ -178,4 +212,36 @@ public void Write_WithSomeMissingContent_Works()
var actual = Encoding.UTF8.GetString(stream.ToArray());
Assert.Equal(expected, actual);
}

[Fact]
public void Write_WithNullExtensionValue_Works()
{
// Arrange
var value = new ProblemDetails
{
Title = "Not found",
Type = "https://tools.ietf.org/html/rfc9110#section-15.5.5",
Status = 404,
Detail = "Product not found",
Instance = "http://example.com/products/14",
Extensions =
{
{ "traceId", null },
{ "some-data", new[] { "value1", "value2" } }
}
};
var expected = $"{{\"type\":\"{JsonEncodedText.Encode(value.Type)}\",\"title\":\"{value.Title}\",\"status\":{value.Status},\"detail\":\"{value.Detail}\",\"instance\":\"{JsonEncodedText.Encode(value.Instance)}\",\"traceId\":null,\"some-data\":[\"value1\",\"value2\"]}}";
var converter = new ProblemDetailsJsonConverter();
var stream = new MemoryStream();

// Act
using (var writer = new Utf8JsonWriter(stream))
{
converter.Write(writer, value, JsonSerializerOptions);
}

// Assert
var actual = Encoding.UTF8.GetString(stream.ToArray());
Assert.Equal(expected, actual);
}
}
14 changes: 8 additions & 6 deletions src/Http/Http.Extensions/src/HttpRequestJsonExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,14 @@ public static class HttpRequestJsonExtensions
/// <param name="request">The request to read from.</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)]
Copy link
Member

Choose a reason for hiding this comment

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

Why havne't we done this for all methods in this class? For example,

    public static async ValueTask<TValue?> ReadFromJsonAsync<TValue>(
        this HttpRequest request,
        JsonSerializerOptions? options,
        CancellationToken cancellationToken = default)

Copy link
Member

Choose a reason for hiding this comment

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

Similar question for HttpResponseJsonExtensions.

Copy link
Member Author

@brunolins16 brunolins16 Jan 18, 2023

Choose a reason for hiding this comment

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

When the user provides a JsonSerializerOptions the TypeInfoResolver could be null, default value, and we cannot call GetTypeInfo dotnet/runtime#80529

[RequiresDynamicCode(RequiresDynamicCodeMessage)]
public static ValueTask<TValue?> ReadFromJsonAsync<TValue>(
this HttpRequest request,
CancellationToken cancellationToken = default)
{
return request.ReadFromJsonAsync<TValue>(options: null, cancellationToken);
ArgumentNullException.ThrowIfNull(request);

var options = ResolveSerializerOptions(request.HttpContext);
return request.ReadFromJsonAsync(jsonTypeInfo: (JsonTypeInfo<TValue>)options.GetTypeInfo(typeof(TValue)), cancellationToken);
}

/// <summary>
Expand Down Expand Up @@ -166,14 +167,15 @@ public static class HttpRequestJsonExtensions
/// <param name="type">The type of object to read.</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 ValueTask<object?> ReadFromJsonAsync(
this HttpRequest request,
Type type,
CancellationToken cancellationToken = default)
{
return request.ReadFromJsonAsync(type, options: null, cancellationToken);
ArgumentNullException.ThrowIfNull(request);

var options = ResolveSerializerOptions(request.HttpContext);
return request.ReadFromJsonAsync(jsonTypeInfo: options.GetTypeInfo(type), cancellationToken);
}

/// <summary>
Expand Down
14 changes: 8 additions & 6 deletions src/Http/Http.Extensions/src/HttpResponseJsonExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ 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)
{
return response.WriteAsJsonAsync(value, options: null, contentType: null, cancellationToken);
ArgumentNullException.ThrowIfNull(response);

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

/// <summary>
Expand Down Expand Up @@ -203,15 +204,16 @@ 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,
Type type,
CancellationToken cancellationToken = default)
{
return response.WriteAsJsonAsync(value, type, options: null, contentType: null, cancellationToken);
ArgumentNullException.ThrowIfNull(response);

var options = ResolveSerializerOptions(response.HttpContext);
return response.WriteAsJsonAsync(value, jsonTypeInfo: options.GetTypeInfo(type), contentType: null, cancellationToken);
}

/// <summary>
Expand Down
3 changes: 2 additions & 1 deletion src/Http/Http.Extensions/src/JsonOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Text.Encodings.Web;
using System.Text.Json;
using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Internal;

#nullable enable

Expand All @@ -26,7 +27,7 @@ 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 calling
// .AddContext<TContext>()
TypeInfoResolver = CreateDefaultTypeResolver()
TypeInfoResolver = TrimmingAppContextSwitches.EnsureJsonTrimmability ? null : CreateDefaultTypeResolver()
};

// Use a copy so the defaults are not modified.
Copy link
Member

Choose a reason for hiding this comment

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

What's the plan for the suppressions on line 39-44?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, they will suppress in the Microsoft.AspNetCore.Http.Extensions.WarningSuppressions.xml and I will file an issue to start a discussion if we need something similar to runtime (LibraryBuild.xml).

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,18 @@
</PropertyGroup>

<ItemGroup>
<Compile Include="$(SharedSourceRoot)ObjectMethodExecutor\**\*.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)ParameterBindingMethodCache.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)EndpointMetadataPopulator.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)PropertyAsParameterInfo.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)ObjectMethodExecutor\**\*.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ParameterBindingMethodCache.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)EndpointMetadataPopulator.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)PropertyAsParameterInfo.cs" LinkBase="Shared" />
<Compile Include="..\..\Shared\StreamCopyOperationInternal.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ApiExplorerTypes\*.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)RoutingMetadata\AcceptsMetadata.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)TypeNameHelper\TypeNameHelper.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)TypeNameHelper\TypeNameHelper.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ProblemDetails\ProblemDetailsDefaults.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ValueStringBuilder\**\*.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)Json\JsonSerializerExtensions.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)ValueStringBuilder\**\*.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)Json\JsonSerializerExtensions.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)TrimmingAppContextSwitches.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)RouteHandlers\ExecuteHandlerHelper.cs" LinkBase="Shared"/>
</ItemGroup>

Expand All @@ -36,4 +37,5 @@
<ItemGroup>
<InternalsVisibleTo Include="Microsoft.AspNetCore.Http.Extensions.Tests" />
</ItemGroup>

</Project>
2 changes: 2 additions & 0 deletions src/Http/Http.Extensions/src/ProblemDetailsJsonContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ namespace Microsoft.AspNetCore.Http;

[JsonSerializable(typeof(ProblemDetails))]
[JsonSerializable(typeof(HttpValidationProblemDetails))]
// ExtensionData
[JsonSerializable(typeof(IDictionary<string, object?>))]
// Additional values are specified on JsonSerializerContext to support some values for extensions.
// For example, the DeveloperExceptionMiddleware serializes its complex type to JsonElement, which problem details then needs to serialize.
[JsonSerializable(typeof(JsonElement))]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="utf-8" ?>
<linker>
<assembly fullname="Microsoft.AspNetCore.Http.Extensions">
<type fullname="Microsoft.AspNetCore.Internal.TrimmingAppContextSwitches">
<method signature="System.Boolean get_EnsureJsonTrimmability()" body="stub" value="true" feature="Microsoft.AspNetCore.EnsureJsonTrimmability" featurevalue="true" />
</type>
</assembly>
</linker>
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,6 @@ await _problemDetailsService.WriteAsync(new()
}
}

[UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "Values set on ProblemDetails.Extensions are supported by the default writer.")]
[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "Values set on ProblemDetails.Extensions are supported by the default writer.")]
private ProblemDetails CreateProblemDetails(ErrorContext errorContext, HttpContext httpContext)
{
var problemDetails = new ProblemDetails
Expand Down
3 changes: 2 additions & 1 deletion src/Mvc/Mvc.Core/src/JsonOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Text.Json;
using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Internal;
using Microsoft.AspNetCore.Mvc.Formatters;
using Microsoft.AspNetCore.Mvc.ModelBinding;

Expand Down Expand Up @@ -42,7 +43,7 @@ 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 calling
// .AddContext<TContext>()
TypeInfoResolver = CreateDefaultTypeResolver()
TypeInfoResolver = TrimmingAppContextSwitches.EnsureJsonTrimmability ? null : CreateDefaultTypeResolver()
};

private static IJsonTypeInfoResolver CreateDefaultTypeResolver()
Expand Down
5 changes: 5 additions & 0 deletions src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj
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)TrimmingAppContextSwitches.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)Json\JsonSerializerExtensions.cs" LinkBase="Shared"/>
</ItemGroup>

Expand Down Expand Up @@ -61,6 +62,10 @@ Microsoft.AspNetCore.Mvc.RouteAttribute</Description>
<Compile Include="$(SharedSourceRoot)ParameterDefaultValue\*.cs" />
</ItemGroup>

<ItemGroup>
<EmbeddedResource Include="Properties\ILLink.Substitutions.xml" LogicalName="ILLink.Substitutions.xml" />
</ItemGroup>

<ItemGroup>
<InternalsVisibleTo Include="Microsoft.AspNetCore.Mvc" />
<InternalsVisibleTo Include="Microsoft.AspNetCore.Mvc.ApiExplorer" />
Expand Down
8 changes: 8 additions & 0 deletions src/Mvc/Mvc.Core/src/Properties/ILLink.Substitutions.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="utf-8" ?>
<linker>
<assembly fullname="Microsoft.AspNetCore.Mvc.Core">
<type fullname="Microsoft.AspNetCore.Internal.TrimmingAppContextSwitches">
<method signature="System.Boolean get_EnsureJsonTrimmability()" body="stub" value="true" feature="Microsoft.AspNetCore.EnsureJsonTrimmability" featurevalue="true" />
</type>
</assembly>
</linker>
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.Nodes;
using System.Text.Json.Serialization;

namespace Microsoft.AspNetCore.Http;
Expand All @@ -23,6 +24,7 @@ public static HttpValidationProblemDetails ReadProblemDetails(ref Utf8JsonReader
throw new JsonException("Unexpected end when reading JSON.");
}

var objectTypeInfo = options.GetTypeInfo(typeof(object));
while (reader.Read() && reader.TokenType != JsonTokenType.EndObject)
{
if (reader.ValueTextEquals(Errors.EncodedUtf8Bytes))
Expand All @@ -31,7 +33,7 @@ public static HttpValidationProblemDetails ReadProblemDetails(ref Utf8JsonReader
}
else
{
ProblemDetailsJsonConverter.ReadValue(ref reader, problemDetails, options);
ProblemDetailsJsonConverter.ReadValue(ref reader, problemDetails, objectTypeInfo);
}
}

Expand Down
33 changes: 14 additions & 19 deletions src/Shared/ProblemDetails/ProblemDetailsJsonConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Mvc;

namespace Microsoft.AspNetCore.Http;
Expand All @@ -25,9 +26,10 @@ public override ProblemDetails Read(ref Utf8JsonReader reader, Type typeToConver
throw new JsonException("Unexpected end when reading JSON.");
}

var objectTypeInfo = options.GetTypeInfo(typeof(object));
while (reader.Read() && reader.TokenType != JsonTokenType.EndObject)
{
ReadValue(ref reader, problemDetails, options);
ReadValue(ref reader, problemDetails, objectTypeInfo);
}

if (reader.TokenType != JsonTokenType.EndObject)
Expand All @@ -45,7 +47,7 @@ public override void Write(Utf8JsonWriter writer, ProblemDetails value, JsonSeri
writer.WriteEndObject();
}

internal static void ReadValue(ref Utf8JsonReader reader, ProblemDetails value, JsonSerializerOptions options)
internal static void ReadValue(ref Utf8JsonReader reader, ProblemDetails value, JsonTypeInfo extensionDataTypeInfo)
{
if (TryReadStringProperty(ref reader, Type, out var propertyValue))
{
Expand Down Expand Up @@ -79,14 +81,7 @@ internal static void ReadValue(ref Utf8JsonReader reader, ProblemDetails value,
{
var key = reader.GetString()!;
reader.Read();
ReadExtension(value, key, ref reader, options);
}

[UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "ProblemDetails.Extensions is annotated to expose this warning to callers.")]
[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "ProblemDetails.Extensions is annotated to expose this warning to callers.")]
static void ReadExtension(ProblemDetails problemDetails, string key, ref Utf8JsonReader reader, JsonSerializerOptions options)
{
problemDetails.Extensions[key] = JsonSerializer.Deserialize(ref reader, typeof(object), options);
value.Extensions[key] = JsonSerializer.Deserialize(ref reader, extensionDataTypeInfo);
}
}

Expand Down Expand Up @@ -130,17 +125,17 @@ internal static void WriteProblemDetails(Utf8JsonWriter writer, ProblemDetails v
writer.WriteString(Instance, value.Instance);
}

WriteExtensions(value, writer, options);

[UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "ProblemDetails.Extensions is annotated to expose this warning to callers.")]
[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "ProblemDetails.Extensions is annotated to expose this warning to callers.")]
static void WriteExtensions(ProblemDetails problemDetails, Utf8JsonWriter writer, JsonSerializerOptions options)
foreach (var kvp in value.Extensions)
{
foreach (var kvp in problemDetails.Extensions)
writer.WritePropertyName(kvp.Key);

if (kvp.Value is null)
{
writer.WriteNullValue();
}
else
{
writer.WritePropertyName(kvp.Key);
// When AOT is enabled, Serialize will only work with values specified on the JsonContext.
JsonSerializer.Serialize(writer, kvp.Value, kvp.Value?.GetType() ?? typeof(object), options);
JsonSerializer.Serialize(writer, kvp.Value, options.GetTypeInfo(kvp.Value.GetType()));
}
}
}
Expand Down
Loading