search

dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.22k stars 9.95k forks source link

Support Latest ProblemDetails RFC (July 2023, RFC 9457) #52414

Open jeremy-allocate opened 9 months ago

jeremy-allocate commented 9 months ago

Is there an existing issue for this?

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

This issue is not related to a problem, but there was a new RFC published in July 2023 that obsoletes RFC 7807 which as best I can find seems to be the basis for the current ProblemDetails support in ASP.NET core. Just filing this as a placeholder to support anything in the new RFC that isn't covered today (there seems to be a gap from what I've read, but I haven't dived in at the field level yet).

Describe the solution you'd like

Support for the new RFC.

Additional context

Old: https://datatracker.ietf.org/doc/html/rfc7807 New: https://www.rfc-editor.org/rfc/rfc9457.html#name-introduction

captainsafia commented 9 months ago

@jeremy-allocate Thanks for reporting this issue! The delta between the two RFCs is relatively small.

Section 4.2 introduces a registry of common problem type URIs

It doesn't look like there's a source code reaction we need to introduce here. It mostly codifies what the existing supported type values should be.

Section 3 clarifies how multiple problems should be treated

From my understanding, the guidance here clarifies how ProblemDetails should be constructed if errors need to be reported from disparate types. I think this primarily affects how we communicate problem details when validating multiple parameters of disparate types. I'll have to double check what MVC's handling here is. The guidance seems to recommend that you should only produce errors for a single type.

Section 3.1.1 provides guidance for using type URIs that cannot be dereferenced

It looks like this is documenting how end-users might take advantage of unreferenced URLs (future-proofing) and doesn't affect much of the implementation.

So, it looks like there's not a major delta for us to resolve here with the changes in the RFC.

Was there something in particular you spotted that would need modification?

ghost commented 9 months ago

Hi @jeremy-allocate. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

jeremy-allocate commented 9 months ago

Hi @captainsafia - thanks! This may well be a no-op; as I mentioned in the description, "just filing this as a placeholder to support anything in the new RFC that isn't covered today (there seems to be a gap from what I've read, but I haven't dived in at the field level yet)." So performing the delta analysis was a big part of the ask here.

But in reading your findings though I have these thoughts/reactions:

Section 4.2: It would be good to confirm that what they codified jives with the defaults in https://github.com/dotnet/aspnetcore/blob/3094ea15c911c95a8adf242adccb4cbfa7141684/src/Shared/ProblemDetails/ProblemDetailsDefaults.cs (bummed this is internal fwiw)

Section 3: This guidance is interesting, "When an API encounters multiple problems that do not share the same type, it is RECOMMENDED that the most relevant or urgent problem be represented in the response." - for a model with two model validation failures that are caught by the Web API framework itself, who exactly is to say which of the two errors is more relevant or urgent? Continuing to return multiple errors in such a case would be my personal preference despite the recommendation, so a UI could display all model validation failures back to the user via a single round trip, rather than requiring multiple fix/submit round-trips.

Regardless of if you determine code changes are needed here or not, it would be great to update any dotnet documentation that says which version of the ProblemDetails RFC is supported to mention 9457 instead (so those coming after us know if the analysis of if dotnet is compliant with the new RFC has been performed or not).

captainsafia commented 8 months ago

Section 4.2: It would be good to confirm that what they codified jives with the defaults in

Based on a spot check, it looks like the defaults here match what's expected in the spec.

(bummed this is internal fwiw)

There's an open API review issue for this over at https://github.com/dotnet/aspnetcore/issues/47978. There's some open unaddressed feedback on it but you're welcome to champion it if particularly interested. It's not a high prioerity item for the team at the moment but a community contribution would be welcome!

Section 3: This guidance is interesting, "When an API encounters multiple problems that do not share the same type, it is RECOMMENDED that the most relevant or urgent problem be represented in the response."

Yep, we actually don't follow the guidance out-of-the-box when it comes to MVC. For example, for a route controller like:

public IActionResult Get([Range(0, 10)] int id, DataFoo input)

Providing invalid data for the id route parameter and the body payload will produce a response with both validation errors.

Similar to you, I prefer getting all the issues reported at once. There's a fair amount of customizability in our model binding layer so users can implement different behavior if they prefer. I'm personally fine maintaining this behavior since it's a recommendation of the spec and not a hard requirement and changing it at this point would be a bothersome breaking change.

Regardless of if you determine code changes are needed here or not, it would be great to update any dotnet documentation that says which version of the ProblemDetails RFC is supported to mention 9457 instead (so those coming after us know if the analysis of if dotnet is compliant with the new RFC has been performed or not).

Good point. We currently link to the older version of the RFC (ref) but I've opened https://github.com/dotnet/AspNetCore.Docs/pull/31345 to resolve this.

KillerBoogie commented 2 weeks ago

Section 3: This guidance is interesting, "When an API encounters multiple problems that do not share the same type, it is RECOMMENDED that the most relevant or urgent problem be represented in the response."

Providing invalid data for the id route parameter and the body payload will produce a response with both validation errors. Similar to you, I prefer getting all the issues reported at once.

To my understanding RFC 9457 encourages to respond with all validation errors. The details are just nested under the "main" error without repeating the common fields of the various errors (see 2nd example in the RFC). I just don't understand the sense of having no separate problem type for each error anymore. The different structure of multiple errors compared to a single error makes the structure complicated for the client.