Skip to content

Problem details /2 #27009

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

Merged
merged 40 commits into from
Sep 28, 2022
Merged

Problem details /2 #27009

merged 40 commits into from
Sep 28, 2022

Conversation

Rick-Anderson
Copy link
Contributor

@Rick-Anderson Rick-Anderson commented Sep 15, 2022

@Rick-Anderson
Copy link
Contributor Author

Public review of the doc build:

  • download the HTML files
  • unzip
  • view HTML file in a browser.
  • For links outside the doc, remove the review. from URL.
    @sammychinedu2ky start at

Client error response

@sammychinedu2ky
Copy link
Contributor

sammychinedu2ky commented Sep 21, 2022

Public review of the doc build:

  • download the HTML files
  • unzip
  • view HTML file in a browser.
  • For links outside the doc, remove the review. from URL.
    @sammychinedu2ky start at

Client error response

Thanks @Rick-Anderson 😅, but I wasn't able to see how the problem was resolved since the controller writes the problem details to stream before the middleware handles it.

Since there is no way around it than changing the controller like this:

image

Should I leave the code in the issue as it is since you explained why in the post?

@Rick-Anderson
Copy link
Contributor Author

Should I leave the https://github.com/dotnet/AspNetCore.Docs.Samples/issues/60as it is since you explained why in the post?

No, do a PR and create a 3rd controller named Values3Controller. After explaining the problem we'll show the work around.

@sammychinedu2ky
Copy link
Contributor

Should I leave the https://github.com/dotnet/AspNetCore.Docs.Samples/issues/60as it is since you explained why in the post?

No, do a PR and create a 3rd controller named Values3Controller. After explaining the problem we'll show the work around.

I can only see one controller from my end or am I mistaken?

@sammychinedu2ky
Copy link
Contributor

@Rick-Anderson

@Rick-Anderson
Copy link
Contributor Author

@sammychinedu2ky
Copy link
Contributor

https://github.com/dotnet/AspNetCore.Docs.Samples/blob/main/fundamentals/middleware/problem-details-service/Controllers/ValuesController.cs#L39

2 controllers, one file. Put the 3rd controller in the same file.

Ooh I need to pull, I don't have this locally 😅

@Rick-Anderson
Copy link
Contributor Author

Ooh I need to pull, I don't have this locally 😅

Never do a PR from your old branch.

@sammychinedu2ky
Copy link
Contributor

Ooh I need to pull, I don't have this locally 😅

Never do a PR from your old branch.

Sure thanks

Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I noted just a couple of nits.

@Rick-Anderson Rick-Anderson marked this pull request as ready for review September 22, 2022 21:52
Copy link
Member

@brunolins16 brunolins16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I have some suggestions. Also, not directly related to your changes but will be great if we could:

  • Remove this topic ## Custom Middleware to handle exceptions
  • Update Produce a ProblemDetails payload for exceptions to keep mentioning the community middleware as an addition to what we have now.

@Rick-Anderson
Copy link
Contributor Author

The sample app also contains the following database context class:

dead wood

@brunolins16
Copy link
Member

The sample app also contains the following database context class:

dead wood

Does "dead wood" mean something special here? 😂

Copy link
Contributor Author

@Rick-Anderson Rick-Anderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brunolins16 start at Internal review URL
sample code
I changed it to say

The automatic creation of a ProblemDetails for error status codes is enabled by default, but error responses can be configured in one of the following ways:

Then I added a new section just below

Default problem details response

I think I responded to most of your suggestions. Let me know what else. We might need to do a 15 minute teams meeting.

### Produce a ProblemDetails payload for exceptions

ASP.NET Core doesn't produce a standardized error payload when an unhandled exception occurs. For scenarios where it's desirable to return a standardized [ProblemDetails response](https://datatracker.ietf.org/doc/html/rfc7807) to the client, the [ProblemDetails middleware](https://www.nuget.org/packages/Hellang.Middleware.ProblemDetails/) can be used to map exceptions and 404 responses to a [ProblemDetails](xref:web-api/handle-errors#pd) payload. The exception handling middleware can also be used to return a <xref:Microsoft.AspNetCore.Mvc.ProblemDetails> payload for unhandled exceptions.

<!-- TODO UPDATE • Update Produce a ProblemDetails payload for exceptions to keep mentioning the community middleware as an addition to what we have now.-->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update Produce a ProblemDetails payload for exceptions to keep mentioning the community middleware as an addition to what we have now.

@brunolins16 what updates are needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rick-Anderson This statement is not true ASP.NET Core doesn't produce a standardized error payload when an unhandled exception occurs. For scenarios where it's desirable to return a standardized [ProblemDetails response] anymore. Maybe we can update to say that with the AddProblemDetails + middleware (StatusCodeMiddleware and ExceptionHandlerMiddleware) we will produce the ProblemDetails response but they can continue using the ProblemDetails middleware or configure manually the ExceptionHandler to write the PR. make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have feedback about one single statement but other than that it is pretty good.

Copy link
Member

@brunolins16 brunolins16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rick-Anderson the content is pretty good, thanks a lot.

}
```

For most apps, the preceding code is all that is needed as unhandled exceptions are rare.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brunolins16 can I stay that? All that's needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the sample code? If yes, the lines that you need to highlight for a non-dev environment are:

builder.Services.AddProblemDetails();
app.UseExceptionHandler();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why exactly, but this statement does not sound good to me For most apps, the preceding code is all that is needed as unhandled exceptions are rare. Maybe you could say something like For most apps, the preceding code is all that is needed, however, if you want to customize [point to the customization topic]


For most apps, the preceding code is all that is needed as unhandled exceptions are rare.

An alternative to a [custom exception handler page](xref:fundamentals/error-handling#exception-handler-page) is to provide a lambda to <xref:Microsoft.AspNetCore.Builder.ExceptionHandlerExtensions.UseExceptionHandler%2A>. Using a lambda allows access to the error and writing a problem details response with [`IProblemDetailsService.WriteAsync`](/dotnet/api/microsoft.aspnetcore.http.iproblemdetailsservice.writeasync?view=aspnetcore-7.0&preserve-view=true):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the strategy for to content duplication?

@Rick-Anderson Rick-Anderson merged commit cdc26d9 into main Sep 28, 2022
@Rick-Anderson Rick-Anderson deleted the nMonPrep branch September 28, 2022 00:25
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

Successfully merging this pull request may close these issues.

Consumes Attribute 415 return format
4 participants