Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Consider adding UnprocessableEntityResult and ControllerBase.UnprocessableEntity #6795

Closed
khellang opened this issue Sep 8, 2017 · 11 comments
Assignees
Labels
3 - Done enhancement up-for-grabs Members of our awesome commnity can handle this issue

Comments

@khellang
Copy link
Contributor

khellang commented Sep 8, 2017

422 Unprocessable Entity is commonly used for semantic validation errors in APIs, as opposed to 400 Bad Request for syntactical errors:

The 422 (Unprocessable Entity) status code means the server
understands the content type of the request entity (hence a
415(Unsupported Media Type) status code is inappropriate), and the
syntax of the request entity is correct (thus a 400 (Bad Request)
status code is inappropriate) but was unable to process the contained
instructions. For example, this error condition may occur if an XML
request body contains well-formed (i.e., syntactically correct), but
semantically erroneous, XML instructions.

It would be nice if MVC included an UnprocessableEntityResult and an accompanying ControllerBase.UnprocessableEntity method.

They'd basically mirror what BadRequestResult and ControllerBase.BadRequest does, only with a different status code.

This has been requested before by @ashmind, but was never followed up.

@Eilon
Copy link
Member

Eilon commented Sep 11, 2017

Thanks for the suggestion, we'll review the request in our next bug triage meeting.

@Eilon Eilon added 1 - Ready enhancement up-for-grabs Members of our awesome commnity can handle this issue and removed super-triage labels Sep 15, 2017
@Eilon
Copy link
Member

Eilon commented Sep 15, 2017

Seems reasonable. We need to add it to both MVC's ControllerBase and to Razor Pages' PageModelBase.

See also #6172

@khellang
Copy link
Contributor Author

I can do the work if you want 😊

@rynowak
Copy link
Member

rynowak commented Sep 17, 2017

go for it! 👍

Should include ControllerBase, PageModelBase and PageBase https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.RazorPages/PageBase.cs

@khellang
Copy link
Contributor Author

khellang commented Sep 19, 2017

Hmm... I can't seem to find PageModelBase. Did you mean PageModel?

Also, there's no existing BadRequest methods on PageBase (or PageModel), so it seems it's already missing quite a few other, similar methods. Should I still go ahead and add UnprocessableEntity anyway? I think it would make sense to have a common base class for pages and controllers here.

@pranavkm
Copy link
Contributor

@rynowak PageBase \ PageModel we intentionally skipped a bunch of API focused IActionResult methods from Razor Pages (See #5846). Presumably this is one the methods that isn't required here either.

@Eilon
Copy link
Member

Eilon commented Sep 20, 2017

@DamianEdwards / @rynowak - thoughts on adding various other missing result factory methods? Were we trying to avoid more API-ish things appearing in Razor Pages or anything like that? Or do we want parity?

@rynowak
Copy link
Member

rynowak commented Sep 20, 2017

Yeah, let's leave it out for now. No one has asked for it

pranavkm pushed a commit that referenced this issue Sep 22, 2017
…ntrollerBase.UnprocessableEntity methods (#6851)

* Added UnprocessableEntityResult

* Added UnprocessableEntityObjectResult

* Added UnprocessableEntity overloads to ControllerBase

Fixes #6795
@vanillajonathan
Copy link

Should the BadRequest method which takes a ModelStateDictionary as parameter be deprecated in favor of the new UnprocessableEntity method?

public virtual BadRequestObjectResult BadRequest(ModelStateDictionary modelState);

Perhaps the method should be marked with the Obsolete attribute?

@vanillajonathan
Copy link

Note that the ValidationProblem method introduced in ASP.NET Core 2.1 have a overload that takes a ModelStateDictionary as parameter and returns a 400 status code.

@khellang
Copy link
Contributor Author

It's not obsolete, it's just that there's no consensus on using 400 for validation errors. Some people use 400 for all client errors, like syntax and validation, while some prefer 400 for syntax error and 422 for semantic validation errors. MVC will still default to 400.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3 - Done enhancement up-for-grabs Members of our awesome commnity can handle this issue
Projects
None yet
Development

No branches or pull requests

5 participants