Skip to content

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

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

Open
tillig opened this issue Sep 9, 2020 · 1 comment
Labels
affected-very-few This issue impacts very few customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding Needs: Design This issue requires design work before implementating. severity-major This label is used by an internal tool
Milestone

Comments

@tillig
Copy link
Contributor

tillig commented Sep 9, 2020

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.

#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.

@pranavkm pranavkm added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Sep 9, 2020
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Sep 10, 2020
@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Sep 10, 2020
@ghost
Copy link

ghost commented Sep 10, 2020

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.

@mkArtakMSFT mkArtakMSFT added the Needs: Design This issue requires design work before implementating. label Sep 10, 2020
@SteveSandersonMS SteveSandersonMS added affected-very-few This issue impacts very few customers severity-major This label is used by an internal tool labels Oct 7, 2020 — with ASP.NET Core Issue Ranking
@mkArtakMSFT mkArtakMSFT 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-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Oct 14, 2021
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-very-few This issue impacts very few customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding Needs: Design This issue requires design work before implementating. severity-major This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

6 participants