search

dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.23k stars 9.95k forks source link

Allow consistent `Problem Details` generation #42212

Closed brunolins16 closed 2 years ago

brunolins16 commented 2 years ago

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

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

Here are some examples of reported issues by the community:

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:

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 commented 2 years ago

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:

pinkfloydx33 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

cc @khellang for feedback.

khellang commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

API review notes:

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

brunolins16 commented 2 years ago

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 commented 2 years ago

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:

halter73 commented 2 years ago

API Review Notes:

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 commented 2 years ago

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 commented 2 years ago

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:

halter73 commented 2 years ago

Quick API review notes:

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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