Aspnetcore: Reconsider Validation HTTP Status Code

[khalidabuhakmeh]

Currently the validation pipeline in ASP.NET Core returns a HTTP Status code of 400 which means BadRequest.

httpstatuses.com describes a status code of 400 as the following:

400 BAD REQUEST

The server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing).

I believe the validation pipeline should return a 422 which is Unprocessable Entity.

422 UNPROCESSABLE ENTITY

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.

It should be assumed that if the ASP.NET Core pipeline has a chance to get through model binding and run validators, it means the syntax of the request was valid (especially around an API call).

This change would keep the HTTP status code in the family of 4xx error codes.

[rynowak]

This comes up from time to time, different people prefer different things. We're interested in ways to make this super easy for you to configure.

[khellang]

I'm assuming you're referring to the automatic conversion of validation errors into an IActionResult that was introduced with the API conventions stuff? I.e. the ModelStateInvalidFilter?

The InvalidModelStateResponseFactory API was added specifically for this reason, because I moaned about it over a year ago:

https://github.com/aspnet/AspNetCore/blob/afb92018f07bf81c7be092ce93ae36cf1aca5365/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs#L21-L29

It's easy enough to customize it, but it could always be improved. Writing your own quick extension method would be trivial.

services.Configure<ApiBehaviorOptions>(options =>
{
    options.InvalidModelStateResponseFactory = context =>
    {
        // Do whatever you want in here, i.e. return an IActionResult or throw an exception.
    };
});

The default implementation looks like this:

https://github.com/aspnet/AspNetCore/blob/708dc5cb5abd1fe0aa409f355b9e0cea8f6846d4/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/ApiBehaviorOptionsSetup.cs#L98-L103

You could swap BadRequestObjectResult with UnprocessableEntityObjectResult and be done 😄

Otherwise, if you're doing it manually, you can return whatever status code you want. I even added UnprocessableEntityResult, UnprocessableEntityObjectResult and ControllerBase.UnprocessableEntity for convenience.

[RehanSaeed]

@khellang is more of an expert on this (he wrote the code) and can confirm this but I think in 2.2 the default API behaviour is to use the new ProblemDetails response object which has a different InvalidModelStateResponseFactory method (link to full code shown below):

internal static IActionResult ProblemDetailsInvalidModelStateResponse(ActionContext context)
{
    var problemDetails = new ValidationProblemDetails(context.ModelState)
    {
        Status = StatusCodes.Status400BadRequest,
    };

    ProblemDetailsClientErrorFactory.SetTraceId(context, problemDetails);

    var result = new BadRequestObjectResult(problemDetails);

    result.ContentTypes.Add("application/problem+json");
    result.ContentTypes.Add("application/problem+xml");

    return result;
}

So, as well as swapping BadRequestObjectResult with UnprocessableEntityObjectResult, you'd also change the status code in the ValidationProblemDetails object.

[RehanSaeed]

With the above said, I still think there is a case for changing the default to 422 because it is more 'correct'. Using the compatibility version method would be the way to go to do it:

services.SetCompatibilityVersion(CompatibilityVersion.Version_3_0)

[khellang]

To be honest, I think the explicit status code here should be removed:

https://github.com/aspnet/AspNetCore/blob/afb92018f07bf81c7be092ce93ae36cf1aca5365/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/ApiBehaviorOptionsSetup.cs#L108-L111

As MVC now sets the ProblemDetails status code if none has been specified, based on the status code of the result itself:

https://github.com/aspnet/AspNetCore/blob/afb92018f07bf81c7be092ce93ae36cf1aca5365/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ObjectResult.cs#L61-L64

[mkArtakMSFT]

@khellang, would you like to send us a PR for this?

[khellang]

I'll see if I can squeeze it in. Is there any way this can be assigned to me?

[mkArtakMSFT]

@khellang, feel free to submit a PR and we'll consider it.

[mkArtakMSFT]

@khellang, and it isn't really possible to assign issues to non-org members.

[khellang]

it isn't really possible to assign issues to non-org members.

Yes, it is. I've been assigned multiple issues in corefx/coreclr 😊

https://help.github.com/en/articles/adding-outside-collaborators-to-repositories-in-your-organization

[mkArtakMSFT]

Thanks for contacting us. We're closing this issue as there was no community involvement here for quite a while now.
And as per an earlier response, this is a breaking change we can't afford. And the scenario is already possible to be done by customers.

[khellang]

FYI; httpbis is working on a new HTTP Semantics RFC, obsoleting the current HTTP/1.1 RFCs. One of the interesting things in there is that they're pulling 422 Unprocessable Entity from WebDAV and changing the reason phrase to Unprocessable Payload, with the following semantics:

The 422 (Unprocessable Payload) status code indicates that the server understands the content type of the request payload (hence a 415 (Unsupported Media Type) status code is inappropriate), and the syntax of the request payload is correct, but was unable to process the contained instructions. For example, this status code can be sent if an XML request payload contains well-formed (i.e., syntactically correct), but semantically erroneous XML instructions.

From the draft.

As @rynowak stated in https://github.com/aspnet/Mvc/issues/6789#issuecomment-328129654:

Some people are bothered by the fact that 422 is defined by the WebDAV RFC and it's not mentioned at all in RFC7231 which came 7 years later.

So at least that won't be an issue anymore (assuming the draft is approved) 😄

[rynowak]

For example, this status code can be sent if an XML request payload contains well-formed (i.e., syntactically correct), but semantically erroneous XML instructions.

I really wish that they would come out and say - this is for communicating validation/business-rule errors. HTTP is apparently a layer 7 protocol, but you wouldn't get that impression from reading the spec 😢

Using phrases like "contained instructions" doesn't match my mental model of what web servers/frameworks do, and is very hard to reconcile with the semantics of REST. /rant

That said, I look forward to seeing this clarified - I still think the spec is pretty unclear, but it's like a 5/10 in unclearness instead of a 7/10.