Accessing Database Entities from Controller [closed]

Question

tl;dr

In a good design. Should accessing the database be handled in a separate business logic layer (in an asp.net MVC model), or is it OK to pass IQueryables or DbContext objects to a controller?

Why? What are the pros and cons of each?

---

I'm building an ASP.NET MVC application in C#. It uses EntityFramework as an ORM.

Let's simplify this scenario a bit.

I have a database table with cute fluffy kittens. Each kitten has a kitten image link, kitten fluffiness index, kitten name and kitten id. These map to an EF generated POCO called Kitten. I might use this class in other projects and not just the asp.net MVC project.

I have a KittenController which should fetch the latest fluffy kittens at /Kittens. It may contain some logic selecting the kitten, but not too much logic. I've been arguing with a friend about how to implement this, I won't disclose sides :)

Option 1: db in the controller:

public ActionResult Kittens() // some parameters might be here
{
   using(var db = new KittenEntities()){ // db can also be injected,
       var result = db.Kittens // this explicit query is here
                      .Where(kitten=>kitten.fluffiness > 10) 
                      .Select(kitten=>new {
                            Name=kitten.name,
                            Url=kitten.imageUrl
                      }).Take(10); 
       return Json(result,JsonRequestBehavior.AllowGet);
   }
}

Option 2: Separate model

public class Kitten{
   public string Name {get; set; }
   public string Url {get; set; }
   private Kitten(){
        _fluffiness = fluffinessIndex;
   }

   public static IEnumerable<Kitten> GetLatestKittens(int fluffinessIndex=10){ 
        using(var db = new KittenEntities()){ //connection can also be injected
            return db.Kittens.Where(kitten=>kitten.fluffiness > 10)
                     .Select(entity=>new Kitten(entity.name,entity.imageUrl))
                     .Take(10).ToList();
        }
    } // it's static for simplicity here, in fact it's probably also an object method
      // Also, in practice it might be a service in a services directory creating the
      // Objects and fetching them from the DB, and just the kitten MVC _type_ here

}

//----Then the controller:
public ActionResult Kittens() // some parameters might be here
{
    return Json(Kittens.GetLatestKittens(10),JsonRequestBehavior.AllowGet);
}

Notes: GetLatestKittens is unlikely to be used elsewhere in the code but it might. It's possible to use the constructor of Kitten instead of a static building method and changing the class for Kittens. Basically it's supposed to be a layer above the database entities so the controller does not have to be aware of the actual database, the mapper, or entity framework.

• What are some pros and cons for each design?
• Is there a clear winner? Why?

Note: Of course, alternative approaches are very valued as answers too.

Clarification 1: This is not a trivial application in practice. This is an application with tens of controllers and thousands of lines of code, and the entities are not only used here but in tens of other C# projects. The example here is a reduced test case.

Answer 1

The second approach is superior. Let's try a lame analogy:

You enter a pizza shop and walk over to the counter. "Welcome to McPizza Maestro Double Deluxe, may I take your order?" the pimpled cashier asks you, the void in his eyes threatening to lure you in. "Yeah I'll have one large pizza with olives". "Okay", the cashier replies and his voice croaks in the middle of the "o" sound. He yells towards the kitchen "One Jimmy Carter!"

And then, after waiting for a bit, you get a large pizza with olives. Did you notice anything peculiar? The cashier didn't say "Take some dough, spin it round like it's Christmas time, pour some cheese and tomato sauce, sprinkle olives and put in an oven for about 8 minutes!" Come to think of it, that's not peculiar at all. The cashier is simply a gateway between two worlds: The customer who wants the pizza, and the cook who makes the pizza. For all the cashier knows, the cook gets his pizza from aliens or slices them from Jimmy Carter (he's a dwindling resource, people).

That is your situation. Your cashier isn't dumb. He knows how to make pizza. That doesn't mean he should be making pizza, or telling someone how to make pizza. That's the cook's job. As other answers (notably Florian Margaine's and Madara Uchiha's) illustrated, there is a separation of responsibilities. The model might not do much, it might be just one function call, it might be even one line - but that doesn't matter, because the controller doesn't care.

Now, let's say the owners decide that pizzas are just a fad (blasphemy!) and you switch over to something more contemporary, a fancy burger joint. Let's review what happens:

You enter a fancy burger joint and walk over to the counter. "Welcome to Le Burger Maestro Double Deluxe, may I take your order?" "yeah, I'll have one large hamburger with olives". "Okay", and he turns to the kitchen, "One Jimmy Carter!"

And then, you get a large hamburger with olives (ew).

Answer 2

Option 1 and 2 are bit extreme and like the choice between the devil and the deep blue sea but if I had to choose between the two I would prefer option 1.

First of all, option 2 will throw a runtime exception because Entity Framework does not support to project into an entity (Select(e => new Kitten(...)) and it does not allow to use a constructor with parameters in a projection. Now, this note seems a bit pedantic in this context, but by projecting into the entity and returning a Kitten (or an enumeration of Kittens) you are hiding the real problem with that approach.

Obviously, your method returns two properties of the entity that you want to use in your view - the kitten's name and imageUrl. Because these are only a selection of all Kitten properties returning a (half-filled) Kitten entity would not be appropriate. So, what type to actually return from this method?

• You could return object (or IEnumerable<object>) (that's how I understand your comment about the "object method") which is fine if you pass the result into Json(...) to be processed in Javascript later. But you would lose all compile time type information and I doubt that an object result type is useful for anything else.
• You could return some named type that just contains the two properties - maybe called "KittensListDto".

Now, this is only one method for one view - the view to list kittens. Then you have a details view to display a single kitten, then an edit view and then a delete confirm view maybe. Four views for an existing Kitten entity, each of which needs possibly different properties and each of which would need a separate method and projection and a different DTO type. The same for the Dog entity and for 100 entities more in the project and you get perhaps 400 methods and 400 return types.

And most likely not a single one will be ever reused at any other place than this specific view. Why would you want to Take 10 kittens with just name and imageUrl anywhere a second time? Do you have a second kittens list view? If so, it will have a reason and the queries are only identical by accident and now and if one changes the other one does not necessarily, otherwise the list view is not properly "reused" and should not exist twice. Or is the same list used by an Excel export maybe? But perhaps the Excel users want to have 1000 kittens tomorrow, while the view should still display only 10. Or the view should display the kitten's Age tomorrow, but the Excel users don't want to have that because their Excel macros would not run correctly anymore with that change. Just because two pieces of code are identical they don't have to be factored out into a common reusable component if they are in a different context or have different semantics. You better leave it a GetLatestKittensForListView and GetLatestKittensForExcelExport. Or you better don't have such methods in your service layer at all.

---

In the light of these considerations an excursion to a Pizza shop as an analogy why the first approach is superior :)

"Welcome to BigPizza, the custom Pizza shop, may I take your order?" "Well, I'd like to have a Pizza with olives, but tomato sauce on top and cheese at the bottom and bake it in the oven for 90 minutes until it's black and hard like a flat rock of granite." "OK, Sir, custom Pizzas are our profession, we'll make it."

The cashier goes to the kitchen. "There is a psycho at the counter, he wants to have a Pizza with... it's a rock of granite with ... wait ... we need to have a name first", he tells the cook.

"No!", the cook screams, "not again! You know we tried that already." He takes a stack of paper with 400 pages, "here we have rock of granite from 2005, but... it didn't have olives, but paprica instead... or here is top tomato ... but the customer wanted it baked only half a minute." "Maybe we should call it TopTomatoGraniteRockSpecial?" "But it doesn't take the cheese at the bottom into account..." The cashier: "That's what Special is supposed to express." "But having the Pizza rock formed like a pyramid would be special as well", the cook replies. "Hmmm ... it is difficult...", the desparate cashier says.

"IS MY PIZZA ALREADY IN THE OVEN?", suddenly it shouts through the kitchen door. "Let's stop this discussion, just tell me how to make this Pizza, we are not going to have such a Pizza a second time", the cook decides. "OK, it's a Pizza with olives, but tomato sauce on top and cheese at the bottom and bake it in the oven for 90 minutes until it's black and hard like a flat rock of granite."

---

If option 1 violates a separation of concerns principle by using a database context in the view layer the option 2 violates the same principle by having presentation centric query logic in the service or business layer. From a technical viewpoint it does not but it will end up with a service layer that is anything else than "reusable" outside of the presentation layer. And it has much higher development and maintenance costs because for every required piece of data in a controller action you have to create services, methods and return types.

Now, there actually might be queries or query parts that are reused often and that's why I think that option 1 is almost as extreme as option 2 - for example a Where clause by the key (will be probably used in details, edit and delete confirm view), filtering out "soft deleted" entities, filtering by a tenant in a multi-tenant architecture or disabling change tracking, etc. For such really repetetive query logic I could imagine that extracting this into a service or repository layer (but maybe only reusable extensions methods) might make sense, like

public IQueryable<Kitten> GetKittens()
{
    return context.Kittens.AsNoTracking().Where(k => !k.IsDeleted);
}

Anything else that follows after - like projecting properties - is view specific and I would not like to have it in this layer. In order to make this approach possible IQueryable<T> must be exposed from the service/repository. It does not mean that the select must be directly in the controller action. Especially fat and complex projections (that maybe join other entities by navigation properties, perform groupings, etc.) could be moved into extension methods of IQueryable<T> that are collected in other files, directories or even another project, but still a project that is an appendix to the presentation layer and much closer to it than to the service layer. An action could then look like this:

public ActionResult Kittens()
{
    var result = kittenService.GetKittens()
        .Where(kitten => kitten.fluffiness > 10) 
        .OrderBy(kitten => kitten.name)
        .Select(kitten => new {
            Name=kitten.name,
            Url=kitten.imageUrl
        })
        .Take(10);
    return Json(result,JsonRequestBehavior.AllowGet);
}

Or like this:

public ActionResult Kittens()
{
    var result = kittenService.GetKittens()
        .ToKittenListViewModel(10, 10);
    return Json(result,JsonRequestBehavior.AllowGet);
}

With ToKittenListViewModel() being:

public static IEnumerable<object> ToKittenListViewModel(
    this IQueryable<Kitten> kittens, int minFluffiness, int pageItems)
{
    return kittens
        .Where(kitten => kitten.fluffiness > minFluffiness)
        .OrderBy(kitten => kitten.name)
        .Select(kitten => new {
            Name = kitten.name,
            Url = kitten.imageUrl
        })
        .Take(pageItems)
        .AsEnumerable()
        .Cast<object>();
}

That's just a basic idea and a sketch that another solution could be in the middle between option 1 and 2.

Well, it all depends on the overall architecture and requirements and all what I wrote above might be useless and wrong. Do you have to consider that the ORM or data access technology could be changed in future? Could there be a physical boundary between controller and database, is the controller disconnected from the context and do the data need to be fetched via a web service for example in future? This would require a very different approach which would more lean towards option 2.

Such an architecture is so different that - in my opinion - you simply can't say "maybe" or "not now, but possibly it could be a requirement in future, or possibly it won't". This is something that the project's stakeholders have to define before you can proceed with architectural decisions as it will increase development costs dramatically and it will we wasted money in development and maintenance if the "maybe" turns out to never become reality.

I was talking only about queries or GET requests in a web app which have rarely something that I would call "business logic" at all. POST requests and modifying data are a whole different story. If it is forbidden that an order can be changed after it is invoiced for example this is a general "business rule" that normally applies no matter which view or web service or background process or whatever tries to change an order. I would definitely put such a check for the order status into a business service or any common component and never into a controller.

There might be an argument against using IQueryable<T> in a controller action because it is coupled to LINQ-to-Entities and it will make unit tests difficult. But what is a unit test going to test in a controller action that doesn't contain any business logic, that gets parameters passed in that usually come from a view via model binding or routing - not covered by the unit test - that uses a mocked repository/service returning IEnumerable<T> - database query and access is not tested - and that returns a View - correct rendering of the view is not tested?

Answer 3

This is the key phrase there:

I might use this class in other projects and not just the asp.net MVC project.

A controller is HTTP-centric. It is only there to handle HTTP requests. If you want to use your model in any other project, i.e. your business logic, you can't have any logic in the controllers. You must be able to take off your model, put it somewhere else, and all your business logic still works.

So, no, don't access your database from your controller. It kills any possible reuse you might ever get.

Do you really want to rewrite all your db/linq requests in all your projects when you can have simple methods that you reuse?

Another thing: your function in option 1 has two responsibilities: it fetches the result from a mapper object and it displays it. That's too many responsibilities. There is an "and" in the list of responsibilities. Your option 2 only has one responsibility: being the link between the model and the view.