Skip to content

Improve Results.Problem and Results.ValidationProblem APIs #36856

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 4 commits into from
Sep 23, 2021

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Sep 22, 2021

Description

In .NET 6, we introduced new extension methods as shorthands for creating Results types from endpoint. However, due to an error, we missed adding a parameter to the Results.Problem and Results.ValidationProblem to align with the Extensions property available in those types.

As a result of this, customers are not able to return ProblemDetails or HttpValidationProblemDetails with a configured Extensions type from their APIs.

Customer Impact

Without this fix, customers are not able to return to return ProblemDetails objects

There are no desirable workarounds for this issue since the Results.Problem and Results.ValidationProblem provide a public abstraction over a lot of currently internal functionality, like setting sensible defaults for the title and type of ProblemDetails.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Low risk because:

  • This change resolves an existing gap in the in the Results extension methods API.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Background and Motivation

The Results.Problem and Results.Validation extension methods do not provide a way for users to customize the Extensions property on the resulting ProblemDetails payload.

To support these scenarios, we are adding the extensions parameter to the existing methods and adding new overloads that takes a ProblemDetails or HttpValidationProblemDetails object to provide further customizability for users.

Proposed API

namespace Microsoft.AspNetCore.Http
{
    public static class Results
    {
        public static IResult Problem(
            string? detail = null,
            string? instance = null,
            int? statusCode = null,
            string? title = null,
            string? type = null,
+           IDictionary<string, object?>? extensions = null)
        {
            ...
        }

+       public static IResult Problem(ProblemDetails problemDetails) { ... }

        public static IResult ValidationProblem(
            IDictionary<string, string[]> errors,
            string? detail = null,
            string? instance = null,
            int? statusCode = null,
            string? title = null,
            string? type = null,
+           IDictionary<string, object?>? extensions = null)
        {
            ...
        }
    }
}

Usage Examples

// {
//   "type": "https://tools.ietf.org/html/rfc7231#section-6.6.1",
//   "title": "An error occurred while processing your request.",
//   "status": 500,
//   "green": "foo"
// }
app.MapGet("/sample1", () => 
    Results.Problem(statusCode: 500, extensions: new Dictionary<string, object?>() { { "green", "foo" } }));
// {
//   "type": "https://tools.ietf.org/html/rfc7231#section-6.6.1",
//   "title": "An error occurred while processing your request.",
//   "status": 500,
//   "green": "foo"
// }
app.MapGet("/sample2", () => 
    Results.Problem(new ProblemDetails() { Status = 500, Extensions = { { "green", "foo"}}}));
var errors = new Dictionary<string, string[]>();
// {
//   "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
//   "title": "Bad Request",
//   "status": 400,
//   "green": "foo",
//   "errors": {
    
//   }
// }
app.MapGet("/sample3", () =>
    Results.ValidationProblem(errors, statusCode: 400, extensions: new Dictionary<string, object?>() { { "green", "foo" } }));
// {
//     "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
//     "title": "One or more validation errors occurred.",
//     "status": 400,
//     "green": "foo",
//     "errors": {
        
//     }
// }
app.MapGet("/sample4", () => 
    Results.Problem(new HttpValidationProblemDetails(errors) { Status = 400, Extensions = { { "green", "foo"}}}));

Fixes #36848

@ghost ghost added the area-runtime label Sep 22, 2021
@rafikiassumani-msft rafikiassumani-msft added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Sep 22, 2021
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Add a few smoke tests that the extensions are there and the new overloads work?

@captainsafia captainsafia force-pushed the safia/results-probdeets-fix branch from 06bc3bd to dc173b4 Compare September 22, 2021 21:43
@captainsafia captainsafia marked this pull request as ready for review September 22, 2021 21:54
@captainsafia captainsafia requested a review from a team September 22, 2021 21:55
@captainsafia captainsafia added the Servicing-consider Shiproom approval is required for the issue label Sep 23, 2021
@ghost
Copy link

ghost commented Sep 23, 2021

"Hi captainsafia. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@captainsafia captainsafia changed the base branch from release/6.0-rc2 to release/6.0 September 23, 2021 15:35
@captainsafia
Copy link
Member Author

Closing since we decided to target release/6.0 instead of release/6.0-rc2 with this and I'll need to re-open a separate PR.

@captainsafia captainsafia reopened this Sep 23, 2021
@captainsafia captainsafia changed the base branch from release/6.0 to release/6.0-rc2 September 23, 2021 18:23
@captainsafia
Copy link
Member Author

Closing since we decided to target release/6.0 instead of release/6.0-rc2 with this and I'll need to re-open a separate PR.

Re-opening since this was Servicing approved for RC2. See #36884 (comment).

@captainsafia captainsafia added Servicing-approved Shiproom has approved the issue api-approved API was approved in API review, it can be implemented and removed Servicing-consider Shiproom approval is required for the issue labels Sep 23, 2021
@captainsafia
Copy link
Member Author

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

@BrennanConroy BrennanConroy merged commit 007c622 into release/6.0-rc2 Sep 23, 2021
@BrennanConroy BrennanConroy deleted the safia/results-probdeets-fix branch September 23, 2021 21:00
BrennanConroy added a commit that referenced this pull request Sep 24, 2021
* Update WiX to signed build (#36865)

- #12078

* Always download blazor-hotreload.js from app root (#36897)

The middleware that we inject always listens to the root of the app. While this might not play very nicely if the app
is running off a virtual path, listening to the root is what we do with the aspnet-browser-refresh.js script and we've had no issue
reports with it thus far.

Fixes #35555

* Improve Results.Problem and Results.ValidationProblem APIs (#36856)

* [release/6.0-rc2] Update sdk to 6.0.100-rc.2.21470.55 (#36783)

* Update sdk to 6.0.100-rc.2.21470.55

Co-authored-by: Will Godbe <[email protected]>

Co-authored-by: Doug Bunting <[email protected]>
Co-authored-by: Pranav K <[email protected]>
Co-authored-by: Safia Abdalla <[email protected]>
Co-authored-by: Stephen Halter <[email protected]>
Co-authored-by: Will Godbe <[email protected]>
Co-authored-by: Brennan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented old-area-web-frameworks-do-not-use *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.

6 participants