khellang / Middleware

Various ASP.NET Core middleware
MIT License
804 stars 110 forks source link

ProblemDetails and text/plain #154

Open ghost opened 2 years ago

ghost commented 2 years ago

I'm seeing an issue where the problem details middleware creates a situation where a 406 Not Acceptable is being returned when other errors should be returned. This is happening when the return content type is "text/plain" and the return type is string. I am returning "text/plain" with some health checks. In actuality, I didn't want these endpoints actually having their content converted to problem details, but it still seems like it should be handled better.

Adding this to your example causes the problem

[HttpGet("text/{statusCode}")]
[Produces("text/plain")]
public ActionResult TextPlain([FromRoute] int statusCode)
{
    return new ObjectResult(statusCode) { StatusCode = statusCode };
}

I've circumvented the problem by returning ContentResult instead of ObjectResult since your middleware ignores ContentResult.

khellang commented 2 years ago

Hmm. What do you think should happen in this case? 🤔 I'm assuming you've called AddProblemDetailsConventions and this is MVC producing the problem details instance, not the actual middleware? There are ways to make it still return application/problem+json, but there's no defined standard for text/plain, so it's a bit hard to produce a "proper" response in this case.

nascosto commented 2 years ago

Yeah I'm calling the conventions. The factory recognizes it as a string and proceeds to convert it. It seems to convert it properly to the problem details object. And then somewhere else the framework is saying 'nope' to the object mixed with text/plain.

Since application/json is the default, if an endpoint is producing text/plain only the developers probably understand the behavior they want to produce. You can probably just pass it through. If their intended behavior is to produce a stringified problem details, they can always easily convert it inside the controller using ProblemDetailsFactory.CreateProblemDetails() and then serializing it to a string. If the behavior is to allow production of application/problem+json, they can add it to the valid production types.

The other option would have it be configurable based on an attribute or an options configuration and then if text/plain only comes through, you'll need to either pass it through or stringify it before creating the ObjectResult based on those settings.

I'd probably go with the first.

thomas-darling commented 2 years ago

I'd argue errors should always be returned as application/problem+json, regardless of what the client specified in the accept header of the request, or what the endpoint would normally produce. The accept header is what the client ideally wants, but the server is under no obligation to comply, and is free to say "nope, can't do that, but here's what I've got". It is then up to the client to decide how it wants to handle that unexpected response. Keep in mind, the status code of a response could always be useful to the client, even if the response body isn't.

Look at it this way:

Let's say you have a server with an endpoint that produces video files, and the client would like to specifically request one in MPEG-4 format. In this scenario, the client would specify video/mp4 in the accept header of the request, and the server would have an endpoint producing that.

Now, if an exception occurs, would you expect it to be returned as a video, or as structured data the client could potentially use to present a meaningful error message to the user? Obviously, a structured response is preferable.

My point is, this is a general problem, and the requested content type may not even be a text format, thus rendering any argument about how to reproduce errors as the requested type moot.

The only scenario where a 406 Not Acceptble response would make sense, is if the endpoint is capable of producing the requested video, but just not in a any of the formats that the client will accept.


Although, in this crazy world, it wouldn't surprise me if errors-as-memes could become a thing 🤣

WetCatException

ghost commented 2 years ago

The only scenario where a 406 Not Acceptable response would make sense, is if the endpoint is capable of producing the requested video, but just not in a any of the formats that the client will accept.

Agreed, and this is what I'd like to stop from happening. When text/plain is specified as the only type, the framework returns this error. I don't know the way around this.

I'd argue errors should always be returned as application/problem+json, regardless of what the client specified in the accept header of the request, or what the endpoint would normally produce.

Mostly agreed, but I don't think it actually works this way in practice as some standard patterns expect something else. For example, Microsoft's health check middleware. The unhealthy states defaults are always text/plain and are not returned as problem details (for some reason this doesn't throw a Not Acceptable when combined with your framework). Here is your example running the health checks..

What do you think about this two-fold approach?

This way the default is to assume that the user made a mistake and always return the problem details, but also provide a way for the developer to say "I know what I'm doing" and not have problem details in those certain cases via override.

khellang commented 2 years ago

The reason you're seeing 406 from MVC, but not health checks is that MVC has this built into its content negotiation. You can control this using MvcOptions.ReturnHttpNotAcceptable. It's basically the middleware saying "I have this response that supports JSON and XML" and your action saying "I only return plaintext", so MVC isn't able to find a fitting formatter.