Allow consistent Problem Details generation

[brunolins16]

Background and Motivation

API Controllers have a mechanism to auto generated Problem Details (https://datatracker.ietf.org/doc/html/rfc7807) for API Controller ActionResult. The mechanism is enabled by default for all API Controllers, however, it will only generates a Problem Details payload when the API Controller Action is processed and produces a HTTP Status Code 400+ and no Response Body, that means, scenarios like - unhandled exceptions, routing issues - won't produce a Problem Details payload.

Here is overview of when the mechanism will produce the payload:

❌ = Not generated
✅ = Automatically generated

Routing issues: ❌

Unhandled Exceptions: ❌

MVC

• StatusCodeResult 400 and up: ✅ (based on SuppressMapClientErrors)
  • BadRequestResult and UnprocessableEntityResult are StatusCodeResult
• ObjectResult: ❌ (Unless a ProblemDetails is specified in the input)
  • eg.: BadRequestObjectResult and UnprocessableEntityObjectResult
• 415 UnsupportedMediaType: ✅ (Unless when a ConsumesAttribute is defined)
• 406 NotAcceptable: ❌ (when happens in the output formatter)

Minimal APIs won't generate a Problem Details payload as well.

Here are some examples of reported issues by the community:

• Usage of ProblemDetails is inconsistent throughout ASP.NET Core #32957
• ReturnHttpNotAcceptable=true Does Not Return ProblemDetails #16889
• ProblemDetails is not returned for 404NotFound and 500Exception #4953

Proposed API

🎯 The goal of the proposal is to have the ProblemDetails generated, for all Status 400+ (except in Minimal APIs - for now) but the user need to opt-in and also have a mechanism that allows devs or library authors (eg. API Versioning) generate ProblemDetails responses when opted-in by the users.

An important part of the proposed design is the auto generation will happen only when a Body content is not provided, even when the content is a ProblemDetails that means scenario, similar to the sample below, will continue generate the ProblemDetails specified by the user and will not use any of the options to suppress the generation:

public ActionResult GetBadRequestOfT() => BadRequest(new ProblemDetails());

Overview:

• Minimal APIs will not have an option to autogenerate ProblemDetails.
• Exception Handler Middleware will autogenerate ProblemDetails only when no ExceptionHandler or ExceptionHandlerPath is provided, the IProblemDetailsService is registered,ProblemDetailsOptions.AllowedProblemTypes contains Server and a IProblemMetadata is added to the current endpoint.
• Developer Exception Page Middleware will autogenerate ProblemDetails only when detected that the client does not accept text/html, the IProblemDetailsService is registered,ProblemDetailsOptions.AllowedProblemTypes contains Server and a IProblemMetadata is added to the current endpoint.
• Status Code Pages Middleware default handler will generate a ProblemDetails only when detected the IProblemDetailsService is registered and the ProblemType requested is allowed and a IProblemMetadata is added to the current endpoint.
• A call to AddProblemDetails is required and will register the IProblemDetailsService and a DefaultProblemDetailsWriter.
• A call to AddProblemDetails is required and will register the IProblemDetailsService and a DefaultProblemDetailsWriter.
• When APIBehaviorOptions.SuppressMapClientErrors is false, a IProblemMetadata will be added to all API Controller Actions.
• MVC will have an implementation of IProblemDetailsWriter that allows content-negotiation that will be used for API Controllers, routing and exceptions. The payload will be generated only when a APIBehaviorMetadata is included in the endpoint.
• Addition Problem Details configuration, using ProblemDetailsOptions.ConfigureDetails, will be applied for all autogenerated payload, including BadRequest responses caused by validation issues.
• MVC 406 NotAcceptable response (auto generated) will only autogenerate the payload when ProblemDetailsOptions.AllowedMapping contains Routing.
• Routing issues (404, 405, 415) will only autogenerate the payload when ProblemDetailsOptions.AllowedMapping contains Routing.

A detailed spec is here.

namespace Microsoft.Extensions.DependencyInjection;

+public static class ProblemDetailsServiceCollectionExtensions
+{
+    public static IServiceCollection AddProblemDetails(this IServiceCollection services) { }
+    public static IServiceCollection AddProblemDetails(this IServiceCollection services, Action<ProblemDetailsOptions> configureOptions) +{}
+}

namespace Microsoft.AspNetCore.Http;

+public class ProblemDetailsOptions
+{
+    public ProblemTypes AllowedProblemTypes { get; set; } = ProblemTypes.All;
+    public Action<HttpContext, ProblemDetails>? ConfigureDetails { get; set; }
+}

+[Flags]
+public enum ProblemTypes: uint
+{
+    Unspecified = 0,
+    Server = 1,
+    Routing = 2,
+    Client = 4,
+    All = RoutingFailures | Exceptions | ClientErrors,
+}

+public interface IProblemDetailsWriter
+{
+   bool CanWrite(HttpContext context);
+   Task WriteAsync(HttpContext context, int? statusCode, string? title, string? type, string? detail, string? instance, IDictionary<string, object?>? extensions);
+}

+public interface IProblemDetailsService
+{
+      bool IsEnabled(ProblemTypes type);
+      Task WriteAsync(HttpContext context, EndpointMetadataCollection? currentMetadata = null, int?  statusCode = null, string? title = null, string? type = null, string? detail = null, string? instance = null, IDictionary<string, object?>?  extensions = null);
+}

namespace Microsoft.AspNetCore.Http.Metadata;

+public interface IProblemMetadata
+{
+    public int? StatusCode { get; }
+    public ProblemTypes ProblemType { get; }
+}

namespace Microsoft.AspNetCore.Diagnostics;

public class ExceptionHandlerMiddleware
{
    public ExceptionHandlerMiddleware(
        RequestDelegate next,
        ILoggerFactory loggerFactory,
        IOptions<ExceptionHandlerOptions> options,
        DiagnosticListener diagnosticListener,
+       IProblemDetailsService? problemDetailsService = null)
    {}
}

public class DeveloperExceptionPageMiddleware
{
    public DeveloperExceptionPageMiddleware(
        RequestDelegate next,
        IOptions<DeveloperExceptionPageOptions> options,
        ILoggerFactory loggerFactory,
        IWebHostEnvironment hostingEnvironment,
        DiagnosticSource diagnosticSource,
        IEnumerable<IDeveloperPageExceptionFilter> filters,
+       IProblemDetailsService? problemDetailsService = null)
    {}
}

Usage Examples

AddProblemDetails

Default options

var builder = WebApplication.CreateBuilder(args);

// Add services to the containers
builder.Services.AddControllers();
builder.Services.AddProblemDetails();

var app = builder.Build();

// When problemdetails is enabled this overload will work even
// when the ExceptionPath or ExceptionHadler are not configured
app.UseExceptionHandler();

// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
}

//Generate PD for 400+
app.UseStatusCodePages();

app.MapControllers();
app.Run();

Custom Options

var builder = WebApplication.CreateBuilder(args);

// Add services to the containers
builder.Services.AddControllers();
builder.Services.AddProblemDetails(options => { 

    options.AllowedProblemTypes = ProblemTypes.Server | ProblemTypes.Client | ProblemTypes.Routing;
    options.ConfigureDetails = (context, problemdetails) => 
    {
       problemdetails.Extensions.Add("my-extension", new { Property = "value" });
    };
});

var app = builder.Build();

// When Problem Details is enabled this overload will work even
// when the ExceptionPath or ExceptionHadler are not configured
app.UseExceptionHandler();

// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
}

app.MapControllers();
app.Run();

Creating a custom ProblemDetails writer

public class CustomWriter : IProblemDetailsWriter
{
    public bool CanWrite(HttpContext context) 
        => context.Response.StatusCode == 400;

    public Task WriteAsync(HttpContext context, int? statusCode, string? title, string? type, string? detail, string? instance, IDictionary<string, object?>? extensions) 
        => context.Response.WriteAsJsonAsync(CreateProblemDetails(statusCode, title, type, detail, instance, extensions));

    private object CreateProblemDetails(int? statusCode, string? title, string? type, string? detail, string? instance, IDictionary<string, object?>? extensions)
    {
        throw new NotImplementedException();
    }
}

// the new write need to be registered
builder.Services.AddSingleton<IProblemDetailsWriter, CustomWriter>();

Writing a Problem Details response with IProblemDetailsService

public Task WriteProblemDetails(HttpContext httpContext)
{
  httpContext.Response.StatusCode = StatusCodes.Status400BadRequest;

  if (context.RequestServices.GetService<IProblemDetailsService>() is { } problemDetailsService)
  {
      return problemDetailsService.WriteAsync(context);
  }

  return Task.CompletedTask;
} 

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[pinkfloydx33]

Problem Details Middleware does a lot (if not all?) of this already and seems a lot less complex than the proposal here. Perhaps the work there could be piggybacked on some how? It definitely would be nice to not need a reference to a 3rd party to get some of what that library offers

[brunolins16]

@pinkfloydx33 Thanks for the feedback. I'd like to know why do you think this proposal is complex? The usage will be just a call for service.AddProblemDetails that will enable the consistent behavior.

Also, just to let you know I don't want to introduce a middleware because it doesn't compose with the other middlewares. As example, in my proposal, the ExceptionMiddleware will behave differently when the service is available instead of have another middleware responsible for do the same.

[davidfowl]

I agree, this doesn't look complicated (though I haven't dug in deeply as yet).

cc @khellang for feedback.

[khellang]

Woah, finally 😅 This will most certainly kill my middleware (which I don't necessarily mind) if done right.

I see the current proposal doesn't mention the existing ProblemDetailsFactory extensibility point - is that on purpose?

What about a quick way to configure mapping of exceptions to different types of problem details? Is that something you've considered?

I'm on the phone right now, but I'll take a deeper look and see if I can come up with some additional feedback later 👌

[pinkfloydx33]

Sorry, "complex" may have been the wrong word. The middleware I cite makes use of ProblemDetailsFactory and doesn't introduce much in the way of anything new (other than some incredible customization options), so I just wonder if the new interfaces/writer are necessary?

I'm using the middleware both in my APIs and in my YARP-backed API gateway (to catch anything that may slip through and to augment 50x statuses), where I've also got Custom StatusCode pages / error pages and have wired everything together to get a nice problem details response in 100% of cases. That said, I did have to reinvent content negotiation in the custom-status-code-page to ensure my gateway returns a nice html page when expected and a problem response otherwise. So that part of the proposal appeals to me since I'd likely be able to remove the hack.

If the goal is to not have this as a middleware, I guess I understand the need for the extra interfaces. It just seems weird this can't be a couple extra options and some tweaks to ProblemDetailsFactory/[Validation]ProblemDetails. I'm no expert on ASP internals though, so take that for what it's worth.

No matter the solution, I welcome this. I am a very satisfied user of @khellang's project and have it included by default in our internal templates. It would definitely be nice to have something like it in-box

(Thinking on it a bit more, I suppose without a middleware you need some mechanism to convert general error responses into problem details, somewhere higher up in the stack where the writer can be injected/available and likely optimized... it's making a bit more sense ).

[brunolins16]

I am not sure if you have a chance to see the detailed design but problemdetailsfactory will be use to handle APi controllers as well but since the idea is to create something that does not rely on MVC the default writer is simpler and does not supports content-negotiation.

That said, those are all good feedback and will make me review the proposal this week. Thanks.

[khellang]

some mechanism to convert general error responses into problem details

Yeah, my understanding is that this is kinda "repurposing" the error handler middleware to produce ProblemDetails without much customization. My middleware is duplicating a lot of what the error handler middleware is already doing.

I still think the proposal needs a few more knobs before you'll be able to make the switch and keep the same functionality. Configuring different mapping functions for different exception types is one of them.

[brunolins16]

I still think the proposal needs a few more knobs before you'll be able to make the switch and keep the same functionality. Configuring different mapping functions for different exception types is one of them.

@khellang the ProblemDetailsOptions.ConfigureDetails is a general-purpose extension point but I agree that more knobs are needed, for exceptions, but I think those should not go to the central configuration and instead each part of the framework should provide their extensibility. A more complex extension point is the IProblemDetailsWriter but I don't see a lot applications implementing this but libraries could potentially do, it is not as complete as implementing a middleware like yours but give more flexibility about how to handle a problem details.

In my initial proposal I included the ExceptionHandlerOptions.ConfigureDetails, that will basically allow the ExceptionHandler Middleware compose with the general-purpose problem details:

public class ExceptionHandlerOptions
{
+    public Action<IExceptionHandlerFeature, ProblemDetails>? ConfigureDetails { get; set; }
}

I prefer not include it in my final proposal, and make it follow up issue, but I will talk about it during the API review. Also, it potentially could be similar to your middleware Map options.

Since you have been working in your middleware for while I really appreciate your feedback.

[khellang]

Yeah, it makes sense to scope exception mapping to the error handler middleware, since it's the one actually handling the exceptions, but I also see it as a way to misconfigure stuff.

What if you try to set ConfigureDetails on ExceptionHandlerOptions without calling AddProblemDetails? Or if AllowedMapping doesn't include Exceptions?

[brunolins16]

but I also see it as a way to misconfigure stuff.
What if you try to set ConfigureDetails on ExceptionHandlerOptions without calling AddProblemDetails? Or if AllowedMapping > doesn't include Exceptions?

It will be a no-op, since the service is not registered (or if registered and not Exceptions allowed) the exception handler will never produce a ProblemDetails. It is not clear and the exact reason why I prefer keeping out of my final proposal.

BTW, if you call services.UseExceptionHandler() without configuring the ExceptionHandler or ExceptionPath today it will throw a misconfigured exception, and in my proposal it will be allowed if AddProblemDetails was called.

[BrennanConroy]

API review notes:

• Can we use a context for the WriteAsync methods? Avoid large number of default arguments :D
• Let's try to use ValueTask for Task returning methods
• Let's check the breaking-change-ness of adding a default value to the middleware ctors, or make a new public ctor with the additional arg and see if DI picks the longer one
• Rename IProblemMetadata and IProblemTypes to IProblemDetailsMetadata IProblemDetailsTypes
• Fix typo: All = RoutingFailures | Exceptions | ClientErrors,
• Is IProblemDetailsService.IsEnabled(ProblemTypes type); needed?
  • Don't think so, let's see what it's like without it
• Multiple writers can be registered and we'll iterate over them until one can write, let's make sure the doc comments are clear and if this is the pattern we want

@brunolins16 will update the proposal for either email or next review

[brunolins16]

Updated API Proposal:

namespace Microsoft.Extensions.DependencyInjection;

+public static class ProblemDetailsServiceCollectionExtensions
+{
+    public static IServiceCollection AddProblemDetails(this IServiceCollection services) { }
+    public static IServiceCollection AddProblemDetails(this IServiceCollection services, Action<ProblemDetailsOptions> configureOptions) +{}
+}

namespace Microsoft.AspNetCore.Http;

+public class ProblemDetailsOptions
+{
+    public ProblemTypes AllowedProblemTypes { get; set; } = ProblemTypes.All;
+    public Action<HttpContext, ProblemDetails>? ConfigureDetails { get; set; }
+}

+[Flags]
+public enum ProblemDetailsTypes: uint
+{
+    Unspecified = 0,
+    Server = 1,
+    Routing = 2,
+    Client = 4,
+    All =Server | Routing | Client ,
+}

+public class ProblemDetailsContext
+{
+    public ProblemDetailsContext(HttpContext httpContext) {}

+    public HttpContext HttpContext { get; }
+    public EndpointMetadataCollection? AdditionalMetadata { get; init; }
+    public ProblemDetails ProblemDetails { get; init; } = new ProblemDetailt();
+}

+public interface IProblemDetailsWriter
+{
+    ValueTask<bool> WriteAsync(ProblemDetailsContext context);
+}

+public interface IProblemDetailsService
+{
+     ValueTask WriteAsync(ProblemDetailsContext context);
+}

namespace Microsoft.AspNetCore.Http.Metadata;

+public interface IProblemDetailsMetadata
+{
+    public int? StatusCode { get; }
+    public ProblemTypes ProblemType { get; }
+}

namespace Microsoft.AspNetCore.Diagnostics;

public class ExceptionHandlerMiddleware
{
+    public ExceptionHandlerMiddleware(
+        RequestDelegate next,
+        ILoggerFactory loggerFactory,
+        IOptions<ExceptionHandlerOptions> options,
+        DiagnosticListener diagnosticListener,
+        IProblemDetailsService? problemDetailsService) {}
}

public class DeveloperExceptionPageMiddleware
{
+    public DeveloperExceptionPageMiddleware(
+        RequestDelegate next,
+        IOptions<DeveloperExceptionPageOptions> options,
+        ILoggerFactory loggerFactory,
+        IWebHostEnvironment hostingEnvironment,
+        DiagnosticSource diagnosticSource,
+        IEnumerable<IDeveloperPageExceptionFilter> filters,
+        IProblemDetailsService? problemDetailsService) {}
}

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

API Review Notes:

• Why public enum ProblemDetailsTypes: uint?
  • We should just leave it default
• Can we rename ConfigureDetails to ConfigureProblemDetails?
  • ProblemDetailsOptions.ConfigureDetails -> ProblemDetailsOptions.CustomizeProblemDetails and make the parameter ProblemDetailsContext.
• What about ProblemDetailsKinds instead of ProblemDetailsTypes?
  • Let's do it
• Are Server, Routing and Client clear enough enum cases?
  • Let's just remove the flags for now. We can add them later if there's an ask.
• Do we want the ability to "configure" ProblemDetails that are returned from an MVC Action or minimal route handler?
  • Seems convenient, but adds complexity.
• Can we use required init for ProblemDetailsContext?
  • Let's do it.
• Can we also get rid of IProblemDetailsMetadata until we add something like ProblemDetailsTypes?
  • It seems like it.
• Let's add internal ExceptionHandlerMiddleware and DeveloperExceptionPageMiddleware classes (we have to keep the public classes with the existing constructors ofc), but we can add the new constructors to the internal class which the extension methods will then use.

API Approved!

namespace Microsoft.Extensions.DependencyInjection;

+public static class ProblemDetailsServiceCollectionExtensions
+{
+    public static IServiceCollection AddProblemDetails(this IServiceCollection services) { }
+    public static IServiceCollection AddProblemDetails(this IServiceCollection services, Action<ProblemDetailsOptions>? configure) { }
+}

namespace Microsoft.AspNetCore.Http;

+public class ProblemDetailsOptions
+{
+    public Action<ProblemDetailsContext>? CustomizeProblemDetails { get; set; }
+}

+public class ProblemDetailsContext
+{
+    public required HttpContext HttpContext { get; init; }
+    public EndpointMetadataCollection? AdditionalMetadata { get; init; }
+    public ProblemDetails ProblemDetails { get; init; } = new ProblemDetails();
+}

+public interface IProblemDetailsWriter
+{
+    ValueTask<bool> TryWriteAsync(ProblemDetailsContext context);
+}

+public interface IProblemDetailsService
+{
+     ValueTask WriteAsync(ProblemDetailsContext context);
+}

[brunolins16]

During the PR review was found that we could have a small memory allocation improvement (avoiding multiple async state machines) if we do the following API change comparing with the approved API:

 public interface IProblemDetailsWriter
{
-    ValueTask<bool> TryWriteAsync(ProblemDetailsContext context); 
+    ValueTask WriteAsync(ProblemDetailsContext context);
+    bool CanWrite(ProblemDetailsContext context); 
}

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

Quick API review notes:

• This looks good to me. We don't expect there to be many IProblemDetailsWriter implementations or callers, so making it slightly clunkier to improve perf seems worthwhile.

API with update approved!

namespace Microsoft.Extensions.DependencyInjection;

+public static class ProblemDetailsServiceCollectionExtensions
+{
+    public static IServiceCollection AddProblemDetails(this IServiceCollection services) { }
+    public static IServiceCollection AddProblemDetails(this IServiceCollection services, Action<ProblemDetailsOptions>? configure) { }
+}

namespace Microsoft.AspNetCore.Http;

+public class ProblemDetailsOptions
+{
+    public Action<ProblemDetailsContext>? CustomizeProblemDetails { get; set; }
+}

+public class ProblemDetailsContext
+{
+    public required HttpContext HttpContext { get; init; }
+    public EndpointMetadataCollection? AdditionalMetadata { get; init; }
+    public ProblemDetails ProblemDetails { get; init; } = new ProblemDetails();
+}

+public interface IProblemDetailsWriter
+{
+    bool CanWrite(ProblemDetailsContext context); 
+    ValueTask WriteAsync(ProblemDetailsContext context);
+}

+public interface IProblemDetailsService
+{
+     ValueTask WriteAsync(ProblemDetailsContext context);
+}

[pinkfloydx33]

So what would be the recommended method for customizing the Problem Details based on caught exceptions? Assuming we're using the Exception handling middleware, would we just check for the IExceptionHandlerFeature in the context.HttpContext.Features from within the CustomizeProblemDetails callback?

[brunolins16]

@pinkfloydx33 Exactly, and you can do this on the CustomizeProblemDetails callback (simplest way) or create a Custom Writer that handle when the feature is available.

public class SamppleExceptionWriter : IProblemDetailsWriter
{
    public bool CanWrite(ProblemDetailsContext context)
        => context.HttpContext.Response.StatusCode >= 500 && 
           context.HttpContext.Features.Get<IExceptionHandlerFeature>() is not null;

    public ValueTask WriteAsync(ProblemDetailsContext context)
    {
        var httpContext = context.HttpContext;

        // Customize you PD (if you want you need to explicit call the CustomizeProblemDetails here)

        // Write to the response
        return new ValueTask(httpContext.Response.WriteAsJsonAsync(context.ProblemDetails));
    }
}

Just to let you know, I am planning to create a new issue to propose changes, in .NET 8, to the ExceptionHandler middleware to support something similar to what we have in @khellang middleware.

[pinkfloydx33]

Cool, thanks for the example. That's pretty much what I was thinking. The trick with the writers however will be ensuring you add them in the correct order since it's the first CanWrite that wins. Not a big deal of course