Is it OK to use repository in view model?

Question

Let's say I have complex view model with a lot of data such as lists of countries, products, categories etc. for which I need to fetch from the database every time I create the ViewModel.

The main problem I want to fix is that when I handle POST actions and some TestModel was posted with incorrect values, which causes ModelState.IsValid to be false, then I have to return the same view with currently posted model. This forces me to get my list of categories again, since I was doing that in the GET action. This adds a lot of duplicated code in controller and I want to remove it. Currently I am doing the following:

My model and view models:

Model, entity stored in the database:

public class Category
{
    public int Id { get; set; }
    public string Name { get; set; }

    public IEnumerable<Category> SubCategories { get; set; }
}

View models:

public class CategoryModel
{
    public int Id { get; set; }
    public string Name { get; set; }
}

public class TestModel
{
    [Required]
    [MaxLength(5)]
    public string Text { get; set; }

    public int SelectedCategory { get; set; }
    public IEnumerable<CategoryModel> Categories { get; set; }
    public SelectList CategoriesList
    {
        get
        {
            var items = Categories == null || !Categories.Any() 
                ? Enumerable.Empty<SelectListItem>()
                : Categories.Select(c => new SelectListItem
                {
                    Value = c.Id.ToString(),
                    Text = c.Name
                });

            return new SelectList(items, "Value", "Text");
        }
    }
}

My controller:

public class HomeController : Controller
{
    private readonly Repository _repository = ObjectFactory.GetRepositoryInstance();

    public ActionResult Index()
    {
        var model = new TestModel
        {
            Categories = _repository.Categories.Select(c => new CategoryModel
            {
                Id = c.Id,
                Name = c.Name
            })
        };
        return View(model);
    }

    [HttpPost]
    public ActionResult Index(TestModel model)
    {
        if (ModelState.IsValid)
        {
            return RedirectToAction("Succes");
        }

        model.Categories = _repository.Categories.Select(c => new CategoryModel
        {
            Id = c.Id,
            Name = c.Name
        });
        return View(model);
    }

    public ActionResult Succes()
    {
        return View();
    }
}

I want to remove duplicated Categories fetching and mapping, basically this code:

.Categories = _repository.Categories.Select(c => new CategoryModel
            {
                Id = c.Id,
                Name = c.Name
            })

from controller. Also I want to remove ModelState validity check, I want to execute the action only if ModelState.IsValid to keep controller code AS CLEAN AS POSSIBLE. So far I have the following solution for removing ModelState validity check:

Create custom ValidateModelAttribute

public class ValidateModelAttribute : ActionFilterAttribute
{
    public override void OnActionExecuting(ActionExecutingContext filterContext)
    {
        var viewData = filterContext.Controller.ViewData;

        if(viewData.ModelState.IsValid) return;

        viewData.Model = filterContext.ActionParameters["model"];
        filterContext.Result = new ViewResult
        {
            ViewData = viewData,
        };
     }
 }

Now model is validated before the action executes. In case of validation errors, we use same view with the same recently posted model. Therefore, the controller POST action looks like this:

[HttpPost]
[ValidateModelAttribute]
public ActionResult Index(TestModel model)
{
    // Do some important stuff with posted data
    return RedirectToAction("Success");
}

This is nice, but now my Categories property of my TestModel is empty, because I have to fetch the categories from the database, and map them accordingly. So is it OK to modify my view model to look something like this:

public class TestModel
{
    private readonly Repository _repository = ObjectFactory.GetRepositoryInstance();

    ...

    public int SelectedCategory { get; set; }
    public IEnumerable<CategoryModel> Categories {
        get
        {
            return _repository.Categories.Select(c => new CategoryModel
            {
                Id = c.Id,
                Name = c.Name
            });
        }
    }

    ...
}

This will allow us to have very clean controller, but wouldn't it cause some kind of performance or architectural issues? Wouldn't it break the single responsibility principle for view models? Should ViewModels be responsible for fetching data it needs?

Implicative

Ideally no, your view models would not interact directly with your repository. If you need to populate your model from the repository, this would happen in your controller. If you don't want to duplicate categories population in separate controller routes, you could try refactoring this logic out into a separate method. – Profusion

Good to know I wasnt the only one doing it wrong... – Gideon

Answer 1

It's not ok. the view model should be mainly a DTO populated by a service/query or even the controller. There was no problem with the previous version, your controller is just a couple of lines of code.

But your repository is not really a repository, it's a an ORM. A proper repository (well here it would be just some query object) would return directly the list of Categories for the view model.

About your auto validation attribute, don't reinvent the wheel, someone else (in this case me) did it before .

Lexicologist

Answer 2

No, you shouldn't put repository reference and logic into the view models. I suppose the only thing you need is to be able to rebuild the model if the validation fails. You can try one of the automated ModelState validation, for example:

http://benfoster.io/blog/automatic-modelstate-validation-in-aspnet-mvc

Ransack

Answer 3

There are a few flows I can see with your approach,

1. Using the repository and doing actual querying in the controller,

   var model = new TestModel
   {
       Categories = _repository.Categories.Select(c => new CategoryModel
       {
           Id = c.Id,
           Name = c.Name
       })
   };
   

Better approach is either to move this to repository or even better put it into a more logical level such as services.

With your new solution it's even worse as you refer the repository inside the view model. Ideally I'd do it like this,

public class TestService : ITestService{
   private IReposotory repo;

   public TestService(IReposotory repo){
     this.repo = repo;
   }

   public TestModel GetModel()
   { 
      return new TestModel()
{
    Categories = _repository.Categories.Select(c => new CategoryModel
    {
        Id = c.Id,
        Name = c.Name
    })
};       
   }
}

public class HomeController : Controller
{
    private readonly ITestService _service;

    public HomeController (ITestService service){
       _service = service;
   }

    public ActionResult Index()
    {        
        return View(_service.GetModel());
    }

    [HttpPost]
    public ActionResult Index(TestModel model)
    {
        if (ModelState.IsValid)
        {
            return RedirectToAction("Succes");
        }
        return View(model);
    }

    public ActionResult Succes()
    {
        return View();
    }
}

Io