Why is my model state not valid?

I have the following model:

public class Team
{
    public int Id { get; set; }

    [Required]
    [MaxLength(100)]
    public string Name { get; set; }

    [Required]
    public string Owner { get; set; }
    public int TransfersRemaining { get; set; }
    public DateTime DateCreated { get; set; }

    public IEnumerable<Player> StartingXI { get; set; }
    public IEnumerable<Player> Subs { get; set; }

    public Team() {
        TransfersRemaining = 5;
        StartingXI = Enumerable.Empty<Player>();
        Subs = Enumerable.Empty<Player>();
        DateCreated = DateTime.Now;
    }

    public Team(String teamName, String userId) 
    {
        Name = teamName;
        TransfersRemaining = 5;
        Owner = userId;
        StartingXI = Enumerable.Empty<Player>();
        Subs = Enumerable.Empty<Player>();
        DateCreated = DateTime.Now;
    }

    public int getTransfersRemaining() {
        return TransfersRemaining;
    }

    public string getTeamName()
    {
        return Name;
    }
}

I have the following ActionResult:

[HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult Create([Bind(Include = "Name, Owner")] Team team)
    {
        team.Owner = "bob";
        if (ModelState.IsValid)
        {
            teamRepository.Insert(team);
            teamRepository.Save();
            return RedirectToAction("Index");
        }



        foreach (ModelState modelState in ViewData.ModelState.Values)
        {
            foreach (ModelError error in modelState.Errors)
            {
                Response.Write(error);
            }
        }

        return View(team);
    }

On debugging, I see that the error is "The Owner field is required."

Surely I have provided that with team.Owner Examining my team object I can see that Owner = "bob" and all the other attributes are correctly set.

Any ideas what the problem is?

Solution

ModelState.IsValid is based on the contents of ModelState, not your entity instance. As a result, setting team.Owner for the first time in your action does nothing to negate the fact that it wasn't posted and therefore doesn't exist in ModelState.

However, you've got a number of crucial problems you need to also solve here. First, through the combination of using Bind and saving the instance of your entity created by the modelbinder directly, you're going to blow out any data that was not posted. Both TransfersRemaining and CreatedDate will be set to their respective default values. Likewise the two enumerables, StartingXI and Subs will be emptied, which will result in these relationship being removed in your database. Further, since Id was not posted, it will be the default for an int, 0, which will cause Entity Framework to create a new record, rather than updating an existing one.

None of these issues are necessarily a problem for the initial creation, but as soon as you apply this to your edit, you're going to screw all your data up. You should flat out never use Bind ever. It's horrible. Like seriously, just don't. It's Microsoft's boneheaded attempt at solving the over-post problem directly working with an entity can lead to, but it causes more problems than it solves. Additionally, there's a much better and simpler solution available: don't use the the entity. Instead, create a view model that only includes the properties you want a user to be able to edit, and then map those posted values onto your entity instance. That way, there is no possible way ever for a user to manipulate the post data because you control explicitly what does and does not get saved from the post. (For additional explanation about why Bind is so awful see: https://cpratt.co/bind-is-evil/.)

Next up, I'm assuming you want an actual relationship to Player with your enumerables here: as in, you'd like that relationship persisted in the database. That can't happen with IEnumerable. You need to use ICollection instead:

public ICollection<Player> StartingXI { get; set; }
public ICollection<Player> Subs { get; set; }

Also, it's not required and some may argue not a good idea anyways, but these properties will need the virtual keyword if you intend to employ lazy-loading. It's a perfectly valid and maybe even recommended choice to not allow lazy-loading, but you must be aware that these collections will be null unless you eagerly or explicitly load them when you need them. This often trips people up, so it's good to pay attention to what you're doing here and understand the pros and cons of that.

Finally, some notes on your class design:

You've got duplicate logic in your two constructors. It's much better to handle via overloading:

public Team()
    : this(null, null)
{
}

public Team(String teamName, String userId) 
{
    Name = teamName;
    TransfersRemaining = 5;
    Owner = userId;
    StartingXI = Enumerable.Empty<Player>();
    Subs = Enumerable.Empty<Player>();
    DateCreated = DateTime.Now;
}

However, the constructor is not the place to set defaults. Importantly, this will run whenever this class is instantiate, which includes when Entity Framework creates instances as a result of a query. For example, if you were to pull a bunch of existing teams from the database, all of these properties would be reset to these default values, rather than what exists for the record in the database. If you want defaults, you need to use a custom getter and setter or C# 6's initializer syntax:

C# 6+
```
public DateTime DateCreated { get; set; } = DateTime.Now;
```
Previous C#
```
private DateTime? dateCreated;
public DateTime DateCreated
{
    get { return dateCreated ?? DateTime.Now }
    set { dateCreated = value; }
}
```
Lists and other complex types are a bit more difficult, since you only want to instantiate them if they are null. The C# 6 initializer syntax is the same here, but for previous versions of C#, you should use:
```
private IEnumerable<Player> subs;
public IEnumerable<Player> Subs
{
    get
    {
        if (subs == null)
        {
            subs = new List<Player>();
        }
        return subs;
    }
    set { subs = value; }
}
```
While it's minor, having methods like getTransfersRemaining and getTeamName that merely return a property value are redundant and only add entropy to your code. If you're following TDD, these are now things you have to maintain tests for, whereas the property itself requires no tests. The properties, themselves, are part of your entity's public API; you don't need anything else. If you're doing this to satisfy an interface, that's actually a violation of the I in SOLID, the interface segregation principle:

A client should never be forced to implement an interface that it doesn't use or clients shouldn't be forced to depend on methods they do not use.