Is it okay to hit the database from a custom model binder?

Question

Say I have an object that gets some data from HttpPost and some from the database. I think I want to allow the ModelBinder to go to the database/repository for the that data missing from the post. In practice, is this a good or bad idea?

Photolysis

Answer 1

I've decided to edit my original answer given my thinking on these types of things has evolved since early 2010.

In my original answer, I basically expressed that, while my instincts told me you shouldn't do this, I was uncomfortable saying we shouldn't without being able to articulate why.

Now, I'd recommend against this on the grounds that the responsibility of a Model Binder is to translate a user request into a Request Model and that retrieving data outside of what can be derived from the request goes beyond this scope of responsibility.

Hibben

This is not a good answer because it violates SRP for the Binder. The binder's purpose is to bind from the request. You could easily end up with a scenario where your business logic bleeds into the ModelBinder. Unit Testing becomes more difficult. – Mack

While I wasn't able to articulate why back in 2010, I certainly would agree today. – Hibben

Thanks for taking the time to update this, and I totally agree with you. – Basra

Answer 2

I would say a bad idea. The idea of the model binder is that it takes the parameters from the request and creates your model from those. If your model binder, behind the scenes, fills in some of the details from the database this breaks the paradigm. I'd much rather expose the database call in my controller by explicitly fetching the required extra data from the database directly. Note that this could be refactored into a method if used frequently.

Linstock

Answer 3

I think this is perfectly fine and use this technique all the time.

The only arguments against are very pedantic and amount to arguing over philosophy. IMHO you can put "fill in missing posted data" code into you MVC app as a method in your base controller vs. method in you ActionFilter vs method in you ModelBinder. It all depends on who get what responsibility. To me the model binder can do a lot more than simply wire up some properties from posted values.

The reason I love doing database calls in my modelbinder is because it helps clean up your action methods.

    //logic not in modelbinder
    public ActionResult Edit( KittyCat cat )
    {
        DoSomeOrthagonalDatabaseCall( cat );

        return View( new MODEL() );
    }

vs.

    //logic in model binder
    public ActionResult Add( KittyCat cat )
    {
        return View( new MODEL() );
    }

Dav

Except that when someone else comes along who is familiar with MVC, he now gets a nice "happy" WTF moment when he finds that the model is fully populated with data even though it wasn't posted from the database and there is no indication of where it might be coming from. Then he gets to dig through a lot of unrelated code in a completely different section to find that "aha" -- there's a custom model binder here that performs magic, filling in the missing properties, including the one that unintentionally got left off the form. Sorry, but I'd rather make it obvious for the poor sap. – Linstock

The "new guy" argument is a pretty poor one. Anybody familiar with asp.net mvc will know to check the model binder if an action method parameter "mysteriously" has some values set that weren't posted with the form. Your criticism is poor considering simply asking another dev. "hey, where are these values coming from?" solves the entire issue. These "new guy" arguments seem to carefully construct a world were experience or sharing of information is non-existent. – Dav

Answer 4

It violates the way MVC is supposed to work. ModelBinder is for binging Models from the data that comes from the view. Populating missing info from the database is something that is supposed to be handled by the controller. Ideally, it would have same data layer/repository class that it uses to do this.

The reason it should be in the controller is because this code is business logic. The business rules dictate that some data may be missing and thus it must be handled by the brains of the operation, the controller.

Take it a step further, say you want to log in the DB what info the user isn't posting, or catch an exception when getting the missing data and email admins about it. You have to put these in your model binder this way and it gets more and more ugly with the ModelBinder becoming more and more warped from its original purpose.

Basically you want everything but the controller to be as dumb and as specialized as possible, only knowing out how to carry out its specific area of expertise which is purely to assist the controller.

Seller

Model binder arguments aside, business logic belongs in the domain layer of your application, not in your controllers. You don't want everything but your controllers to be as dumb as possible, in fact you want the exact opposite. Controllers are merely the entry point to the application which translates a user's request into actions performed on domain model objects. I would recommend you go explore Domain-Driven Design concepts and then bring what you've learned there back to MVC. I think you'll see things quite differently. – Hibben

What do you mean by business logic? Maybe I am confused in my understanding of it. I'm not saying that the controller is performing low level operations and is thus smart. I am saying the controller decides what to do, but doesn't know how it is done. This deciding of what to do is what I consider business logic. It is dumb in the sense that without the model it is helpless. Do you have a good article on domain driven design? – Seller

Answer 5

I would say, no.

Here's why: It would create a dependency on your database for testing your controller actions that would not be easy to abstract out.

Ioannina

How so, usually I give the action a fully populated model as a parameter, skipping the model binder entirely in my unit tests? Why should the code under test care how the model is built? – Linstock

If your model is being bound from the HttpPost and some from the database and its either going to be bound in the controller/repository/model binder, then wouldnt using the model binder be a clean way of avoiding duplicate functionality that might be required on add AND edit operations? – Springhead

Regarding: " It would create a dependency on your database for testing your controller actions that would not be easy to abstract out." This is false. You can easily have access to anything inside of your controller inside of your modelbinder, there is no additional dependency your creating. Therefore if your injecting a DAL/Repository/Service into your controller you also have can get it in your Modelbinder. – Dav

The thing that did it for me was testing. That's enough reason to stay away from mixing modelbinders with db access. – Photolysis

@Byron If you had to do some database access to populate a parameter to an action method for a unit test then something has gone awry in the universe :) All you should have needed to do is call _controller.SomeAction(new Entity { Param1 = 1, Param2 = 2, Param 3 = 3});. That is, if your action actually needed those values stubbed out for the particular behavior you were testing. This answer provides no real justification, so it shouldn't have been marked as the answer. Doing so sounds more like it was just the first answer that agreed with a predisposed idea you had. – Hibben

Answer 6

I would say it is ok. The argument that creates dependency to database is a false argument for 2 reasons:

1- Database access should be abstracted via repository interfaces. Repository interfaces are part of the domain model and their implementation is part of the infrastructure/data access layer. So there is no dependency to the database.

2- Model Binders and Controllers are both part of presentation layer implemented using ASP.NET MVC Framework. If Controllers are allowed to access database using repository interfaces, how come Model Binders are not allowed?

Also, there are situations that you'd "better" fill the missing data in your model from Model Binders. Consider the scenario where you have a drop-down list on your view. The first time the view is loaded, the drop-down list is populated. The user submits the form but the validation fails. So you'll need to return the form again. At this stage, you'll have to re-populate the list in the Model for the drop-down list. Doing this in Controller looks ugly:

public ActionResult Save(DocumentViewModel viewModel)
{
     if (!ModelState.IsValid)
     {
         viewModel.Categories = _repository.GetAll();
         return View(viewModel);
     }
}

I believe the initialization of Categories here is ugly and like a code smell. What if you had a few properties that needed to be filled out from database? What if you had more than 1 action that had DocumentViewModel as an argument? You'd have to repeat this ugly step over and over. A better approach is to fill all the properties of the model using the Model Binder and pass it to the Controller. So the object that is passed to the controller is in a "consistent" state.

Progenitor