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
1 change: 1 addition & 0 deletions eng/Dependencies.props
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ and are generated based on the last package release.
<LatestPackageReference Include="Microsoft.Internal.Runtime.AspNetCore.Transport" />
<LatestPackageReference Include="Microsoft.Bcl.AsyncInterfaces" />
<LatestPackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" />
<LatestPackageReference Include="Microsoft.DotNet.RemoteExecutor" />
<LatestPackageReference Include="Microsoft.EntityFrameworkCore.Design" />
<LatestPackageReference Include="Microsoft.EntityFrameworkCore.InMemory" />
<LatestPackageReference Include="Microsoft.EntityFrameworkCore.Relational" />
Expand Down
4 changes: 4 additions & 0 deletions eng/Version.Details.xml
Original file line number Diff line number Diff line change
Expand Up @@ -319,5 +319,9 @@
<Uri>https://github.com/dotnet/arcade</Uri>
<Sha>1b04d6de502c4108ada6ea8e5ccefdc2ddc3ee7b</Sha>
</Dependency>
<Dependency Name="Microsoft.DotNet.RemoteExecutor" Version="8.0.0-beta.23063.7">
<Uri>https://github.com/dotnet/arcade</Uri>
<Sha>000000</Sha>
</Dependency>
</ToolsetDependencies>
</Dependencies>
1 change: 1 addition & 0 deletions eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@
<!-- Packages from dotnet/arcade -->
<MicrosoftDotNetBuildTasksInstallersVersion>8.0.0-beta.23063.7</MicrosoftDotNetBuildTasksInstallersVersion>
<MicrosoftDotNetBuildTasksTemplatingVersion>8.0.0-beta.23063.7</MicrosoftDotNetBuildTasksTemplatingVersion>
<MicrosoftDotNetRemoteExecutorVersion>8.0.0-beta.23063.7</MicrosoftDotNetRemoteExecutorVersion>
<!-- Packages from dotnet/source-build-externals -->
<MicrosoftSourceBuildIntermediatesourcebuildexternalsVersion>8.0.0-alpha.1.23062.2</MicrosoftSourceBuildIntermediatesourcebuildexternalsVersion>
<!-- Packages from dotnet/xdt -->
Expand Down
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 All @@ -13,8 +12,6 @@ namespace Microsoft.AspNetCore.Mvc;
[JsonConverter(typeof(ProblemDetailsJsonConverter))]
public class ProblemDetails
{
private readonly IDictionary<string, object?> _extensions = new Dictionary<string, object?>(StringComparer.Ordinal);

/// <summary>
/// A URI reference [RFC3986] that identifies the problem type. This specification encourages that, when
/// dereferenced, it provide human-readable documentation for the problem type
Expand Down Expand Up @@ -62,10 +59,5 @@ public class ProblemDetails
/// In particular, complex types or collection types may not round-trip to the original type when using the built-in JSON or XML formatters.
/// </remarks>
[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;
}
public IDictionary<string, object?> Extensions { get; } = new Dictionary<string, object?>(StringComparer.Ordinal);
}
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>
47 changes: 47 additions & 0 deletions src/Http/Http.Extensions/test/JsonOptionsTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Http.Json;
using Microsoft.AspNetCore.Testing;
using Microsoft.DotNet.RemoteExecutor;

namespace Microsoft.AspNetCore.Http.Extensions;

public class JsonOptionsTests
{
[ConditionalFact]
[RemoteExecutionSupported]
public void DefaultSerializerOptions_SetsTypeInfoResolverNull_WhenEnsureJsonTrimmabilityTrue()
{
var options = new RemoteInvokeOptions();
options.RuntimeConfigurationOptions.Add("Microsoft.AspNetCore.EnsureJsonTrimmability", true.ToString());

using var remoteHandle = RemoteExecutor.Invoke(static () =>
{
// Arrange
var options = JsonOptions.DefaultSerializerOptions;

// Assert
Assert.Null(options.TypeInfoResolver);
}, options);
}

[ConditionalFact]
[RemoteExecutionSupported]
public void DefaultSerializerOptions_SetsTypeInfoResolverToDefault_WhenEnsureJsonTrimmabilityFalse()
{
var options = new RemoteInvokeOptions();
options.RuntimeConfigurationOptions.Add("Microsoft.AspNetCore.EnsureJsonTrimmability", false.ToString());

using var remoteHandle = RemoteExecutor.Invoke(static () =>
{
// Arrange
var options = JsonOptions.DefaultSerializerOptions;

// Assert
Assert.NotNull(options.TypeInfoResolver);
Assert.IsType<DefaultJsonTypeInfoResolver>(options.TypeInfoResolver);
}, options);
}
}
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>
Loading