aspnet / Mvc

[Archived] ASP.NET Core MVC is a model view controller framework for building dynamic web sites with clean separation of concerns, including the merged MVC, Web API, and Web Pages w/ Razor. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
5.62k stars 2.14k forks source link

Consider adding UnprocessableEntityResult and ControllerBase.UnprocessableEntity #6795

Closed khellang closed 7 years ago

khellang commented 7 years ago

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 commented 7 years ago

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

Eilon commented 7 years ago

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

See also https://github.com/aspnet/Mvc/issues/6172

khellang commented 7 years ago

I can do the work if you want 😊

rynowak commented 7 years ago

go for it! 👍

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

khellang commented 7 years ago

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 commented 7 years ago

@rynowak PageBase \ PageModel we intentionally skipped a bunch of API focused IActionResult methods from Razor Pages (See https://github.com/aspnet/Mvc/issues/5846). Presumably this is one the methods that isn't required here either.

Eilon commented 7 years ago

@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 commented 7 years ago

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

vanillajonathan commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.