[Docs] Purpose of ProblemDetails library

lonix1 commented 2 years ago

I've seen various mentions of Hellang.Middleware.ProblemDetails and I'm trying to figure out what it does. Regrettably, the readme and sample code don't explain.

I suspect it catches unhandled exceptions and converts them to ProblemDetails. However that's something that can be done in an exception middleware/filter/handler/whatever. Since this library seems really "fancy", I'm sure it does more than just that! :smiley:

Can someone please tell me what this library actually "does"? Thanks!

(BTW: I'm not trolling, I really want to know. The Microsoft docs site even recommends this library.)

lonix1 commented 2 years ago

I had to look at the source and various blogs to get a sense of what this does.

converts responses to ProblemDetails if 1) they are error responses (in range 400...599), or 2) Content-Length empty, or 3) Content-Type empty
in development, shows ProblemDetails for error (400..599) responses, as well as unhandled exceptions; shows exception details and stack trace
in production, same, but hides exception details and stack trace
handles error responses (400..599) from downstream middleware, which typically returns empty body
handles verb errors (405), e.g. post instead of get, or vice versa

I cannot figure out however what AddProblemDetailsConventions() does, and if I need it since I'm using API controllers (not MVC or RazorPages).

@khellang Please tell me if this is correct and if I missed something?

khellang commented 2 years ago

Yes, you're more or less correct. It's basically just an error handling middleware. The advantage to using middleware as opposed to an MVC filter (or something similar,) is that it'll handle error states that can occur outside of MVC, in other middleware.

Both error responses (based on status code, as long as no body has been produced yet) and exceptions are handled by the middleware (using the default configuration, almost anything can be configured) and transformed to consistent problem responses based on RFC7807. You can choose centrally how exceptions and status codes are mapped to problem responses.

The middleware is, unlike Microsofts error handling/developer exception middleware, meant to be used in the pipeline regardless of environment. There's a setting that determines whether exception details should be added to the response. The default is to only include them in development. Like other error handling middleware, it's best to include it as early as possible in the pipeline, to make sure it wraps as much of it as possible, so no error "gets out" 😆

AddProblemDetailsConventions is specific to MVC. It basically just disables MVC's built-in "client error mapping" so the middleware will handle them instead. It also does a bit of magic to use the middleware configuration when producing error responses through MVC ObjectResult. See this filter. API controllers are MVC as well, they just have some conventions enabled, like implicit model binding and automatic validation error responses. You might want to call the method if you want to have 100% consistent error responses from your API (produced by the middleware).

Let me know if something still isn't clear 😅

lonix1 commented 2 years ago

Thanks for your detailed explanation! (It may be a good idea to add your comment to the wiki/readme.)

And thanks for making/maintaining this library!

lonix1 commented 2 years ago

Some more info about AddProblemDetailsConventions, based on khellang's comments here:

It swaps MVC's built-in ProblemDetailsFactory to use the middleware's configuration instead. This will give you more consistent errors, no matter where they are produced. I.e. from MVC's invalid state filter etc.
It turns off MVC's built-in client error mapping, which also gives you more consistent error responses
It tries to set the produced error response type on API controllers
It adds a filter that makes returning pre-baked ProblemDetails instances from controllers a bit more consistent, by calling the middleware hooks

angularsen commented 2 years ago

Here are some observed differences with AddProblemDetailsConventions:

Type URL differs: "https://httpstatuses.com/500" instead of "https://tools.ietf.org/html/rfc7231#section-6.5.1"
Problem details is also used when returning ObjectResult via BadRequest(string) and BadRequest(ModelState).
422 Unprocessable Entity for ValidationProblem(ModelState), not sure why?

Problem()

return Problem(detail: "returning Problem() ObjectResult");

With AddProblemDetailsConventions: ✅

{
    "type": "https://httpstatuses.com/500",
    "title": "Internal Server Error",
    "status": 500,
    "detail": "returning Problem() ObjectResult",
    "traceId": "00-840e2012c73f076e8f07a9c48ce3b5a0-b75ad6757b2b0e60-00"
}

Without AddProblemDetailsConventions:

Type URL is "https://tools.ietf.org/html/rfc7231#section-6.5.1" ℹ

ValidationProblem(ModelState)

ModelState.AddModelError("MyKey", "My error");
return ValidationProblem(detail: "returning ValidationProblem() ObjectResult", modelStateDictionary: ModelState);

With AddProblemDetailsConventions:

Status code 422 Unprocessable Entity, why? ⚠

{
    "errors": {
        "myKey": [
            "My error"
        ]
    },
    "type": "https://httpstatuses.com/422",
    "title": "One or more validation errors occurred.",
    "status": 422,
    "detail": "returning ValidationProblem() ObjectResult",
    "traceId": "00-497278577c5bef3a2855d7b4d8aa437d-9a1eae9a5b3c99e0-00"
}

Without AddProblemDetailsConventions:

400 Bad Request ✅
Type URL is "https://tools.ietf.org/html/rfc7231#section-6.5.1" ℹ

BadRequest(ModelState)

ModelState.AddModelError("MyKey", "My error");
return BadRequest(ModelState);

With AddProblemDetailsConventions: ✅

{
    "errors": {
        "myKey": [
            "My error"
        ]
    },
    "type": "https://httpstatuses.com/400",
    "title": "One or more validation errors occurred.",
    "status": 400,
    "traceId": "00-7049949e294851e3521e5ae29335dad5-224db1813fa26f5d-00"
}

Without AddProblemDetailsConventions:

400 Bad Request ✅
No problem details ⚠
```
{
"myKey": [
    "My error"
]
}
```

BadRequest(string)

return BadRequest("some string");

With AddProblemDetailsConventions: ✅

{
    "type": "https://httpstatuses.com/400",
    "title": "Bad Request",
    "status": 400,
    "detail": "some string",
    "traceId": "00-fefab8a1fcad1de4e3f7b4225d1bd9a4-45d6461d80f3b514-00"
}

Without AddProblemDetailsConventions:

400 Bad Request ✅
No problem details ⚠
```
"some string"
```

Exception

throw new ArgumentException("invalid argument yo");

With AddProblemDetailsConventions: ✅

{
    "type": "https://httpstatuses.com/400",
    "title": "Bad Request",
    "status": 400,
    "detail": "invalid argument yo",
    "exceptionDetails": [],
    "traceId": "00-625f871eee38aa1fa7d926bcfdb2ddcb-a3bb56d6128ea74e-00"
}

Without AddProblemDetailsConventions: Same. ✅

khellang commented 2 years ago

Yeah, I think that matches what I would expect :+1:

Regarding the 422 status code; it's an opinion of mine that syntactically correct, but semantically invalid requests should return back a different status code than 400 Bad Request:

From RFC 7231:

The 400 (Bad Request) status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing).

It's increasingly common to use 422 Unprocessable Entity for this 😃

From RFC 4918:

The 422 (Unprocessable Entity) status code means the server understands the content type of the request entity (hence a 415 (Unsupported Media Type) status code is inappropriate), and the syntax of the request entity is correct (thus a 400 (Bad Request) status code is inappropriate) but was unable to process the contained instructions. For example, this error condition may occur if an XML request body contains well-formed (i.e., syntactically correct), but semantically erroneous, XML instructions.

Some people don't like it (often because it's part of the WebDAV RFC and not an "official" HTTP RFC (but this will soon change, with the inclusion of 422 in HTTPbis' upcoming HTTP Semantics RFC, which obsoletes RFC 7231), so I've added an option to change it:

https://github.com/khellang/Middleware/blob/eb7f59cb5aaff0727466622af6230db12703a32f/src/ProblemDetails/ProblemDetailsOptions.cs#L133-L136

angularsen commented 2 years ago

Thanks, that is helpful.

I'm sure there are many heated philosophical arguments whether to use 400 or 422, but our team agreed to start using 422 for validation errors moving forward to better distinguish from malformed requests.

This includes missing required fields:

PUT /foo with body { "baz": "tabom" } now fails with 422 instead of 400 bad request, when the required field "bar" is missing.

#nullable enable
public class MyUpdateDto
{
    public string Bar { get; set; } = null!;
    public string Baz { get; set; } = null!;
}

khellang commented 2 years ago

I'm sure there are many heated philosophical arguments whether to use 400 or 422, but our team agreed to start using 422 for validation errors moving forward to better distinguish from malformed requests.

Indeed. Welcome to the dark side 😜

PUT /foo with body { "baz": "tabom" } now fails with 422 instead of 400 bad request, when the required field "bar" is missing.

Yeah, what you sent is perfectly valid JSON, but your domain expected the Bar value to be there, but it wasn't, so 422 makes a lot of sense to me 😃

pinkfloydx33 commented 2 years ago

What should return 400, then? I assumed that invalid json would return bad request, but with the conventions enabled I still get a 422.

pinkfloydx33 commented 2 years ago

@lonix1 I'm familiar with how the model binder works and I'm not disputing 400 vs 422. I'm trying to understand in the context of this library--based on this documentation--the expected outcomes. As both you and this thread seem to imply there are cases where I could get a 400 triggered during model binding, except that's not the behavior I'm actually experiencing.

With the conventions enabled, both malformed json (which is syntactically invalid) and passing a date where a number is expected result in 422 and never a 400. Same goes for any mismatched data types in any location (query, request body, path; string/int/date) that I've tested.

Try as I might, I'm unable to trigger a 400 response from the framework (wrt model binding). I merely wish to understand the expectations here as I actually prefer the 422, but if MVC will still return a 400 in some circumstances then I want to understand when/why so I can explain the difference to consumers (and think it should be included in this documentation). If it's expected that other than an explicit BadRequest I won't see a 400 that's great; it's just not clear to me.

I'm simply asking for clarification on the points already discussed here. If this sounds like a bug I'm more than happy to transfer my comments to a new issue (or if you'd like, I can do that anyways since this one is technically closed)

lonix1 commented 2 years ago

@pinkfloydx33 Maybe this will help. I use it to ensure the model binder returns 400.

services.PostConfigure<ApiBehaviorOptions>(c => {
  c.InvalidModelStateResponseFactory = actionContext => {
    var problemsDetailsFactory = actionContext.HttpContext.RequestServices.GetRequiredService<ProblemDetailsFactory>();
    var modelState = new ModelStateDictionary(); // populate modelState here as needed
    var problemDetails = problemsDetailsFactory.CreateValidationProblemDetails(actionContext.HttpContext, modelState, StatusCodes.Status400BadRequest);
    return new BadRequestObjectResult(problemDetails);
  };
});

khellang commented 2 years ago

What should return 400, then? I assumed that invalid json would return bad request, but with the conventions enabled I still get a 422.

There's no customization in MVC's handling of syntactical JSON errors using this middleware (even with the conventions enabled). It's really weird that MVC calls the validation problem method to create a response for a syntax error. I would've guessed they threw an exception long before it reached the validation stage, but your post is devoid of any details so it's hard to tell 😅

Anyway, I think it might be better to transfer this to a new issue...

UPDATE: Seems like MVC is simply swallowing a perfectly good JsonException and stuffing some message into the validation ModelState. Looks like it's impossible to differentiate between syntactic and semantic errors in MVC.

https://github.com/dotnet/aspnetcore/blob/757367ebc3a19fcfb2dd101e7c0a9d4e9896fdfd/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs#L75-L96

cremor commented 2 years ago

I just noticed this change from HTTP 400 to 422 myself. I agree that 422 is better for validation errors, but would like to use 400 for JSON syntax errors. If I understand you correctly there is no way to do that? Also no workaround? That would be really sad.

Do you maybe know of an related issue in the ASP.NET Core repository? I'd think that differentiating syntax and validation errors would be a usual request.

khellang commented 2 years ago

If I understand you correctly there is no way to do that? Also no workaround? That would be really sad.

Nope, MVC just catches JSON errors and stick them in the ModelStateDictionary, which is the same place validation errors goes. See https://github.com/dotnet/aspnetcore/blob/0b1e29fc4d0b8a4f50ecf36832fa6a4e4ea94a3c/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs#L229-L338 or https://github.com/dotnet/aspnetcore/blob/0b1e29fc4d0b8a4f50ecf36832fa6a4e4ea94a3c/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs#L75-L96

cremor commented 2 years ago

I think I found a way for my use case. I use FluentValidation with ProblemDetailsOptionsExtensions, so I configure ProblemDetailsOptions.ValidationProblemStatusCode to 400 and change MapFluentValidationException to explicitly pass 422 to CreateValidationProblemDetails.

It's not perfect since only ValidationExceptions use 422 but I think it will be good enough for me.

khellang commented 2 years ago

Cool! Yeah, sounds like a reasonable compromise ☺️

khellang / Middleware