MVC SaveChanges() doesn't seem to work with passed object

I’m going through a few tutorials to learn Razor and EF and it is stated that this is how to update a Product
Pass the product in and save the changes on the context.

public void SaveProduct(Product product)
{
	context.SaveChanges();

}

but it doesn’t seem to work. No changes to the database but it will pass a message back to the view that it was updated and give the name of the product if I try that, it just doesn’t get to the database.
I have to do it like this instead:

public void SaveProduct(Product product)
{
	Product old = (from p in context.Products
			where p.ProductID == product.ProductID
			select p).SingleOrDefault();
	old.Name = product.Name;
	old.Description = product.Description;
	old.Category = product.Category;
	old.Price = product.Price;
	context.SaveChanges();
}

I can’t explain why it doesn’t work as the tutorial says and I seem to remember having the same exact issue with the MVC1 version of the tutorial. Am I doing something wrong or does it just not work like the tutorial says?

This is a prime reason you don’t pass entities to views. At the end of the request that sent that Product to a view, your context was disposed, leaving the Product detached. The instance incoming to your save action isn’t attached either. Your call to save changes works, it just isn’t updating your product because it doesn’t know about it.

Your best bet is to convert the entity into a view model first, send it to the view, and then on post action, refetch it and update it.

[HttpGet]
public ActionResult Edit(int id)
{

    var product = _context.Products.Find(id);

    var model = ProductEditModel.Build(product);

    return View(model);

}

[HttpPost]
public ActionResult Edit(int id, ProductEditModel model)
{

    if (ModelState.IsValid)
    {
        var product = _context.Products.Find(id);
        model.Update(product);
        _context.SaveChanges();
        return RedirectToAction("Index");
    }

    return View(model);

}

However, if you insist on sending entities, you can take advantage of EF’s auto-attaching methods…

[HttpPost]
public ActionResult Edit(Product product)
{

    if (ModelState.IsValid)
    {
        // attach/reattach entity and mark it as modified
        _context.Entry(product).State = EntityState.Modified;
        _context.SaveChanges();
        return RedirectToAction("Index");
    }

    return View(model);

}

Thanks, Drew. :slight_smile: That’s good information.

This brings up a bigger question: why is this in the tutorials from reputable sources as if it should work?

I couldn’t say without knowing the exact source.

http://amazon.com/Pro-ASP-NET-MVC-3-Framework/dp/1430234040/ref=sr_1_1?ie=UTF8&qid=1323705691&sr=8-1 the book I mentioned on another part. It’s a good resource except for this one problem that seems to be in every edition.

Ah, well, it could just be a publication oversight. Also, I noticed the method you posted didn’t seem to be an mvc action, as it returned void. It may be part of a bigger solution, such as a repository implementation. In which case, they are still doing it wrong. Repositories should not expose work related methods. That’s the job of the unit-of-work. But that’s another issue altogether. Just remember that sometimes final editors of printed material may not be in tune with the material being printed and don’t always catch things the author might. Keep working through the book, and if you have problems, then come here. One of us will surely advise. =)

Yeah, it’s a repository. I would change a lot if I weren’t just going through to learn Razor and EF.

Well, since you seem to be learning about repositories, I’ll save you some time and headache.

The most important thing about repositories and units of work is that they are two very distint types of constructs, with very different goals in mind. A lot of the online tutorials I’ve seen suggest that you should either pass a unit of work into your repositories, or let the units of work act as repository factories. Both of these methods are fundamentally wrong in their implementation. A car and a train have two different roles to fulfill. You wouldn’t expect a car to produce the functionality of a train, or have a train produce a car upon request would you?

Let’s look at the industry standard unit of work interface as defined in the PoEAA book:


public interface IUnitOfWork : IDisposable
{
void RegisterNew(object entity);
void RegisterDirty(object entity);
void RegisterDeleted(object entity);
void Commit();
void Rollback();
}

The role of this object should be obvious. As it’s name implies, it perform the work, or heavy lifting, for the application. It’s only job is to batch-transactionalize insert, update and delete commands. This leaves relatively few things for the repository to be responsible for. So let’s take a look at what’s left:


public interface IRepository<TEntity>
    where TEntity : class
{
    IEnumerable<TEntity> GetAll();
    TEntity GetById(object id);
}

The repository and unit of work act as separate contracts. Neither should be injected into the other, nor should either act as a factory for the other. In this arrangement, the name ‘repository’ is a bit of a misnomer. The name suggests ‘the place you put things’, but in reality it is ‘the place you get things’. I’d call for a change in name, but it’s too widely used under this name.

So how would you go about implementing these? First, let’s do the repository, as it is the simplest:


public class Repository<TEntity> : IRepository<TEntity>
    where TEntity : class
{

    private readonly DbContext _context;

    public Repository(DbContext context)
    {
        _context = context;
    }

    public IEnumerable<TEntity> GetAll()
    {
        return _context.Set<TEntity>();
    }

    public TEntity GetById(object id)
    {
        return _context.Set<TEntity>().Find(id);
    }

}

Let’s address some questions now:

  1. Why did you declare TEntity as class when it should be a common entity base class?

You can, and should. At first, you may simply use a common “Entity” base class, but as you progress, you will start using distinct types for aggregate root and dependent entities, at which point, you will want to force your repository to act only upon aggregate roots. For now, an exact type isn’t required.

  1. Why did you declare the id parameter of GetById as object?

This is a good idea because, as you progress, you will find yourself using things other than a db-filled int. The Find method supports any type usable as an id, including int, string, Guid, and others. Having this method be open to any id type means less work on your part.

Ok, now let’s take a look at the unit of work implementation. This one is a little more involved, so take your time and review the sample code:


public class UnitOfWork : IUnitOfWork
{

    private readonly DbContext _context;
    private readonly IList<object> _registeredNew;
    private readonly IList<object> _registeredDirty;
    private readonly IList<object> _registeredDeleted;

    public UnitOfWork(DbContext context)
    {
        _context = context;
    }

    public void RegisterNew(object entity)
    {
        _registeredNew.Add(entity);
    }

    public void RegisterDirty(object entity)
    {
        _registeredDirty.Add(entity);
    }

    public void RegisterDeleted(object entity)
    {
        _registeredDeleted.Add(entity);
    }

    public void Commit()
    {
        foreach (var entity in _registeredNew)
            _context.Entry(entity).State = EntityState.Added;
        foreach (var entity in _registeredDirty)
            _context.Entry(entity).State = EntityState.Modified;
        foreach (var entity in _registeredDeleted)
            _context.Entry(entity).State = EntityState.Deleted;
        try
        {// if this fails, data is rollback due to internal transaction
            _context.SaveChanges();
        }
        catch (Exception exception)
        {// report the failure
            throw new Exception("Transaction failed!", exception);
        }
        finally
        {// good idea to clear our own cache after success or failure
            Rollback();
        }
    }

    public void Rollback()
    {
        _registeredNew.Clear();
        _registeredDirty.Clear();
        _registeredDeleted.Clear();
    }

    public void Dispose()
    {
        Rollback();
    }

}

And lastly, a simple factory class for the UoWs:


public static class UnitOfWorkFactory
{
    public static IUnitOfWOrk Create()
    {
        DbContext context = DependencyResolver.Current.GetService<DbContext>();
        return new UnitOfWork(context);
    }
}

Usage would look like this:


[HttpPost, ValidateAntiForgeryToken]
public ActionResult Edit(int id, FooModel model)
{
    if (ModelState.IsValid)
    {
        using (var uow = UnitOfWorkFactory.Create())
        {
            var foo = _fooRepository.GetById(id);
            // update foo values (or call a business method if using DDD)
            uow.RegisterDirty(foo);
            uow.Commit();
        }
        return RedirectToRoute("some_route_name");
    }
    return View(model);
}

The repository is injected into the controller, as it will be used in almost every single action. The unit of work will only be used when work is needed, and is therefore instantiated through the factory class.

I hope that heads you in the right direction, without having to read through an entire volume.

Thanks, man. :slight_smile: I’m actually just learning about the new Razor view engine.

I’m looking over your examples but I have a conceptual error where I can’t understand examples unless they have a real-world application. I just can’t grok foo and bar for some reason.

   catch (Exception exception)
    {// report the failure
        throw new Exception("Transaction failed!", exception);
    }

Any particular reason to do this, you are just truncating the call stack. I would probably just skip the catch there myself and go straight into finally for cleanup.

Not really. I don’t use EF personally. I go back and forth between an NHibernate solution and CQRS with Event Sourcing for bigger things (which means raw SqlClient in Event persistors). My NHibernate uow looks like the following, since we have access to an actual transaction object with _session.BeginTransaction(IsolationLevel.ReadCommitted), so I just put something in there to fill the gap:

try { transaction.Commit(); } catch { _transaction.Rollback(); } finally { Rollback(); }

That’s all. He can do what he likes with the catch.

Just use Raven and forget about repositories, units of work and mappings for good. You actually get to spend your time building the application instead of the data access layers. Not used it (Raven) for a month or so (currently developing with node and Redis primarily - great fun to work with) but other than a few looks in the management studio you won’t have to bother with data access rubbish again.

That really doesn’t have anything to do with the topic at hand.

I’ve looked at it. Though most of my high $ clients insist on MS only products. That narrows my margin of choices a bit. When using CQRS, I don’t use an orm at all either.

Since you seem to have spent more time with it though, here’s a question for you. How does it handle private fields with no public getters?

It has plenty to do with the topic at hand, Serenarules made a post containing the unit of work and repository and I was suggesting an alternative. If you don’t like it don’t reply.

I’ve never been in a situation where I’ve needed private fields, but you should be able to assign them with the [JsonProperty] attribute and they’ll get stored. You could use private setters on properties so that they can’t be changed externally. Raven is a good solution for CQRS based solutions if you’re event sourcing, there are a few examples lying about using it (although if you’ve spent the time to learn CQRS you’ve probably come across them).

Can it.

Mark, sorry for inadvertantly getting your thread hi-jacked, after this post, I’m gracefully backing out until you need me. =)

No worries. There was no non sequitur from you. I’m not upset or anything. I just failed to see any relevance.

Alligators are great pets!!!

Using Serenarules suggestion I’ve found that this works:

public void SaveProduct(Product product)
{
	if (product.ProductID == 0)
	{
		context.Products.Add(product);
	}
	else
	{
		context.Entry(product).State = EntityState.Modified;
	}
	context.SaveChanges();
}

I’m still wondering if I’m the only one that needed to make this change though. :shrug: