SitePoint Sponsor

User Tag List

Results 1 to 20 of 20
  1. #1
    Chopped Liver bronze trophy imaginekitty's Avatar
    Join Date
    Aug 2007
    Location
    Pennsyltucky
    Posts
    1,494
    Mentioned
    3 Post(s)
    Tagged
    0 Thread(s)

    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.
    Code Csharp:
    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:
    Code Csharp:
    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?

  2. #2
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    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.

    Code Csharp:
    [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....

    Code Csharp:
    [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);
     
    }

  3. #3
    Chopped Liver bronze trophy imaginekitty's Avatar
    Join Date
    Aug 2007
    Location
    Pennsyltucky
    Posts
    1,494
    Mentioned
    3 Post(s)
    Tagged
    0 Thread(s)
    Thanks, Drew. That's good information.

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

  4. #4
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    I couldn't say without knowing the exact source.

  5. #5
    Chopped Liver bronze trophy imaginekitty's Avatar
    Join Date
    Aug 2007
    Location
    Pennsyltucky
    Posts
    1,494
    Mentioned
    3 Post(s)
    Tagged
    0 Thread(s)
    http://amazon.com/Pro-ASP-NET-MVC-3-...3705691&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.

  6. #6
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    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. =)

  7. #7
    Chopped Liver bronze trophy imaginekitty's Avatar
    Join Date
    Aug 2007
    Location
    Pennsyltucky
    Posts
    1,494
    Mentioned
    3 Post(s)
    Tagged
    0 Thread(s)
    Yeah, it's a repository. I would change a lot if I weren't just going through to learn Razor and EF.

  8. #8
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    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:
    Code Csharp:
    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:

    Code Csharp:
    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:
    Code Csharp:
    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.

    2) 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:
    Code Csharp:
    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:
    Code Csharp:
    public static class UnitOfWorkFactory
    {
        public static IUnitOfWOrk Create()
        {
            DbContext context = DependencyResolver.Current.GetService<DbContext>();
            return new UnitOfWork(context);
        }
    }

    Usage would look like this:
    Code Csharp:
    [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.

  9. #9
    Chopped Liver bronze trophy imaginekitty's Avatar
    Join Date
    Aug 2007
    Location
    Pennsyltucky
    Posts
    1,494
    Mentioned
    3 Post(s)
    Tagged
    0 Thread(s)
    Thanks, man. 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.

  10. #10
    SitePoint Author silver trophybronze trophy
    wwb_99's Avatar
    Join Date
    May 2003
    Location
    Washington, DC
    Posts
    10,576
    Mentioned
    4 Post(s)
    Tagged
    0 Thread(s)
    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.

  11. #11
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    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.

  12. #12
    SitePoint Zealot
    Join Date
    May 2004
    Location
    Jersey
    Posts
    175
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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.
    Matt Daly

  13. #13
    Chopped Liver bronze trophy imaginekitty's Avatar
    Join Date
    Aug 2007
    Location
    Pennsyltucky
    Posts
    1,494
    Mentioned
    3 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by matt-daly View Post
    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.

  14. #14
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    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?

  15. #15
    SitePoint Zealot
    Join Date
    May 2004
    Location
    Jersey
    Posts
    175
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by imaginekitty View Post
    That really doesn't have anything to do with the topic at hand.
    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.
    Matt Daly

  16. #16
    SitePoint Zealot
    Join Date
    May 2004
    Location
    Jersey
    Posts
    175
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Serenarules View Post
    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?
    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).
    Matt Daly

  17. #17
    Chopped Liver bronze trophy imaginekitty's Avatar
    Join Date
    Aug 2007
    Location
    Pennsyltucky
    Posts
    1,494
    Mentioned
    3 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by matt-daly View Post
    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.
    Can it.

  18. #18
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    Mark, sorry for inadvertantly getting your thread hi-jacked, after this post, I'm gracefully backing out until you need me. =)

  19. #19
    Chopped Liver bronze trophy imaginekitty's Avatar
    Join Date
    Aug 2007
    Location
    Pennsyltucky
    Posts
    1,494
    Mentioned
    3 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Serenarules View Post
    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!!!

  20. #20
    Chopped Liver bronze trophy imaginekitty's Avatar
    Join Date
    Aug 2007
    Location
    Pennsyltucky
    Posts
    1,494
    Mentioned
    3 Post(s)
    Tagged
    0 Thread(s)
    Using Serenarules suggestion I've found that this works:
    Code csharp:
    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:


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •