ProblemDetails is not returned for 404NotFound and 500Exception

[PureKrome]

Describe the bug

Given a new ASPNET Core 2.2 "API" Website, ProblemDetails are not returned when:

• the route doesn't exist (404 Not found)
• an exception is thrown from inside an ActionMethod.

I understand that currently ProblemDetails are provided when a Controller is decorated with ApiController. So the first scenario (route doesn't exist/match) can't currently be handled. The second scenario (exeception thrown) is inside an Controller/ActionMethod that has been decorated.

ProblemDetails should apply globally, not just on a half-specific set of scenario's. This breaks application-consistency.

Currently, the only workaround is to use @khellang 's ProblemDetails nuget library (part of his 'Middleware' repo).

Would be very helpful to either consider:
a) implementing the full functionality
b) just removing it because the current implementation feels inconsistent / half-complete. (NOTE: please don't remove it! lets complete it!!)

To Reproduce

• Open Visual Studio 2018
• File -> New -> Project -> Visual C# -> Web -> NET Core -> ASP.NET Core Web Application -> API
• Edit Startup.cs. Comment out \\ app.UseDeveloperExceptionPage();
• Edit Controllers\ValuesController to (more or less) the following:

[Route("api/[controller]")]
[ApiController]
public class ValuesController : ControllerBase
{
    // GET api/values
    [HttpGet]
    public ActionResult<IEnumerable<string>> Get()
    {
        return new string[] { "value1", "value2" };
    }

    // GET api/values/5
    [HttpGet("{id}")]
    public ActionResult<string> Get(int id)
    {
        return Ok(id);
    }

    // GET api/values/notfound
    [HttpGet("notfound")]
    public ActionResult<string> Get2()
    {
        return NotFound();
    }

    // GET api/values/notfound/aaaa
    [HttpGet("notfound/{text}")]
    public ActionResult<string> Get3(string text)
    {
        return NotFound(text);
    }

    // GET api/values/exception
    [HttpGet("exception")]
    public ActionResult<string> Get4()
    {
        throw new Exception("pew pew");
    }
}

I've just added some specific routes to test problem details. (look below 😎 )

• Test the following routes:
  • ✅ https://localhost:7001/api/values/a (400 Bad Request)
  • ✅ https://localhost:7001/api/values/notfound (404 Not Found)
  • ❌ https://localhost:7001/api/values/notfound/aaaaaa (404 Not Found)
  • ❌ https://localhost:7001/api/values/exception (500 Server Error)
  • ❌ https://localhost:7001/api/values/sdfkdsjfhksdfh (404 Not Found and route doesn't exist)

Expected behavior

• At any time the response is 4xx/5xx I would expect the ProblemDetails to be returned.
• Would love to have the option to specify wether exception details are included (default is not). This just helps when IsDevelopment mode.
• Assumption would be to wire this up in Startup.cs.

Additional context

.NET Core SDK (reflecting any global.json):
 Version:   2.2.100
 Commit:    b9f2fa0ca8

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.17134
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.2.100\

Host (useful for support):
  Version: 2.2.0
  Commit:  1249f08fed

Also using Visual Studio 15.9.4

polite cc @glennc

Thank you kindly for your awesome work and here's Dwayne/TheRock singing sweet nothings to help persuade the decision process, here. Like, who would say 'no' to TheRock??

[wangengzheng]

try to use routetemplate [HttpGet("/notfound/{text}")]

[khellang]

Y NO USE MY MIDDLEWARE? 🤔 😢

[PureKrome]

@khellang but I am :) and it's great! Just seeing if the feature could be further expanded/completed or ... not (which means we all stick with yours). It just feels incomplete, right now. Sure, this 'might be as designed' but I thought .. when comparing your middleware to the one in MVC .. there's a heap of gaps.

I also understand that this can be compared to the MS IoC/DI vs structuremap, etc. (3rd parties). I feel that the MS DI is really usable for seriously most "common" scenarios - very workable. But comparing the the MS ProblemDetails stuff, I don't think it's complete for "common" scenario's.

[mkArtakMSFT]

Thanks for contacting us, @PureKrome.
@pranavkm, any suggestions? Thanks!

[pranavkm]

cc @rynowak \ @JamesNK for thoughts about 404 from routing. IMO, our philosophy has been to use ApiControllerAttribute to determine if a particular action should have web API-like behavior. We cannot make that determination for a 404 for an action that does not exist.

It's fairly trivial to configure the filter to respond to additional status codes (https://github.com/aspnet/AspNetCore/blob/release/2.2/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs#L194-L203). Even if we configured it to handle 5xx status codes, we wouldn't add an exception filter \ do exception handling by default (too opinionated, breaks existing logging and exception handling etc). I'm not sure if it would be of much use in this configuration.

[PureKrome]

@pranavkm thanks heaps for jumping into this issue 👍

Even if we configured it to handle 5xx status codes, we wouldn't add an exception filter \ do exception handling by default

Ok - so based upon this answer, what you're suggesting is that you think it's totally fine/acceptable to have mixed results for a standard Web/REST Api website (** I hate those names to describe an API website but it's a common way to describe these apps).

Mixed == sometimes we get problem details. Other times, we don't get PD's and (by default I think) just a status code.

vs

An API website that returns PD's for any non 200 response.

[pranavkm]

@glennc thoughts on this?

[PureKrome]

/me tries poorly to jedi-mind trick Glenn...

[ukod]

I agree with @PureKrome, the main reason, because I have enabled ProblemDetails, was one fine data structure to resolve problems with error handling. That's really important. Please, consider how to include other status codes into ProblemDetails as default.

[PureKrome]

Polite ping to @glennc and @pranavkm 👋

.NET Core v3 is not far off and preview 7 is next. Any chance to try and include this in there before stuff is closed off?

pwitty pwease ?

[pranavkm]

/cc @davidfowl who spent some time looking at this.

a) With 500s, you could use the error handler to point to an MVC action and have that return ProblemDetails. That works fairly well for most parts. You could also incorporate the check from DeveloperExceptionPageMiddleware to do some form of conneg if your application hosts both UI and API content.

b) 400s have the same solution using either MapFallback or UseStatusCodePagesWithReExecute.

While the templates aren't exactly wired up to do this, it does seem like there are enough building blocks to meet your goals. What do you think?

[PureKrome]

While the templates aren't exactly wired up to do this, it does seem like there are enough building blocks to meet your goals. What do you think?

Yep - there are enough building blocks to do this. As a case in point, I did this nearly a year ago and have been using this in various .NET apps.

I guess my main request was to try and bake this more into the common template stuff. Part of the issue here was that having a webapi only handle some errors are problem-details feels like an incomplete solution. Think about it -> why would a webapi only return some errors as PD's while other scenario's as .. not-PD's?

So the discussion was more about having a consistent solution across a webapi template.

[RehanSaeed]

I tested this with the latest 3.0.100 SDK and 404 and 500 now correctly return ProblemDetails responses. I think this issue can be closed.

However, 406 Not Acceptable responses which are returned when you enable MvcOptions.ReturnHttpNotAcceptable=true are returning empty response bodies instead of ProblemDetails. See #16889.

[PureKrome]

@RehanSaeed wait what? So 5xx errors now return ProblemDetails? (If yes, then woooot!) When did this sneak in? 3.0? (3.0.100 == 3.0, m'right?)

If yes, wish there was some info dropped into this thread to give us some exciting news, etc...

[pranavkm]

AFAIK, this should only be for 500 StatusCodeActionResult returned from an MVC action. For arbitrary 500s in your application, you could configure the error handler to return Problem responses - https://docs.microsoft.com/en-us/aspnet/core/web-api/handle-errors?view=aspnetcore-3.0#exception-handler.

@RehanSaeed would you mind filing a separate issue for the 406 scenario? We'll need to do something nicer here: https://github.com/aspnet/AspNetCore/blob/4c67855ccfb77e4e06087aa7d61f1d4894716af4/src/Mvc/Mvc.Core/src/Infrastructure/ObjectResultExecutor.cs#L150-L154

[PureKrome]

Cheers @pranavkm ! Okay - so nothing new has changed and the initial question still remains a valid one.

We understand that we can do some custom code to handle this stuff ... but the root question remains valid, IMO. So far, what I'm reading is this (unfortunately):

• If you return a 5xx/4xx from your ActionMethod, then a ProblemDetails will be returned.
• Otherwise, it won't -AND- we (Microsoft) want this 'hybrid' behaviour.

As such, can you please explain the reasoning for this hybrid behavior? To keep things simple, can you use the 404 example, please?

• return 404 from an Action Method == PD.
• no route that matches ... 404 and not a PD.

[khellang]

no route that matches ... 404 and not a PD.

This is because the behavior is MVC-specific. If a path doesn't match any routes, MVC won't even touch the request and fall through. Without middleware to handle this on the way back, there's no way to be consistent here...

[PureKrome]

no route that matches ... 404 and not a PD.

This is because the behavior is MVC-specific. If a path doesn't match any routes, MVC won't even touch the request and fall through. Without middleware to handle this on the way back, there's no way to be consistent here...

Yep correct! that's the technical reason why this is currently occurring. I'm hoping the product team can answer why they prefer this hybrid result and explain the reasoning for it. I'm not hating, just trying to learn and understand the decision, here.

[pranavkm]

@PureKrome it's a non-trivial amount of work to move ProblemDetails and everything else required to configure and serialize it closer to the heart of the stack. We would need very compelling reasons to invest in something like this which we just haven't seen as yet.

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[PureKrome]

Thanks @captainsafia for the update! Really appreciate it 😄 🍰

Despite not being the best answer, there's still hope this can get some more love, next release. It's nice to see that tough decisions can be made and then explained to us - so cheers.

@khellang your code lives for another day!

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[RehanSaeed]

Good to see this being looked at again for .NET 7. Please also remember to consider this related issue:

• ReturnHttpNotAcceptable=true Does Not Return ProblemDetails #16889

[PureKrome]

@brunolins16 any chance for a quick summary of what ended up getting merged/added to the codebase?

[brunolins16]

@PureKrome here is the final API #42212 (comment).

Basically, it is the introduction of the ProblemDetailsService that could be used everywhere. In the PR, the DeveloperExceptionHandler, ExceptionHandler and StatusCodePages middleware were updated to use it when nothing custom is defined.

That means, for your very initial example:
✅ https://localhost:7001/api/values/a (400 Bad Request)
✅ https://localhost:7001/api/values/notfound (404 Not Found)
❌ https://localhost:7001/api/values/notfound/aaaaaa (404 Not Found)
❌ https://localhost:7001/api/values/exception (500 Server Error)
❌ https://localhost:7001/api/values/sdfkdsjfhksdfh (404 Not Found and route doesn't exist)

you could get a full ProblemDetails generation, adding this your code:

builder.Service.AddProblemDetails()

and adding the following middleware:

app.UseExceptionHandler();
app.UseStatusCodePages();

[brunolins16]

Also, it is important to mention, for now, it is not a total replacement to @khellang's middleware, if your app has a lot customizations, eg. Map Exceptions, etc., however, I am planning to create issues, almost sure targeting .NET 8, asking for new features for both ExceptionHandler and StatusCodePages that could at least match with the well-known customization in the current Problem Details middleware.