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

ProblemDetails and text/plain #154

Open
ghost opened this issue Nov 17, 2021 · 5 comments
Open

ProblemDetails and text/plain #154

ghost opened this issue Nov 17, 2021 · 5 comments

Comments

@ghost
Copy link

ghost commented Nov 17, 2021

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
Copy link
Owner

khellang commented Nov 18, 2021

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
Copy link

nascosto commented Nov 18, 2021

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
Copy link

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
Copy link
Author

ghost commented Dec 20, 2021

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?

  • Figure out what is causing the 406 Not Acceptable and override it to return the problem details response.
  • Provide a way for the middleware to ignore a given method or controller (e.g. with an attribute).

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants