Why do formatters unnecessarily double validate the Required fields?

Topics: ASP.NET Web API
Sep 20, 2012 at 9:41 AM
Edited Sep 20, 2012 at 9:42 AM

Assuming I sent the following request to my API:

POST http://localhost:4940/api/cars HTTP/1.1
User-Agent: Fiddler
Host: localhost:4940
Content-Type: application/json
Content-Length: 44

{"Make":"Make1","Year":2010,"Price":10732.2}

And I have the following Car class definition: 

public class Car {

    public int Id { get; set; }

    [Required]
    [StringLength(20)]
    public string Make { get; set; }

    [Required]
    [StringLength(20)]
    public string Model { get; set; }

    public int Year { get; set; }

    [Range(0, 500000)]
    public float Price { get; set; }
}

The controller I have is as follows:

[InvalidModelStateFilter]
public class CarsController : ApiController {
        
    private readonly CarsContext _carsCtx = 
        new CarsContext();
	
    // GET /api/cars
    public IEnumerable<Car> Get() {

        return _carsCtx.All;
    }

    // POST /api/cars
    public HttpResponseMessage PostCar(Car car) {
            
        var createdCar = _carsCtx.Add(car);
        var response = Request.CreateResponse(
            HttpStatusCode.Created, createdCar);

        response.Headers.Location = new Uri(
            Url.Link("DefaultHttpRoute", 
                new { id = createdCar.Id }));

        return response;
    }

    // PUT /api/cars/{id}
    public Car PutCar(int id, Car car) {

        car.Id = id;

        if (!_carsCtx.TryUpdate(car)) {

            var response = Request.CreateResponse(
                HttpStatusCode.NotFound);

            throw new HttpResponseException(response);
        }

        return car;
    }
}

InvalidModelStateFilter is just an action filter which handles the ModelState errors:

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
public class InvalidModelStateFilterAttribute : ActionFilterAttribute {

    public override void OnActionExecuting(HttpActionContext actionContext) {

        if (!actionContext.ModelState.IsValid) {

            actionContext.Response = actionContext.Request.CreateErrorResponse(
                HttpStatusCode.BadRequest, actionContext.ModelState);
        }
    }
}

The response I got back is as follows: 

HTTP/1.1 400 Bad Request
Cache-Control: no-cache
Pragma: no-cache
Content-Type: application/json; charset=utf-8
Expires: -1
Server: Microsoft-IIS/8.0
X-AspNet-Version: 4.0.30319
X-SourceFiles: =?UTF-8?B?RTpcRHJvcGJveFxCb29rc1xQcm9XZWJBUEkuU2FtcGxlc1xDaGFwdGVyMTNcRGF0YUFubm90YXRpb25WYWxpZGF0aW9uQXR0cmlidXRlc1NhbXBsZVxEYXRhQW5ub3RhdGlvblZhbGlkYXRpb25BdHRyaWJ1dGVzU2FtcGxlXGFwaVxjYXJz?=
X-Powered-By: ASP.NET
Date: Mon, 17 Sep 2012 11:38:58 GMT
Content-Length: 182

{"Message":"The request is invalid.","ModelState":{"car":["Required property 'Model' not found in JSON. Path '', line 1, position 44."],"car.Model":["The Model field is required."]}}

Here is the more readable form of the message body: 

{
    "Message": "The request is invalid.",
    "ModelState": {
        "car": [
            "Required property 'Model' not found in JSON. Path '', line 1, position 44."
        ],
        "car.Model":[
            "The Model field is required."
        ]
    }
}

As you can see, I also have one additional ugly error message for car which containst the info about the path, line, etc. info.

I debugged the source code and it tuns out the JsonMediaTypeFormatter throws an error if a Property is marked with Required and not supplied. the following code is a part from theJsonMediaTypeFormatter:

// Error must always be marked as handled
// Failure to do so can cause the exception to be rethrown at every recursive level and overflow the stack for x64 CLR processes
jsonSerializer.Error += (sender, e) =>
{
    Exception exception = e.ErrorContext.Error;
    formatterLogger.LogError(e.ErrorContext.Path, exception);
    e.ErrorContext.Handled = true;
}

and this triggers the ModelStateFormatterLogger.LogError method which puts the error inside theModelState:

public void LogError(string errorPath, Exception exception)
{
    if (errorPath == null)
    {
        throw Error.ArgumentNull("errorPath");
    }
    if (exception == null)
    {
        throw Error.ArgumentNull("exception");
    }

    string key = ModelBindingHelper.ConcatenateKeys(_prefix, errorPath);
    _modelState.AddModelError(key, exception);
}

It tuns out that abstract MediaTypeFormatter class holds a property called RequiredMemberSelector which is type of IRequiredMemberSelector and the default implementation for this is ModelValidationRequiredMemberSelector class and this is completely useless class IMO.

This class is not only useless but also behaves wrong. E.g: If I send a POST request as below, I should get a validation error because I didn't set AllowEmptyStrings to true for RequiredAttribute:

POST http://localhost:4940/api/cars HTTP/1.1
User-Agent: Fiddler
Host: localhost:4940
Content-Type: application/json
Content-Length: 59

{"Make": "","Model":"Model123","Year":2012,"Price":10982.2}

And I got the validation error but this time, the formatter doesn't throw the validation error which is wrong (according to what is being tried to be done here):

{
    "Message":"The request is invalid.",
    "ModelState": {
        "car.Make": [ 
            "The Make field is required."
        ]
    }    
}

To suppress this unnecessary and useless behavior, I can supply a custom GlobalConfiguration.Configuration.Formatters.JsonFormatter.RequiredMemberSelect‌​or for my formatter or change the config.Formatters.JsonFormatter.SerializerSettings.ContractResolver which handles this validation issue.

Now, I might be totally off-base here but I really don't understand why this behavior is there. Is it because of value types? If so, I think that's completely unnecessary as the user should handle those on his/her own.

Figuring this out took me long time and I am sure that this does nothing hut confuses people.

Sep 20, 2012 at 6:40 PM
Edited Sep 20, 2012 at 6:40 PM

Hi there,

Sorry for the confusion, but here's what's happening:

The [Required] attribute actually has two meanings:

  1. That the property is present in the request body.
  2. That the property is not null after it has been deserialized.

Formatters are responsible for raising model state errors when a required property is missing from the stream, and the value validation component is responsible for running all the validation attributes (including [Required]) and raising the appropriate errors.

What you're seeing for the double validation is actually:

  1. The formatter realizing the property isn't in the request body and raising an error
  2. The value validation component running the code for RequiredAttribute.IsValid and raising an error because the property is null after it has been deserialized

The reason you wouldn't see a second error for Make in these cases:

 {"Make": "","Model":"Model123","Year":2012,"Price":10982.2}

{"Make": null,"Model":"Model123","Year":2012,"Price":10982.2}

is because Make is actually on the wire so the formatter doesn't raise a model error, but the [Required] attribute still raises a validation error.

Now, what you'll want to do depends on what kind of validation you want. Do you want to say that "Make" must be on the wire, or do you want to say that "Make" must not be null after it's deserialized?

If you want to only say that "Make" must be on the wire, but that Make can be null in the request, then you could use [DataMember(IsRequired=true)] instead. That attribute has meaning #1, but without the meaning that the property cannot be null after it has been deserialized. If instead, you just want the meaning that the property shouldn't be null after being deserialized, then you could choose to define your own attribute that doesn't have [Required]'s extra special meaning like this:

    public class NotNullAttribute : ValidationAttribute
    {
        protected override ValidationResult IsValid(object value, ValidationContext validationContext)
        {
            if (value == null)
            {
                return new ValidationResult("Property should not be null.");
            }
            return null;
        }
    }

You can also turn off formatter validation altogether by setting the RequiredMemberSelector to null, or you can right your own custom validation for deserialized objects by replacing the default implementation of IBodyModelValidator by your own in config.Services. Hopefully, this explains things a bit. Please let me know if you have any more questions or concerns,

Youssef

Sep 20, 2012 at 7:20 PM

Hi youssefm,

First of all, thanks for taking the time to give this detailed explanation. It's very informative.

So, formatters basically provide the additional functionality for Required attributes to be applicable on value types. Isn't that correct?

However, the default behavior is still an overkill (even if it is easy to override it). If I have a reference type and DataAnnotationValidatorProvider registred, there is no need formatters to take required attributes into consideration and stick additional validation errors into ModelState.

Wouldn't u think the same?

Sep 20, 2012 at 8:06 PM

Yes, that is correct.

I see your point. We could definitely change the default behavior to not ask formatters to check that the property is on the wire if it's a reference type that is marked as [Required]. That would avoid duplicate errors for reference types while preserving required member model errors for value types.

If you'd like to create an issue, I'll be glad to see what we can do about this. You can also submit a pull request if you're interested in contributing.

Youssef

Sep 20, 2012 at 8:37 PM
Edited Sep 20, 2012 at 8:44 PM

Thanks for the answer again. Ok, I'll look into it whether I can pull it off. Otherwise I'll open up an issue.


From: "youssefm" <notifications@codeplex.com>
Date: 20 Sep 2012 13:06:44 -0700
To: <tugberkugurlu@gmail.com>
ReplyTo: ASPNETWebStack@discussions.codeplex.com
Subject: Re: Why do formatters unnecessarily double validate the Required fields? [ASPNETWebStack:396199]

From: youssefm

Yes, that is correct.

I see your point. We could definitely change the default behavior to not ask formatters to check that the property is on the wire if it's a reference type that is marked as [Required]. That would avoid duplicate errors for reference types while preserving required member model errors for value types.

If you'd like to create an issue, I'll be glad to see what we can do about this. You can also submit a pull request if you're interested in contributing.

Youssef

Oct 31, 2012 at 11:06 PM

Filed this bug to handle fixing this for vNext:

http://aspnetwebstack.codeplex.com/workitem/590

Thanks for reporting the issue.