Skip to content

Support more ProblemDetails Titles out-of-the-box for Results.Problem() #36417

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

Closed
martincostello opened this issue Sep 11, 2021 · 14 comments · Fixed by #43232
Closed

Support more ProblemDetails Titles out-of-the-box for Results.Problem() #36417

martincostello opened this issue Sep 11, 2021 · 14 comments · Fixed by #43232
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc cost: S Will take up to 2 days to complete feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Priority:3 Work that is nice to have triage-focus Add this label to flag the issue for focus at triage

Comments

@martincostello
Copy link
Member

Is your feature request related to a problem? Please describe.

If a developer using Minimal Actions uses the Results.Problem() method with an HTTP status code that is not part of ProblemDetailsDefaults by default, then without it being explicitly set themselves, a default title is not returned in the response.

Take the following intentionally obscure example:

var builder = WebApplication.CreateBuilder(args);

var app = builder.Build();

app.MapGet("/teapot", () => Results.Problem("I like tea", statusCode: 418));

app.Run();

Calling the /teapot resource will return the following JSON:

{"status":418,"detail":"I like tea"}

If more default HTTP status codes were built in, the default experience would be more informative:

{"title":"I'm a teapot","status":418,"detail":"I like tea"}

Describe the solution you'd like

A more comprehensive list of HTTP status code descriptions are available in the ReasonPhrases class.

However, using it directly in ObjectResult here is not possible as Microsoft.AspNetCore.WebUtilities is not referenced by Microsoft.AspNetCore.Http.Results.

The out-of-the-box experience for Results.Problem() could be improved with one of the following:

  1. Make Microsoft.AspNetCore.Http.Results reference Microsoft.AspNetCore.WebUtilities, and then use ReasonPhrases.GetReasonPhrase(string) as a fallback.
  2. Refactor the HTTP status code descriptions into shared source, and then have both assemblies use it to enhance ObjectResult and/or ProblemDetailsDefaults and to provide the implementation for ReasonPhrases.GetReasonPhrase(string).

Otherwise, the developer has to manually specify the title for any HTTP status codes not already catered for, like this:

// using Microsoft.AspNetCore.WebUtilities;
app.MapGet("/teapot", () => Results.Problem("I like tea", statusCode: 418, title: ReasonPhrases.GetReasonPhrase(418)));

Additional context

Found using .NET SDK version 6.0.100-rc.1.21460.8

@davidfowl davidfowl added feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Sep 11, 2021
@rafikiassumani-msft rafikiassumani-msft added this to the .NET 7 Planning milestone Sep 13, 2021
@rafikiassumani-msft
Copy link
Contributor

Triage: We will re-evaluate this during .NET7 planning.

@rafikiassumani-msft rafikiassumani-msft added triage-focus Add this label to flag the issue for focus at triage Priority:2 Work that is important, but not critical for the release Priority:3 Work that is nice to have cost: S Will take up to 2 days to complete and removed Priority:2 Work that is important, but not critical for the release labels Jan 11, 2022
@martincostello
Copy link
Member Author

@brunolins16 Would your changes in #42384 make this easier to implement with things being moved around in the layers? If you think it does I'll take a look once your PR gets merged.

@brunolins16
Copy link
Member

@martincostello probably not but let me check how close we are, because I did not know about this issue before. Thanks.

@GREsau
Copy link

GREsau commented Jul 28, 2022

I've run into this problem, so an improvement here would be very nice. When I implemented an API Gateway using YARP, it came as a bit of a surprise that Results.Problem(statusCode: 502) produced such an empty response.

Would it also be worth adding more default types? So e.g. the 418 example above may result in:
{"title":"I'm a teapot","status":418,"detail":"I like tea","type":"https://tools.ietf.org/html/rfc2324#section-2.3.2"}

@martincostello
Copy link
Member Author

I'd be happy to take a stab at implementing the changes for this if the aspnetcore team could give a steer as to a solution to the layering question at the top. Of course if it's too big a change to do at this stage without other changes, then it can stay here in the backlog.

@brunolins16
Copy link
Member

@martincostello I'd be more than happy to help on it.

I did not participate in the decision of creating the ProblemDetailsDefaults and not reuse the ReasonPhrases class but, in my opinion, I totally agree with you, and I don't see a reason why we couldn't reference WebUtilities from other layers (again I might have been missing some aspect of this change).

I like this approach:

Refactor the HTTP status code descriptions into shared source, and then have both assemblies use it to enhance ObjectResult and/or ProblemDetailsDefaults and to provide the implementation for ReasonPhrases.GetReasonPhrase(string).

The very initial simple idea that I have in mind, would love to hear @dotnet/minimal-apis thoughts, is change the Phrases dictionary to include the Reference URL without changing how the GetReasonPhrase works, since it is a public API, and maybe expose another public API that expose both information. With that we can update the ProblemDetailsDefaults to use this new method in Apply instead the dictionary owned by ProblemDetailsDefaults (that could be removed).

I see it as a simple change if no problem referencing WebUtilities.

@martincostello
Copy link
Member Author

Thanks for the feedback Bruno. I'll look at pushing up a draft with your suggested preferred approach some time in the next week for you and the team to weigh-in on.

@brunolins16
Copy link
Member

Let me have a discussion about this design before you start.

@khellang
Copy link
Member

khellang commented Aug 5, 2022

@martincostello Will you also update the URLs to RFC9110?

@martincostello
Copy link
Member Author

I wasn't aware of a need to do it, but I can do so when I make the change if that's something the team is fine with.

@khellang
Copy link
Member

khellang commented Aug 5, 2022

Given all the existing URLs point to obsolete RFCs, I think it makes sense 😅

@brunolins16
Copy link
Member

brunolins16 commented Aug 11, 2022

Let me have a discussion about this design before you start.

@martincostello I did not hear anything against referencing WebUtilities, so, feel free to work on a draft. Let me know if I can help you on anything.

@martincostello
Copy link
Member Author

Great, thanks Bruno - I'll try and push up something tomorrow.

@martincostello
Copy link
Member Author

#43232

@ghost ghost locked as resolved and limited conversation to collaborators Sep 26, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc cost: S Will take up to 2 days to complete feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Priority:3 Work that is nice to have triage-focus Add this label to flag the issue for focus at triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants