dotnet / dotnet/aspnetcore

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

Open
#25,732 1 comment 12 reactions 0 assignees View on GitHub
affected-very-few area-mvc enhancement feature-model-binding Needs: Design severity-major
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 6h
Merged PRs (30d)
290

Description

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:
- dotnet/aspnetcore#6145
- aspnet/Mvc#6789

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](https://tools.ietf.org/html/draft-ietf-httpbis-semantics-07#section-9.5.20), 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](https://github.com/dotnet/aspnetcore/issues/6145) 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](https://github.com/dotnet/aspnetcore/blob/3d042ac9aaf6df4cfd1d2bd4a984a86a45629285/src/Mvc/Mvc.Core/src/ApiBehaviorOptions.cs#L25) 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.