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

Backport/pr 42384 to release/7.0 preview7 #42809

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

brunolins16
Copy link
Member

Backport of #42384 to release/7.0-preview4

/cc @brunolins16

Adding ProblemDetailsService

Adding a new ProblemDetailsService responsible for consistent Problem Details generation.

Description

API Controllers have a mechanism to auto generated Problem Details (https://datatracker.ietf.org/doc/html/rfc7807) for API Controller ActionResult. The mechanism is enabled by default for all API Controllers, however, it will only generates a Problem Details payload when the API Controller Action is processed and produces a HTTP Status Code 400+ and no Response Body, that means, scenarios like - unhandled exceptions, routing issues - won't produce a Problem Details payload.

Here is overview of when the mechanism will produce the payload:

❌ = Not generated
✅ = Automatically generated

Routing issues: ❌

Unhandled Exceptions: ❌

MVC

  • StatusCodeResult 400 and up: ✅ (based on SuppressMapClientErrors)
    • BadRequestResult and UnprocessableEntityResult are StatusCodeResult
  • ObjectResult: ❌ (Unless a ProblemDetails is specified in the input)
    • eg.: BadRequestObjectResult and UnprocessableEntityObjectResult
  • 415 UnsupportedMediaType: ✅ (Unless when a ConsumesAttribute is defined)
  • 406 NotAcceptable: ❌ (when happens in the output formatter)

Minimal APIs won't generate a Problem Details payload as well.

Here are some examples of reported issues by the community:

This API introduce a new service that is consumed by Diagnostics Middlewares to have the ProblemDetails generated, for all Status 400+ and also have a mechanism that allows devs or library authors (eg. API Versioning) generate ProblemDetails responses when opted-in by the users.

Sample Usage

Default options
var builder = WebApplication.CreateBuilder(args);

// Add services to the containers
builder.Services.AddControllers();
builder.Services.AddProblemDetails();

var app = builder.Build();

// When problemdetails is enabled this overload will work even
// when the ExceptionPath or ExceptionHadler are not configured
app.UseExceptionHandler();

// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
}

//Generate PD for 400+
app.UseStatusCodePages();

app.MapControllers();
app.Run();

Fixes #42212

Customer Impact

This issue has been reported for years, including a community middleware is available to try fix the gap in the framework. This change does not replace the community middleware totally, however, enable the core functionality in the framework.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

This is a new API. The change to existing code is very small and non-impactful unless the new AddProblemDetails method is called.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

* MVC Changes

* ExceptionHandler changes

* Routing changes

* Http.Extensions changes

* Minimal APi draft changes

* HttpResults changes

* New ProblemDetails project

* Using ProblemDetailsOptions in mvc

* ProblemDetails project simplification

* Using metadata

* Using metadata

* Latest version

* Removing changes

* Initial cleanup

* Clean up

* Public api clean up

* Updating public API

* More clean ups

* Adding initial unit tests

* Updating unit tests

* Adding more unit tests

* Cleanup

* Clean up

* clean up

* clean up

* Removing nullable

* Simplifying public api

* API Review feedback

* API review feedback

* Clean up

* Clean up

* clean up

* Reusing Endpoint & EndpointMetadataCollection

* Fix build issues

* Updates based on docs/Trimming.md

* Adding Functional tests

* Fix unittest

* Seal context

* Seal ProblemMetadata

* PR Feeback

* API Review

* Clean  up

* Fixing publicapi  warnings

* Clean up

* PR Feedback

* Fixing build

* Fix unit test

* Fix unit tests

* PR review

* Fixing JsonSerializationContext issues

* Adding statuscode 405

* Update src/Http/Http.Extensions/src/ProblemDetailsDefaultWriter.cs

Co-authored-by: Brennan <brecon@microsoft.com>

* PR review

* Apply suggestions from code review

Co-authored-by: Stephen Halter <halter73@gmail.com>

* Fixing bad merge

* Fix bad merge

* Adding analysis.NextMiddlewareName

* PR review

* Changing to CanWrite/WriteAsync

Co-authored-by: Brennan <brecon@microsoft.com>
Co-authored-by: Stephen Halter <halter73@gmail.com>
@brunolins16 brunolins16 added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jul 19, 2022
@brunolins16 brunolins16 requested a review from a team July 19, 2022 15:40
@brunolins16 brunolins16 changed the title Adding ProblemDetailsService (#42384) Backport/pr 42384 to release/7.0 preview7 Jul 19, 2022
@brunolins16
Copy link
Member Author

brunolins16 commented Jul 19, 2022

The original approved via email backport PR (#42741) was merged targeting main instead of release/7.0-preview-7

@brunolins16 brunolins16 added the Servicing-approved Shiproom has approved the issue label Jul 19, 2022
@brunolins16
Copy link
Member Author

@dotnet/aspnet-build Can I get help merging?

@dougbu
Copy link
Member

dougbu commented Jul 19, 2022

@mmitche this was servicing-approved by Steve yesterday. Can we get it in now w/o disrupting your world❔

@mmitche
Copy link
Member

mmitche commented Jul 19, 2022

Yep go for it.

@dougbu dougbu merged commit 8869a47 into release/7.0-preview7 Jul 19, 2022
@dougbu dougbu deleted the backport-problem-details branch July 19, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants