What's Wrong With This Code? (#29)

Here is some code similar to other code I've seen that has a severe bug. The Entity Framework model configuration is setup to allow optimistic concurrency checks via a movie's Version property.

public class Movie
{
    public virtual int ID { get; set; }
    public virtual string Title { get; set; }
    public byte[] Version { get; set; }
}

public class MovieDB : DbContext
{
    public DbSet<Movie> Movies { get; set; }

    protected override void OnModelCreating(
        DbModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Movie>()
                    .Property(p => p.Version)
                    .IsRowVersion();
        base.OnModelCreating(modelBuilder);
    }
}

During editing, the view correctly holds the row version in a hidden input.

@Html.HiddenFor(model => model.Version)

But unfortunately, the controller action accepting the edit request has a flaw. It's trying to retrieve the existing movie from the database, overwrite the entity with new values (and the previously retrieved version value), then save the entity. The idea is that if the underlying record changed, we'll see an optimistic concurrency exception because we gave the entity an old row version value.

[HttpPost]
public ActionResult Edit(int id, Movie editedMovie)
{
    if (ModelState.IsValid)
    {
        var movie = db.Movies.Find(id);
        movie.Title = editedMovie.Title;
        movie.Version = editedMovie.Version;       
        db.SaveChanges();
        return RedirectToAction("Index");
    }
    return View(editedMovie);
}

The update will almost never fail with an OptimisticConcurrencyException. The update will almost always overwrite the current record in the database*.

What's wrong?

Hint: the Entity Framework doesn't behave like the implementation expects.

Answer

A property marked as a RowVersion property implicitly holds a  "store-generated" value. The Entity Framework never expects an application to modify the value. When you retrieve an entity and set a computed property to a new value, you'll still see the original value sent to the database when you call SaveChanges. The Entity Framework will always use the original value loaded from the database. The correct way to save the new values is to "attach" the movie instead of using a query to retrieve values from the database.

[HttpPost]
public ActionResult Edit(Movie editedMovie)
{
    if (ModelState.IsValid)
    {
        db.Entry(editedMovie).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index");
    }
    return View(editedMovie);
}

* I say "almost" because you could have a concurrency exception if the data changes between the query and the call to SaveChanges (where time is measured in milliseconds). It doesn't fail if the data changes between the Edit GET request and the POST request (where time might be measured in minutes).