31

I can't understand, how to use TryUpdateModel and save the MVC architecture at the same time.

If I am not mistaken, work with datacontexts must be in the Model. So, such code

var db=new TestEverybody();//it is class, which was generated by EntityFramework 
var currentTesting=db.Testing.(t => t.id == id).First();

must be situated in the Model, not in the Controller, mustn't it?

But the ussual examples of TryUpdateModel usage is following:

    public ActionResult Edit(Testing obj)//Testing collection
    {
        var db = new TestEverybody();
        var currentTesting=db.Testing.(t => t.id == obj.id).First();
        TryUpdateModel(currentTesting);
        db.SaveChanges();            
        return RedirectToAction("Index");
    }

Doesn't this way break the MVC architecture? We work with database in the controller, not in the special Model class.

So, what is the best way to use TryUpdateModel in a real project?

Sir Hally
  • 2,318
  • 3
  • 31
  • 48
  • 1
    My advice, in a real project, don't use it. You should use view specific models and map between the properties of your view model and those on your entities that you want to update. TryUpdateModel/UpdateModel is greedy and will bite you...eventually. – Ben Foster Oct 13 '11 at 00:58
  • Is it MVVM pattern? Where can I read about your method? – Sir Hally Oct 15 '11 at 06:07
  • 2
    No this is MVC (done right). I've added an answer below to explain. Hope this helps. – Ben Foster Oct 15 '11 at 09:46

2 Answers2

115

Since the OP asked, here's an example of the ViewModel pattern, or as I like to call it - ASP.NET MVC done properly.

So why use a view specific model

  1. You should only pass the information to your view that it needs.
  2. Often you'll need to add additional view-meta-data (such as title/description attributes). These do not belong on your entities.
  3. Using TryUpdateModel/UpdateModel is wrong. Don't use (I'll explain why).
  4. It's very rare that your view-models will exactly match your entities. People often end up adding additional cruft to their entities or (not much better) just using ViewBag rather than strongly typed view model properties.
  5. If you're using an ORM you can run into issues with Lazy loaded properties (N+1). Your views should not issue queries.

We'll start with a simple entity:

public class Product {
    public int Id {get;set;}
    public string Name {get;set;}
    public string Description {get;set;}
    public decimal Price {get;set;}
}

And let's say you have a simple form where the user can only update the Name and Description of the product. But you're using (the very greedy) TryUpdateModel.

So I use any number of tools (like Fiddler) to construct a POST myself and send the following:

Name=WhatverIWant&Description=UnluckyFool&Price=0

Well the ASP.NET MVC model binder is going to inspect the input form collection, see that these properties exist on your entity and automatically bind them for you. So when you call "TryUpdateModel" on the entity you've just retrieved from your database, all of the matching properties will be updated (including the Price!). Time for a new option.

View Specific Model

public class EditProductViewModel {
    [HiddenInput]
    public Guid Id {get;set;}

    [Required]
    [DisplayName("Product Name")]
    public string Name {get;set;}

    [AllowHtml]
    [DataType(DataType.MultilineText)]
    public string Description {get;set;}
}

This contains just the properties we need in our view. Notice we've also added some validation attributes, display attributes and some mvc specific attributes.

By not being restricted in what we have in our view model it can make your views much cleaner. For example, we could render out our entire edit form by having the following in our view:

@Html.EditorFor(model => model)

Mvc will inspect all of those attributes we've added to our view model and automatically wire up validation, labels and the correct input fields (i.e. a textarea for description).

POSTing the form

[HttpPost]
public ActionResult EditProduct(EditProductViewModel model) {

    var product = repository.GetById(model.Id);

    if (product == null) {
        return HttpNotFound();
    }

    // input validation
    if (ModelState.IsValid) {

        // map the properties we **actually** want to update
        product.Name = model.Name;
        product.Description = model.Description;

        repository.Save(product);

        return RedirectToAction("index");
    }

    return View(model)
}

It's fairly obvious from this code what it does. We don't have any undesirable effects when we update our entity since we are explicitly setting properties on our entity.

I hope this explains the View-Model pattern enough for you to want to use it.

Ben Foster
  • 34,340
  • 40
  • 176
  • 285
  • Thank you. What is the best way for automatic property mapping? "model" has only Name and Description, so If I try to use ApplyCurrentValues, the price will be replaced by null, isn't it? – Sir Hally Oct 17 '11 at 19:49
  • The common question - how to get model object from viewmodel object and map the viewmodel's properties to model's one? I tried to avoid such mapping "product.Name = model.Name", because model can contains a lot of properties – Sir Hally Oct 17 '11 at 19:53
  • 4
    You can automatically map the properties of your entity to your viewmodel using something like AutoMapper. In my opinion (and most of the people on the AutoMapper group) you should not map the other way round (from your viewmodel back to your entity) as yet again, this can lead to unexpected results. – Ben Foster Oct 17 '11 at 21:13
  • But how can I organise update logic in such case? According to our example, I have viewModel with 3 properties (Id, Name, Description) and model with 4 ones (Id, name,Description, Price). So, I must change old data (Name and Description) in Model, because I can't write Update method for viewModel, because every model can have a lot of viewModels. So, how to avoid mapping from viewModel back to model? – Sir Hally Oct 18 '11 at 05:53
  • 2
    I don't understand the question. You can't avoid mapping from the viewModel to the model, nor should you. That was the point of the above, to show you how you should MAP between a view specific model and an entity. – Ben Foster Oct 20 '11 at 23:29
  • Yes, I've understand. Thank you! – Sir Hally Oct 21 '11 at 14:25
  • 5
    +1 this is an excellent answer and has helped me overcome some problems I was facing with my application. This always seemed like a lot of duplication but your answer shows why it is a good idea - it actually makes your application a lot easier to develop and much easier to maintain. – Benjamin Gale Aug 31 '12 at 10:58
  • What would your advice be for action methods that return a list of entities (E.G. products)? Would you recommend using your approach I.E. return IEnumerable or return IEnumerable? Just to be clear I'm talking about methods that respond to HTTP GET requests. – Benjamin Gale Aug 31 '12 at 11:38
  • You should return `IEnumerable`. – Ben Foster Aug 31 '12 at 11:46
  • If AutoMapper is fine to use `EF -> VM`, how can it not be fine, in reverse? Or rather, the same pitfalls of the latter exist on the former, right? So if you shouldn't convert from `VM -> EF` because extra properties may get set unwillingly, then surely the same issues exist in the reverse. Moreover, since you're in control of your VM properties, it's easy to name them differently if you *don't* want them AutoMapped. Worse, boilerplate code for copying properties shouldn't live in Controllers anyway, so now you'll have 1 strategy for copying `VM -> EF`, but another for `EF -> VM`. that's ugly – JoeBrockhaus Nov 10 '14 at 18:06
  • 1
    @BenFoster If you use TryUpdateModel with a list of strings to include/exclude, doesn't that remove the aggressive nature of it? Couldn't you also specify the Bind attribute in the ActionResult parameter to prevent over-posting? I'd rather do that than right assignment statements for each property to update as you did in the example. – Brad Sep 02 '16 at 14:23
19

So, such code must be situated in the Model, not in the Controller, mustn't it?

Not necessarily. Personally I prefer to put data access code in a repository. Then use constructor injection to pass some specific repository implementation to the controller (for example if I was using EF, I would write an EF repository implementation). So the controller will look like this:

public class HomeController: Controller
{
    private readonly IMyRepository _repository;
    public HomeController(IMyRepository repository)
    {
        _repository = repository;
    }

    public ActionResult Edit(int id)
    {
        var currentTesting = _repository.GetTesting(id);
        TryUpdateModel(currentTesting);
        _repository.SaveChanges();            
        return RedirectToAction("Index");
    }
}
Darin Dimitrov
  • 1,023,142
  • 271
  • 3,287
  • 2,928
  • 1
    It would probably be useful to show (or explain) how to actually inject the repository, since MVC instantiates the controller for you. This is generally just used for Testing with Mocks, so your Controller can create an instance of the `ActulRepository` in its default constructor. However, novice devs could look at this and be at a loss for how to get their actual Repo into `_repository` without instantiating it locally. – JoeBrockhaus Nov 10 '14 at 17:50
  • @JoeBrockhaus, I agree, as I am one of them =) – RoLYroLLs Mar 08 '18 at 00:06