ModelState.IsValid or Model.IsValid?

Question

I'm writing a controller and unit tests for it, when I came across two ways (equally valid I think) to do something. All my models have an IsValid property, which I can check to ask the model if it's valid or not.

On postback to a controller action method, if the model is valid I want to save, otherwise I want to redisplay the form for the user to correct their errors.

My initial thought was to just verify that the model is being asked if it's valid, but I realized I could also check ModelState.IsValid.

Does anyone have any particular reason to look at one vs the other?

score 12 · Accepted Answer · answered Feb 16 '11 at 00:35

I think having custom business validation built into your model is a fine approach. The way I would handle it would be to add any custom validation errors to the ModelState:

if (ModelState.IsValid)
{
    if (!model.IsValid)
    {
       ModelState.AddModelError("The model is not valid");
    }
    else
    {
        return RedirectToAction("Index");
    }
}

return View(model);

That way your view has access to the validation errors regardless of whether they are custom or built in.

LukLed · Answer 2 · 2011-02-16T01:02:27.230

7

ModelState can be transferred to TempData to follow Post-Redirect-Get. Example:

    [HttpPost]
    [ExportModelStateToTempData]
    public ActionResult Delete(int id)
    {
        if (_service.DeleteTask(id))
            return RedirectToAction(ControllerActions.Index);

        return RedirectToAction(ControllerActions.Edit, new { id });
    }

    [ImportModelStateFromTempData]
    public ActionResult Edit(int id)
    {
        var task = _service.GetTask(id);
        return View(ControllerActions.Edit, GetEditModel(task));
    }

User can delete task by callig /Task/Delete action, but if something goes wrong and error message appears, pressing F5 won't call deletion again. when ModelState after Delete is transferred to Edit, all errors are shown on edit page.

This is code for attributes importing/exporting ModelState:

public abstract class ModelStateTempDataTransferAttribute : ActionFilterAttribute
{
    protected static readonly string Key = typeof(ModelStateTempDataTransferAttribute).FullName;
}

public class ExportModelStateToTempDataAttribute : ModelStateTempDataTransferAttribute
{
    public override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        //Only export when ModelState is not valid
        if (!filterContext.Controller.ViewData.ModelState.IsValid)
        {
            //Export if we are redirecting
            if ((filterContext.Result is RedirectResult) || (filterContext.Result is RedirectToRouteResult))
            {
                filterContext.Controller.TempData[Key] = filterContext.Controller.ViewData.ModelState;
            }
        }

        base.OnActionExecuted(filterContext);
    }
}

public class ImportModelStateFromTempDataAttribute : ModelStateTempDataTransferAttribute
{
    public override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        ModelStateDictionary modelState = filterContext.Controller.TempData[Key] as ModelStateDictionary;

        if (modelState != null)
        {
            //Only Import if we are viewing
            if (filterContext.Result is ViewResult)
            {
                filterContext.Controller.ViewData.ModelState.Merge(modelState);
            }
            else
            {
                //Otherwise remove it.
                filterContext.Controller.TempData.Remove(Key);
            }
        }

        base.OnActionExecuted(filterContext);
    }
}

Doing the same with Model would be very problematic.

edited Feb 16 '11 at 01:02

answered Feb 16 '11 at 00:57

LukLed

31,452
17
82
107

What is your `ControllerActions` class? That is new to me. – Matt Greer Feb 16 '11 at 01:24
@Matt Greer: That is just class with constant names of actions, just to have them strongly typed. if you want to play with stronlgy typed names, T4MVC is probably way to go. – LukLed Feb 16 '11 at 01:28
So just a collection of const strings? Drat, I was hoping you had a clever solution to avoiding magic strings. Magic strings have become my biggest pet peeve in recent .NET :) – Matt Greer Feb 16 '11 at 01:29
@Matt Greer: T4MVC once again. – LukLed Feb 16 '11 at 01:29
Awesome, thanks! I am new to ASP.NET MVC and had not heard of this. – Matt Greer Feb 16 '11 at 01:36
Sorry, but I don't see at all how this is relevent to my question. – Andy Feb 16 '11 at 14:19
@Andy: I wanted to say that `ModelState` is useful and you shouldn't resign from using it/part of it. Why did you add `IsValid` to your models? – LukLed Feb 16 '11 at 14:37
@LukLed: I realize `ModelState` is useful. IsValid was added because you should be able to determine the validity of a model outside of any UI context. So IsValid is the result of evaluating the DataAnnotations. Also, it allows for more complex rules to be run which can't (or are difficult) with attributes. My question is probably not quite right either. I suppose my issue is that MVC can look at the attributes and say the model is valid, while a non-attribute rule may cause it to be invalid. So I think I should be checking my model, but make sure to update `ModelState` with add'l info. – Andy Feb 17 '11 at 14:22
@Andy: If you validate inside of model class, you can do the same with attribute. There is no reason not to use attribute, so there is no reason to create additional validations. – LukLed Feb 18 '11 at 01:27
No, there are some validation which is not appropriate to do in an attribute. Namely anything that would require you to break encapuslation. A good reason not to use attributes is that running them on your own (ie, similating what MVC would do) is non trivial and error prone. – Andy Feb 19 '11 at 16:40
@Andy: Running validation on your own is not that complicated. I really see no reason to check for validity of model twice. If you accepted MrDustpan's answer then remember, that you can also upvote him. – LukLed Feb 19 '11 at 16:47

score 0 · Answer 3 · answered Feb 18 '16 at 16:13

You can can also enumerate through the collection of errors and write the Exact error message to the output.

else if (!ModelState.IsValid)
{
    List<ModelError> modelErrors = new List<ModelError>();
    ModelErrorCollection errors = new ModelErrorCollection();
                              ;
    // got to the model and look at the Array of Values
    foreach (var item in ModelState)
    {
        if (item.Value.Errors.Count != 0)
        {
            // get the error
            errors = item.Value.Errors;
            modelErrors.Add(errors.Single());
        }
     }

     if (modelErrors.Count != 0)
     {
         foreach (var error in modelErrors)
         {
             Debug.WriteLine(error.ErrorMessage);
         }
      }

...

ModelState.IsValid or Model.IsValid?

3 Answers3

Linked