Is it correct way to use ModelState.Remove to deal with ModelState?

Question

Im working on a big MVC3 web application and have an annoyance regarding the ModelState.IsValid method.

ModelState is being used in nearly all of my controllers so to validate the data being posted. The views are all based on ViewModels which contain different classes and these classes obviously contain properties which could be marked as [Required].

The problem i am having is the required properties are sometimes not required and im having to use the ModelState.Remove method so that ModelState.IsValid becomes true.

My question is by using ModelState.Remove, is this the correct way of doing things or is there a more efficient approach.

Sepsis

that is different issue but it may give you an idea #5426329 – Apophasis

Answer 1

If you're using the same view model with a [Required] property in two different contexts, one where the property is required and one where it isn't, then you'll need to manually alter the ModelState as you're doing.

An alternative is to use a different view model. Perhaps have a base class with all properties except the required property in question. Then derive two view models from it, one with the property where is it required and one with the property where it is not (it's duplication, I know). You may decide to keep them entirely separate altogether and not use inheritance.

Halifax

you could also manually validate that field as required in the controller and add a modelstate error. Remove the [required] annotation. Either that or identify how it's variably required and write a custom validator. – Consummation

Answer 2

Here's my solution - a RemoveFor() extension method on ModelState, modelled after MVC HTML helpers:

    public static void RemoveFor<TModel>(this ModelStateDictionary modelState, 
                                         Expression<Func<TModel, object>> expression)
    {
        string expressionText = ExpressionHelper.GetExpressionText(expression);

        foreach (var ms in modelState.ToArray())
        {
            if (ms.Key.StartsWith(expressionText + ".") || ms.Key == expressionText)
            {
                modelState.Remove(ms);
            }
        }
    }

Here's how it's used :

if (model.CheckoutModel.ShipToBillingAddress == true) 
{
    // REUSE BILLING ADDRESS FOR SHIPPING ADDRESS
    ShoppingCart.ShippingAddress = ShoppingCart.BillingAddress;

    // REMOVE MODELSTATE ERRORS FOR SHIPPING ADDRESS
    ModelState.RemoveFor<SinglePageStoreModel>(x => model.CheckoutModel.ShippingAddress);
}

So in answer to your question I believe there are definitely use-cases where this is the right way to do it, and a strongly typed helper like this makes it much nicer to look at - and easier to justify if you're concerned about lots of magic strings.

Loop

Your "solution" is too complicated imo. – Hypoglossal

@Hypoglossal complicated in what respect? you don't like the idea of removing ModelState at all - or you don't have a problem with code like ModelState.Remove("CheckoutModel.ShippingAddress.City") and don't see a need for this. i find this a much safer way to do things and if i change the name of something I find out at compile time. I've always hated having to remove modelstate but sometimes you just have to and I'm much more confortable with this approach. curious what your more detailed opinion is – Loop

In my opinion you are solving the problem on the wrong place. Making the solution harder than it should be. Use a different input model (view model) as Steve Morgan already mentioned. – Hypoglossal

@Hypoglossal I'd love suggestions on how to better do this for specifically a checkout model. There are very often parts of a model that are optional (such as shipping address here) and I really don't see how else I can do this without an even more overly complicated modelbinder. The model I have is already designed specifically for this page. At the end of the day I want to say ModelState.IsValid and get a true or false so I need to remove errors that don't apply based on other conditions. I also want to be sure it will work without javascript. This method is working great for me so far. – Loop

I like this solution. I often decorate my classes and data with CreatedBy, CreatedDate, LastUpdatedBy and LastUpdatedDate which I don't want displayed or entered in the view. This saves me having to make dozens of viewmodels and handle the whole back and forth of switching object types. Thanks. – Ruhr

What if the property I want to remove the error for is located in collection item? LIke this: ModelState.RemoveFor<MyViewModel>(x => x.MyCollection[0].ItemProperty); That would only work on the first item. – Periostitis

@Loop On line modelState.Remove(ms); I'm getting the error: Argument 1: cannot convert from 'System.Collections.Generic.KeyValuePair<string, Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateEntry>' to 'string'. I'm using ASP.NET Core on VS2015. – Possibility

I like this solution to, I have to round trip a model being posted back to view and this makes my life easier. I found a bug I think, I was removing a field which was an array. the match for ending with "." fails so i added ms.Key.StartsWith(expressionText + "[") to the if condition. – Xerophyte

samehere @RobinLuiten – Deflected

Honestly, haven't tested this, but to me it'd make sense to apply this logic one step earlier. Rather than marking ShippingAddress as Required, wouldn't it be more logical to create an ValidationAttribute called ConditionallyRequired that takes in an expression? Then you'd just need to call Model.IsValid & the model itself deals with the logic. E.g. #3414215 – Yorkist

Answer 3

Fundamentally, your issue is that while your classes are decorated with [Required], it's not always true. If you're operating in a context where it isn't true, you should really be using a class that doesn't define the property as being [Required].

You should really use a ViewModel that is correctly defined for its specific usage and that may mean duplicating some classes. A ViewModel is associated with the implementation of the UI and while it may use classes from your domain model, it's not always the right thing to do.

Failing that, the options are to either not use ModelState.IsValid, or to continue to use ModelState.Remove.

But logically, it makes sense for your ViewModel to be 'validatable', not to have to ignore certain validation errors.

Entree

A separate view model isn't required, just don't use attribute based validation for this. Use something that can accurately express this logic and conditionally add an error to the model state. – Menstruation

A separate view model IS required since it allows the two models to change independent of each other when the views change without having to worry about changing two places. – Disaster

Thought I replied to this years ago - but what I was really referring to is something like an AddressModel where Zip may not be required if countryCd is Ireland. I am still using this kind of method today where I remove zip errors from modelstate if the country doesn't require it. That's an example where a separate view model isn't possible. – Loop

Answer 4

If your property is not always required you should not decorate it with [Required].

A good alternative to do your validation is to implement the interface IValidatableObject.

For example, let's say that you want to make the field State required only when the country is United States. You could do it that way :

public class AddressModel : IValidatableObject
{
    [Required]
    public string Country { get; set; }
    public string State { get; set; }

    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
    {
        if(Country == "United States" && String.IsNullOrEmpty(State))
        {
            yield return new ValidationResult("State is required for United States", new [] { nameof(State) });
        }
    }
}

Note : This kind of validation only works on the server side.

Other alternatives?

As mentioned in other answers, it's sometimes a good idea to create 2 or more models if the views and validations are very different.

Chlori

Answer 5

I'm totally with Mr.Steve Morgan

So if your ViewModel doesn't always need some property to be Required then you shouldn't decorate it as Required.

I don't know why you wanna this issue but I suppose that in some cases you need PropertyOne to be Required if PropertyTwo has value. In this case you may need to make your CustomValidationAttribute to check these two properties.

I'm using something like this :

[AttributeUsage(AttributeTargets.Class, AllowMultiple = true, Inherited = true)]
public class PropertyNeededAttribute : ValidationAttribute
{
    private const string defaultErrorMessage = "'{0}' needs '{1}' to be valid.";

    public PropertyNeededAttribute(string originalProperty, string neededProperty)
        : base(defaultErrorMessage)
    {
        NeededProperty = neededProperty;
        OriginalProperty = originalProperty;
    }

    public string NeededProperty { get; private set; }
    public string OriginalProperty { get; private set; }

    public override object TypeId
    {
        get { return new object(); }
    }

    public override string FormatErrorMessage(string name)
    {
        return String.Format(CultureInfo.CurrentUICulture, ErrorMessageString,
                             OriginalProperty, NeededProperty);
    }

    public override bool IsValid(object value)
    {
        object neededValue = Statics.GetPropertyValue(value, NeededProperty);
        object originalValue = Statics.GetPropertyValue(value, OriginalProperty);
        if (originalValue != null && neededValue == null)
            return false;
        return true;
    }
}

note: Statics.GetPropertyValue(...) do nothing but get the value from the property to compare it.

Hope this helped :)

Conscience