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

[AOT] Fixing the problems with ProblemDetails #45646

Merged
merged 9 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from 7 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
10 changes: 9 additions & 1 deletion src/Http/Http.Abstractions/src/ProblemDetails/ProblemDetails.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.Json.Serialization;
using Microsoft.AspNetCore.Http;

Expand All @@ -12,6 +13,8 @@ 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 @@ -59,5 +62,10 @@ 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 { get; } = new Dictionary<string, object?>(StringComparer.Ordinal);
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 @@ -11,6 +11,40 @@ public class HttpValidationProblemDetailsJsonConverterTest
{
private static JsonSerializerOptions JsonSerializerOptions => new JsonOptions().SerializerOptions;

[Fact]
public void Write_Works()
{
var converter = new HttpValidationProblemDetailsJsonConverter();
var problemDetails = new HttpValidationProblemDetails();

problemDetails.Type = "https://tools.ietf.org/html/rfc9110#section-15.5.5";
problemDetails.Title = "Not found";
problemDetails.Status = 404;
problemDetails.Detail = "Product not found";
problemDetails.Instance = "http://example.com/products/14";
problemDetails.Extensions["traceId"] = "|37dd3dd5-4a9619f953c40a16.";
problemDetails.Errors.Add("key0", new[] { "error0" });
problemDetails.Errors.Add("key1", new[] { "error1", "error2" });

var ms = new MemoryStream();
var writer = new Utf8JsonWriter(ms);
converter.Write(writer, problemDetails, JsonSerializerOptions);
writer.Flush();

ms.Seek(0, SeekOrigin.Begin);
var document = JsonDocument.Parse(ms);
Assert.Equal(problemDetails.Type, document.RootElement.GetProperty("type").GetString());
Assert.Equal(problemDetails.Title, document.RootElement.GetProperty("title").GetString());
Assert.Equal(problemDetails.Status, document.RootElement.GetProperty("status").GetInt32());
Assert.Equal(problemDetails.Detail, document.RootElement.GetProperty("detail").GetString());
Assert.Equal(problemDetails.Instance, document.RootElement.GetProperty("instance").GetString());
Assert.Equal((string)problemDetails.Extensions["traceId"]!, document.RootElement.GetProperty("traceId").GetString());
var errorsElement = document.RootElement.GetProperty("errors");
Assert.Equal("error0", errorsElement.GetProperty("key0")[0].GetString());
Assert.Equal("error1", errorsElement.GetProperty("key1")[0].GetString());
Assert.Equal("error2", errorsElement.GetProperty("key1")[1].GetString());
}

[Fact]
public void Read_Works()
{
Expand Down
31 changes: 24 additions & 7 deletions src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Text.Json;
using System.Text.Json.Serialization;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Options;
Expand Down Expand Up @@ -43,20 +45,20 @@ public bool CanWrite(ProblemDetailsContext context)
}

[UnconditionalSuppressMessage("Trimming", "IL2026",
Justification = "JSON serialization of ProblemDetails.Extensions might require types that cannot be statically analyzed and we need to fallback" +
"to reflection-based. The ProblemDetailsConverter is marked as RequiresUnreferencedCode already.")]
Justification = "JSON serialization of ProblemDetails.Extensions might require types that cannot be statically analyzed. The property is annotated with a warning")]
[UnconditionalSuppressMessage("Trimming", "IL3050",
Justification = "JSON serialization of ProblemDetails.Extensions might require types that cannot be statically analyzed and we need to fallback" +
"to reflection-based. The ProblemDetailsConverter is marked as RequiresDynamicCode already.")]
Justification = "JSON serialization of ProblemDetails.Extensions might require types that cannot be statically analyzed. The property is annotated with a warning")]
public ValueTask WriteAsync(ProblemDetailsContext context)
{
var httpContext = context.HttpContext;
ProblemDetailsDefaults.Apply(context.ProblemDetails, httpContext.Response.StatusCode);
_options.CustomizeProblemDetails?.Invoke(context);

if (context.ProblemDetails.Extensions is { Count: 0 })
// Use source generation serialization in two scenarios:
// 1. There are no extensions. Source generation is faster and works well with trimming.
// 2. Native AOT. In this case only the data types specified on ProblemDetailsJsonContext will work.
if (context.ProblemDetails.Extensions is { Count: 0 } || !RuntimeFeature.IsDynamicCodeSupported)
Copy link
Member Author

Choose a reason for hiding this comment

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

@eerhardt @jkotas @vitek-karas @davidfowl Like #45604 (comment), this is a place where dotnet/runtime#39806 would be useful.

Developers being able to run the "AOT path" while writing and debugging their app would help them find incompatible extension values during development instead of when they publish as AOT for production.

Copy link
Member

Choose a reason for hiding this comment

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

Would it fail to serialize if we did that in "build"? Or would the reflection based serializer kick in?

Copy link
Member

Choose a reason for hiding this comment

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

It would fail at runtime. This is what we're going to be doing manually today for AOT based JSON in ASP.NET Core. The reflection fallback would be disabled and our calls to resolve the JsonTypeInfo would return null.

Copy link
Member

Choose a reason for hiding this comment

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

In that case - let's do dotnet/runtime#39806.
We just need to "hide" it enough so that people don't use it as the first option to solve AOT problems (still better than current behavior, but ideally we want them to really fix things)

Copy link
Member

Choose a reason for hiding this comment

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

This would probably be a good candidate for an "AOT-specific" test, similar to how we test trimming and AOT in dotnet/runtime. For example: https://github.com/dotnet/runtime/tree/main/src/libraries/System.Diagnostics.DiagnosticSource/tests/NativeAotTests.

{
// We can use the source generation in this case
return new ValueTask(httpContext.Response.WriteAsJsonAsync(
context.ProblemDetails,
ProblemDetailsJsonContext.Default.ProblemDetails,
Expand All @@ -69,7 +71,22 @@ public ValueTask WriteAsync(ProblemDetailsContext context)
contentType: "application/problem+json"));
}

// 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(ProblemDetails))]
[JsonSerializable(typeof(JsonElement))]
[JsonSerializable(typeof(string))]
[JsonSerializable(typeof(decimal))]
[JsonSerializable(typeof(float))]
[JsonSerializable(typeof(double))]
[JsonSerializable(typeof(int))]
[JsonSerializable(typeof(long))]
[JsonSerializable(typeof(Guid))]
[JsonSerializable(typeof(Uri))]
[JsonSerializable(typeof(TimeSpan))]
[JsonSerializable(typeof(DateTime))]
[JsonSerializable(typeof(DateTimeOffset))]
internal sealed partial class ProblemDetailsJsonContext : JsonSerializerContext
{ }
{
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Text;
using System.Text.Json;
using System.Text.Json.Serialization;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Diagnostics.RazorViews;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Http.Json;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.FileProviders;
Expand All @@ -34,6 +37,7 @@ internal class DeveloperExceptionPageMiddlewareImpl
private readonly ExceptionDetailsProvider _exceptionDetailsProvider;
private readonly Func<ErrorContext, Task> _exceptionHandler;
private static readonly MediaTypeHeaderValue _textHtmlMediaType = new MediaTypeHeaderValue("text/html");
private readonly ExtensionsExceptionJsonContext _serializationContext;
private readonly IProblemDetailsService? _problemDetailsService;

/// <summary>
Expand All @@ -45,6 +49,7 @@ internal class DeveloperExceptionPageMiddlewareImpl
/// <param name="hostingEnvironment"></param>
/// <param name="diagnosticSource">The <see cref="DiagnosticSource"/> used for writing diagnostic messages.</param>
/// <param name="filters">The list of registered <see cref="IDeveloperPageExceptionFilter"/>.</param>
/// <param name="jsonOptions">The <see cref="JsonOptions"/> used for serialization.</param>
/// <param name="problemDetailsService">The <see cref="IProblemDetailsService"/> used for writing <see cref="ProblemDetails"/> messages.</param>
public DeveloperExceptionPageMiddlewareImpl(
RequestDelegate next,
Expand All @@ -53,6 +58,7 @@ public DeveloperExceptionPageMiddlewareImpl(
IWebHostEnvironment hostingEnvironment,
DiagnosticSource diagnosticSource,
IEnumerable<IDeveloperPageExceptionFilter> filters,
IOptions<JsonOptions>? jsonOptions = null,
IProblemDetailsService? problemDetailsService = null)
{
if (next == null)
Expand All @@ -77,15 +83,22 @@ public DeveloperExceptionPageMiddlewareImpl(
_diagnosticSource = diagnosticSource;
_exceptionDetailsProvider = new ExceptionDetailsProvider(_fileProvider, _logger, _options.SourceCodeLineCount);
_exceptionHandler = DisplayException;
_serializationContext = CreateSerializationContext(jsonOptions?.Value);
_problemDetailsService = problemDetailsService;

foreach (var filter in filters.Reverse())
{
var nextFilter = _exceptionHandler;
_exceptionHandler = errorContext => filter.HandleExceptionAsync(errorContext, nextFilter);
}
}

private static ExtensionsExceptionJsonContext CreateSerializationContext(JsonOptions? jsonOptions)
{
// Create context from configured options to get settings such as PropertyNamePolicy and DictionaryKeyPolicy.
jsonOptions ??= new JsonOptions();
return new ExtensionsExceptionJsonContext(new JsonSerializerOptions(jsonOptions.SerializerOptions));
}

/// <summary>
/// Process an individual request.
/// </summary>
Expand Down Expand Up @@ -172,21 +185,7 @@ private async Task DisplayExceptionContent(ErrorContext errorContext)

if (_problemDetailsService != null)
{
var problemDetails = new ProblemDetails
{
Title = TypeNameHelper.GetTypeDisplayName(errorContext.Exception.GetType()),
Detail = errorContext.Exception.Message,
Status = httpContext.Response.StatusCode
};

problemDetails.Extensions["exception"] = new
{
Details = errorContext.Exception.ToString(),
Headers = httpContext.Request.Headers,
Path = httpContext.Request.Path.ToString(),
Endpoint = httpContext.GetEndpoint()?.ToString(),
RouteValues = httpContext.Features.Get<IRouteValuesFeature>()?.RouteValues,
};
var problemDetails = CreateProblemDetails(errorContext, httpContext);

await _problemDetailsService.WriteAsync(new()
{
Expand Down Expand Up @@ -214,6 +213,31 @@ 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
{
Title = TypeNameHelper.GetTypeDisplayName(errorContext.Exception.GetType()),
Detail = errorContext.Exception.Message,
Status = httpContext.Response.StatusCode
};

// Problem details source gen serialization doesn't know about IHeaderDictionary or RouteValueDictionary.
// Serialize payload to a JsonElement here. Problem details serialization can write JsonElement in extensions dictionary.
problemDetails.Extensions["exception"] = JsonSerializer.SerializeToElement(new ExceptionExtensionData
(
Details: errorContext.Exception.ToString(),
Headers: httpContext.Request.Headers,
Path: httpContext.Request.Path.ToString(),
Endpoint: httpContext.GetEndpoint()?.ToString(),
RouteValues: httpContext.Features.Get<IRouteValuesFeature>()?.RouteValues
), _serializationContext.ExceptionExtensionData);

return problemDetails;
}

private Task DisplayCompilationException(
HttpContext context,
ICompilationException compilationException)
Expand Down Expand Up @@ -328,3 +352,10 @@ private Task DisplayRuntimeException(HttpContext context, Exception ex)
return errorPage.ExecuteAsync(context);
}
}

internal record ExceptionExtensionData(string Details, IHeaderDictionary Headers, string Path, string? Endpoint, RouteValueDictionary? RouteValues);

[JsonSerializable(typeof(ExceptionExtensionData))]
internal sealed partial class ExtensionsExceptionJsonContext : JsonSerializerContext
{
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Net.Http.Headers;
using Microsoft.AspNetCore.Mvc;
using System.Net.Http.Json;
using System.Text.Json;

namespace Microsoft.AspNetCore.Diagnostics.FunctionalTests;

Expand Down Expand Up @@ -49,5 +50,14 @@ public async Task DeveloperExceptionPage_ShowsProblemDetails_WhenHtmlNotAccepted
Assert.NotNull(body);
Assert.Equal(500, body.Status);
Assert.Contains("Demonstration exception", body.Detail);

var exceptionNode = (JsonElement)body.Extensions["exception"];
Assert.Contains("System.Exception: Demonstration exception.", exceptionNode.GetProperty("details").GetString());
Assert.Equal("application/json", exceptionNode.GetProperty("headers").GetProperty("Accept")[0].GetString());
Assert.Equal("localhost", exceptionNode.GetProperty("headers").GetProperty("Host")[0].GetString());
Assert.Equal("/", exceptionNode.GetProperty("path").GetString());
Assert.Equal("Endpoint display name", exceptionNode.GetProperty("endpoint").GetString());
Assert.Equal("Value1", exceptionNode.GetProperty("routeValues").GetProperty("routeValue1").GetString());
Assert.Equal("Value2", exceptionNode.GetProperty("routeValues").GetProperty("routeValue2").GetString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public void WriteWorks()
using Utf8JsonWriter writer = new(stream);

// Act
converter.Write(writer, problemDetails, null);
converter.Write(writer, problemDetails, new JsonSerializerOptions());

writer.Flush();
var json = Encoding.UTF8.GetString(stream.ToArray());
Expand Down
Loading