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.42k stars 10.01k forks source link

Separate type/parsing validation error handling from business logic validation (400 vs 422) #25732

Open tillig opened 4 years ago

tillig commented 4 years ago

We'd like our API to be able to return 400 errors for problems where the inbound payload is syntactically incorrect (e.g., a Boolean is provided where we expected a number, or the inbound body can't be parsed into valid JSON); and 422 errors for semantic errors (e.g., required field missing, or a value is outside an acceptable range). This doesn't seem possible without replacing large sections of the validation framework at the moment.

Related:

In previous issues there was never really a resolution to this.

dotnet/aspnetcore#6145 somewhat led to the desire to remove the hardcoded status code, a PR that was never accepted, but there still wasn't a facility to differentiate between error types.

The "HTTP Semantics" RFC is still on track to include 422 as the way to express business logic validation problems, so the desire is rooted in some level of standards and the desire to adhere to those semantics.

Skipping past some details, all the ProblemDetailsFactory implementations appear to generate their information based on ModelStateDictionary contents... but the ModelStateDictionary drops all the strongly-typed exception information so there's no way to intelligently look for parsing or type conversion errors to differentiate from other validation error types.

Simply intercepting at the InvalidModelStateFactory level doesn't work because, despite getting the whole ActionContext passed in, it's the ActionContext.ModelState you end up building from, and that's already set, the exception information lost.

I think if the exception information could always be kept rather than conditionally thrown out in favor of only keeping the exception message then we would have something to work with. We could use the exception type to determine if there were any parsing/syntactic issues and go 400 with those; and 422 for everything else.

ghost commented 4 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.