One thing I really don’t like about software engineers or software craftmen is that they tend to make things more complicated then they actually are. They love to admire their own code and speak or write about the elegance of the architecture and patterns they used. There is no problem with that, as long as they keep their code simple.
Yes, I know I’m guilty too and I often discussed this topic with my co-worker Christoph (@dasheddot), who is one of the dark side. At the time writing this, I’m reviewing an early ASP.NET MVC application. It is a multi-tenant web-application and its solution looks like this:
- MyApp.Data
- MyApp.Model
- MyApp.Core
- MyApp.Web
- MyApp.Tests there should be one, but actually there is no such
Until now, there is no real problem except that they all could go under one single project MyApp.Web, which is what I suggest for the sake of simplicity. You might argue that it’s easier to manage dependencies between the different layers of your application, but if you really have issues with that I suggest you go for tools like NDepend.
Let’s review the different projects, beginning with the .Data project.
We’re using MS SQL and FluentNHibernate in this application. For whatever reason I had (or more honestly: reason I did not have) I decided to use a repository pattern. Being a good Clean Code citizen I would also have interfaces for all of them and make sure that DRY is respected.
This leads to the following Interfaces: IRepository, ICommonRepository, ITenantRepository
And these abstract base-classes: Repository, CommonRepository, TenantRepository
Take a look a the Repository:
public interface IRepository<T> where T : Entity
{
T GetById(int id);
IList<T> FindAll();
void Add(T entity);
void Remove(T entity);
void Update(T entity);
}
public abstract class Repository<T> : IRepository<T> where T : Entity
{
protected ISession Session { get; private set; }
// our custom Unit-of-Work implementation
protected ContextScope Scope { get; private set; }
public Repository(ContextScope scope)
{
Scope = scope;
}
public virtual T GetById(int id)
{
return Session.Get<T>(id);
}
public virtual IList<T> FindAll()
{
return Session.Query<T>()
.ToList();
}
public virtual void Add(T entity)
{
Session.Save(entity);
}
public virtual void Update(T entity)
{
Session.Update(entity);
}
public virtual void Remove(T entity)
{
Session.Delete(entity);
}
}
And here is the TenantRepository from which all the other repositories for tenant-specific data will derive:
public abstract class TenantRepository<T> : Repository<T>, ITenantRepository<T> where T : TenantEntity
{
public TenantRepository(ContextScope scope)
: base(scope)
{
}
public override T GetById(int id)
{
return base.GetById(id);
}
public override IList<T> FindAll()
{
return Session.Query<T>()
.Where(x.TenantId == Scope.CurrentTenantId);
.ToList();
}
public override void Add(T entity)
{
entity.TenantId = Scope.CurrentTenantId;
base.Add(entity);
}
public override void Update(T entity)
{
if (entity.TenantId <= 0)
throw new Exception("Entity has invalid TenantId assigned and must not be updated!");
base.Update(entity);
}
public override void Remove(T entity)
{
if (entity.TenantId <= 0)
throw new Exception("Entity has invalid TenantId assigned and must not be removed!");
base.Remove(entity);
}
}
A concrete repository will then look like this:
public interface ICustomerRepository : ITenantRepository<Customer>
{
IList<Customer> Find(string text);
IList<Customer> GetByName(string name);
}
public class CustomerRepository : TenantRepository<Customer>, ICustomerRepository
{
public CustomerRepository(ContextScope scope)
: base(scope)
{
}
public IList<Customer> Find(string text)
{
var query = Session.QueryOver<Customer>()
.Where(x => x.AccountId == Scope.AccountId)
.AndRestrictionOn(x => x.Name).IsLike("%" + text + "%");
return query.List();
}
public IList<Customer> GetByName(string name)
{
var query = Session.QueryOver<Customer>()
.Where(x => x.AccountId == Scope.AccountId)
.And(x => x.Name == name);
return query.List();
}
}
Pretty cool, isn’t it?
>> No, definitely not! But more on that later, just take a look at the CustomerService first, which lives in the .Core project:
public class CustomerService : ICustomerService
{
private ContextScope _scope;
private readonly ICustomerRepository _customerRepository;
private readonly IProjectRepository _projectRepository;
public CustomerService(ContextScope scope)
{
if (scope == null)
throw new ArgumentNullException("scope");
this._scope = scope;
_customerRepository = new CustomerRepository(scope.InnerScope);
_projectRepository = new ProjectRepository(scope.InnerScope);
}
public IList<Customer> GetAll()
{
return _customerRepository.FindAll();
}
public Customer GetById(int id)
{
return _customerRepository.GetById(id);
}
public IList<Customer> Find(string text)
{
return _customerRepository.Find(text);
}
public void AddCustomer(Customer customer)
{
customer.Validate();
if (_customerRepository.GetByName(customer.Name).Any())
throw new ValidationException("Duplicate name");
_customerRepository.Add(customer);
}
public void UpdateCustomer(Customer customer)
{
customer.Validate();
foreach (var duplicate in _customerRepository.GetByName(customer.Name))
if (duplicate.Id != customer.Id)
throw new ValidationException("Duplicate name");
_customerRepository.Update(customer);
}
public void DeleteCustomer(Customer customer)
{
customer.Validate();
if (_projectRepository.GetCount(customer) > 0)
throw new ValidationException("Projects exist");
_customerRepository.Remove(customer);
}
}
… and inside the CustomersController:
public ActionResult Details(int id)
{
Customer customer;
using (var tx = Scope.Current.BeginTransaction())
{
customer = new CustomerService(Scope.Current).GetById(id);
tx.Commit();
}
return View(customer.MapTo(CustomerViewModel));
}
So, what is the problem with this architecture?
Actually there are 2:
First, it is very slow to develop.
Let’s say we want to add an action ByNumber that shows a single customer based on his Number. We would have to touch the following pieces: CustomersController (of course), ICustomerService, CustomerService, ICustomerRepository, CustomerRepository. All that, just for something simple as Session.Query<Customer>.Where(x => x.Number == number).
Second, optimizing for performance sucks.
We all know that we should reduce the number of requests to the database and that there are such evil things like Select N+1. Most of them are a result of lazy-loading, so the solution is to let NHibernate eager-load your entities from the database. Normally this would be a really easy task, but with our architecture it will be hard, since you don’t want the eager loading to apply everywhere one uses the repository method. This is a fight between DRY and single responsibility.
To give you an example – In a project management application we want to display a list of tasks, along with the names of the project and the customer they belong to. To avoid Select+1 we design our query like this:
public class TaskRepository : TenantRepository<Task>, ITaskRepository
{
public IList<Task> GetAll()
{
return Session.QueryOver<Task>()
.Fetch(x => x.Project).Eager
.Fetch(x => x.Customer).Eager
.List();
}
}
Great, we load all the tasks along with their projects and customers in one roundtrip to the database. Now, at another place in our application we also want to show a list of tasks, but there we don’t need the customer. Somewhere else we have an autocomplete box where we only need the name of the task. How can we do that?
Option 1:
public class TaskRepository : TenantRepository<Task>, ITaskRepository
{
public IList<Task> GetAll()
{
return Session.QueryOver<Task>()
.List();
}
public IList<Task> GetAllEagerProjects()
{
return Session.QueryOver<Task>()
.Fetch(x => x.Project).Eager
.List();
}
public IList<Task> GetAllEagerProjectsAndCustomers()
{
return Session.QueryOver<Task>()
.Fetch(x => x.Project).Eager
.Fetch(x => x.Customer).Eager
.List();
}
}
Option 2:
public class TaskRepository : TenantRepository<Task>, ITaskRepository
{
public IList<Task> GetAll(bool eagerProjects = false, bool eagerCustomers = false)
{
var query = Session.QueryOver<Task>();
if (eagerProjects)
query = query.Fetch(x => x.Project).Eager;
if (eagerCustomers)
query = query.Fetch(x => x.Customer).Eager;
return query.List();
}
}
Now imagine you have very complex queries with more than 5 different things one could want to eagerly load. Not really nice…
Instead, ask yourself – what value does it provide to have these architecture?
For me the answer is simply: no value at all. Therefore I will rip out all these unnecessary layers and instead query the NHibernate Session object directly in my controller actions. That means, I remove all of my interfaces, all repository classes and the unit-of-work encapsulation class, which just wraps up the NHibernate Session anyway.
Doing so, my controller actions will be much easier to understand (no need for endless F12 navigation), queries are easier to optimize and in the end it will speed up development of new features.
However, I will keep my Services, not the interfaces but the classes! They won’t provide any methods to read, but they have methods (or should I say commands, eh?) to write to the database. That way I can ensure validation and domain logic but still can query very easily.
How should I call this architecture? “Poor-mans CQRS”?
Exactly! I’ve been doing the same lately. PM’s CQRS is a good way to put it.
I’ve been coming to the same conclusions lately. Could you possibly show how you would unit test such an architecture? The only real advantage of IRepositories and IServices that I can see are the mocking possibilities they provide when unit testing.
I don’t test CRUD. For the parts that are worth testing I can do so using SQLite or EmbeddableDocumentStore if I’m on raven.But you’re right – testing was easier with interfaces.
about the name, you can call it EQO
http://fabiomaulo.blogspot.com/2010/07/enhanced-query-object.html
I think one of the problems here is that people take programming to interfaces too literally. Programming to an interface\contract does not just mean the c# interface.
I believe in this case NhIbernate is fulfilling the Repository pattern contract\interface here so implementing another on top is of no value.
I tend to wrap the session however to provide a Unit of Work interface , which can also be mocked for testing.
- Did you also remove all your tests? Or you didn’t have any?
- I agree, you don’t need all the fluff. Being closer to the metal allows for more power. And I have yet to see the first project that switches from NH to EF halfway in. That’s also my beef with ORMs though. I’d rather have one that optimizes for the relational platform I’m targeting instead of the lowest common denominator.
- Aside from this all, you might wanna take a look at using fetching strategies (http://www.infoq.com/presentations/Making-Roles-Explicit-Udi-Dahan).
Thanks for the link, I will have a look at it.
As for testing – in this particular application I actually had no tests. But on more recent works it was not that hard, you can use embedded SQLite in case you’re on NHibernate.
Hardly unit testing … but then again maybe YAGNI in your case.
I was under the impression you also made “products” at your company. For those, I’d recommend you build in the proper abstractions as they tend to require more adjustments.
Don’t fight between DRY and Single responsibility. Single responsibility should always win. DRY is an anti pattern in my opinion. It is infinitely more maintainable to have small aggregates covering the most specific behaviour, than a phony highly structural model a developer believes is correct, at the time.
Repeat yourself as often as possible, be as specific as possible and avoid inheritance. This is the road to truly simple code.
Yeah! I really think that DRY is not good for some case, and inheritance some time make things more complex (like a mesh).
Awesome post – thanks for this.
I’m probably being thick, but if you move your Session.QueryOver() querying logic directly into your controllers like as you said, doesn’t that mean that..
1) You’re going to have some DRY issues, since you may have many controllers/action methods that need to get some query frequently, and your query isn’t encapsulated in a separate object. E.g. a couple of places want to know “all customers by surname X”
2) Testing would become an issue, unless you fake out Session (sorry I dont know nhib so i might have missed a point)
Again, I might be a bit thick asking these questions.
Don’t worry, these are interesting question.
1) I don’t consider them to be issues. If I want to query “all customers by surname X” on a couple of places, chances are that these places have different views on the data and so they might have different fetching strategies (see the paragraph about optimization above). However, there are common things you could want to query often like the current user. In this particular case I have an extension method on the ISession, but you could also have a query class and it will be just fine.
2) You don’t need to fake ISession. Instead you can use embedded SQLite. However, I don’t test stupid crud operations.
so what is your plan on handling data caching when two different controllers need the same data?
There should be no difference in this regard. I open/close sessions and transcations inside a custom ActionFilterAttribute (respecting Child-Actions) on a per-action basis. If I want to address caching between different controllers but within the same request then I’ll store my Session inside the HttpContext and close it at the end of the request.
What’s the problem?
I completely agree you on keeping the things simple. Your point is valid if there are no business logic involved for fetching data(like the example you have shown).
In some application there will business rules for fetching the data also, for example “GetAll” in the “CustomerService” should not return all the customers in the system instead return only the customers that are allowed to see by the logged in user.
In that case, I will write that logic in the service class, instead of putting it in the controller. I won’t be creating the repository method for this case but will use the same repository method call _customerRepository.FindAll()(provided this is not going to return millions of rows) in the service layer, then I will loop through the returned collection and apply my business logic.
Rajeesh There shouldn’t be business logic in a query.
@Mark that depends on what we want to achieve. You can’t say that is true in all the scenarios
With regards to comments that this change makes unit testing more difficult- it doesn’t make acceptance testing more difficult. And if you’re writing acceptance tests, you don’t need to write unit tests for every simple action method. And until the code that would use the repository is really that actually complicated, acceptance are a better choice anyhow.
@Rajeesh Fetch Queries are data only, readonly adhoc insight into a system.
However Applying some kind of state change to the system will invoke some logic. Such as stopping a logged in user from modifying a customer when not appropriate as defined by some business rule.
It seems to me that placing a query as close to the controller action as possible provides the best solution always. If you are worried about repeating queries than place them in a separate object on their own.
The whole thing seems like a straw man argument to me. The repository classes are setup based on unvalidated assumptions about actual acceptance criteria, so the resulting code has YAGNI written all over it. By contrast the proposed “solution” seems simple. But clunky to test.
@Mark
“If you are worried about repeating queries than place them in a separate object on their own.”
Yes, I would call that in a separate class of it’s own, which will be service class. That could be reused in different controllers or on any external service which I will be exposing.
@Rajeesh OK I concede that you can place the code in some shared space, whether it be a class or static method. However it isn’t a reason to crack out the repository pattern.
The fact is that NHIbernate gives us what we need in respect to an abstracted DAL the problem the repository pattern tries to address.
Use the database Luke! Capable and powerful as it is being build on math it is disturbing to behold how we build such arbitrary and unnecessary distinctions in our solutions. Isn’t all application logic “business” logic by definition? (Why is that field required?) Isn’t the select n +1 problem a result of trying to fit a square peg into a round hole? You would think we’d be smarter than that. Yet we’ve built our architectural fences so high and in such a maze of competing (non mathematical) abstractions (testable? SRP? Business/Data Access logic? dry? layers?) that we haven’t seen the forest for some time. What would happen if we try to think about building systems as sets of composable higher order functions? We should at least be thinking about it…
Hi Daniel,
Interesting post. I agree there is a use case for just having NHibernate calls in your controller.
However, when using the repository pattern with NHibernate, there is a simple solution which gets around select n + 1 and doesn’t require conditional eager loading. That is batch fetching.
http://www.nhforge.org/doc/nh/en/index.html#performance-fetching-batch
When getting all tasks with projects and customers it will make 3 calls. One for the tasks, one of the projects and one for the customers which is actually often faster than doing the one query with 2 joins. When you are just getting the tasks you can call the same method on the repository and it won’t fetch the customers/projects.
Whether the numbers of calls is 3 depends on the batch-size and the number of items you get back. Whether it is faster then joining depends on these (variable) number and also on the database latency.
I have another problem with that approach: It decouples the place where you define if you _need_ eager-fetching (the view) from the place where you define it (the mapping). It becomes dangerous to forget about the batch-size while working on the controller.
That’s true, if the batch size is less than the total projects/customers then there will be more queries. Although, I’m not aware of a downside of setting your batch size quite large eg 500 which means you shouldn’t ever hit that limit.
The potential downside of forgetting about batch size is pretty small IMO. If you are lazy loading a bunch of projects the batch-size works well. If you aren’t lazy loading any projects it doesn’t impact. The only case where there may be a slight negative effect is when you lazy load only one project, as this will cause NHibernate to wait to see if more come through, but even in this case it is going to be a slight difference in performance.
Also, if you can decouple the controller from having to get its hands dirty with eager and lazy loading, this should be a nice MVC win and simplifies your controller and model code.
Anyway, it’s a good option to keep in mind. For the project that I was working on I found this to be faster about 90% of the time (and the times when it wasn’t faster was when there was almost no data so either way doesn’t really matter) and it simplified my Controller and Model code. But obviously you would need to compare these 2 methods performance wise to make a choice with your data/database.
Yes, I agree. One should always compare the possible methods on a project basis. Thanks for your useful hint!
This is a valid critique of some “best practices.” Often it doesn’t make sense to go overboard with a simple app. However, I think there are times when all the interfaces and repositories have value. The wise developer knows when that is.
And in a larger system that will be maintained over time, testability may be more important than you seem to think.
Sure, you have to choose the right tools and methods that fit the complexity of the system you develop. I don’t suggest to write SPROCs or throw handwritten SQL all over your code. I’m an advocate of best-practise patterns but at least you should be conscious about why and when to choose a particular one. If we’re talking about data-driven CRUD-applications (and most of the times it’s just about that) I see no point of having all these layers of abstraction.
Thank you! Now I finally have a good explination for doing what I’ve always naturally done. Keep it simple stupid.
In my opinion controller should be as simple as possible, but querying NHibernate session exactly from class we will end up with controller with 200 line of code just for 4-5 Actions and I believe it’s can’t be called Simple.
BTW: interfaces allow to use IoC
Most of the queries in many applications are as simple as:
var customers = Session.Query()
.Where(x => x.Name == “whatever”)
.Skip(page * pageSize).Take(pageSize)
.ToList();
These queries don’t let your controller actions grown from let’s say 20 to 200 lines of code. If you have more complex queries and there will certainly be one (especially in reporting scenarios) you can put them in their own QueryWhatever class.
There isn’t much point in using the MVC pattern unless you use it as it was intended – you may as well just go back to using Webforms with data access controls on the page. Not having a go, just pointing out that there are many more reasons to do it the ‘MVC’ way than the ‘simple’! Doing it the ‘simple’ way would very rapidly become unmanageable in any non-small or non-trivial application, as anyone who has worked with Webforms will know.
The point of the MVC/repository pattern is that it encourages you to think about separation of concerns (SOC), which is an important idea even in simple apps. Therefore putting data access (a Model function) in your controller is not encouraged. In fact many advocates of MVC would say that you shouldn’t even be calling the repository functions directly from the controller either, although there is not so much of a consensus about that. The reasons for SOC are many-fold – inversion-of-control, easy testing, easier maintenance, common structure to all projects/solutions (which means anyone new in the team can find your data-access stuff easily), much quicker extensibility, simpler to create documentation, and so on. Most importantly it makes you think about what you are doing.
And it really doesn’t take that long to code if you use CRUD scaffolders to create it!
Totally agree with you!
I disagree that the repository pattern in itself is complex, but this implementation certainly is.
If I were going this route, I would keep the ICustomerRepository and leave out the base interfaces. If I should decide I don’t need to mock the repository, I’d leave out the interface all together.
In either case, I should have one implementation, the CustomerRepository class. It could inherit from a base Repository if that’s convenient.
How would you rate using something like Microsoft Moles to perform unit/integration testing in an architecture like this? I don’t like seeing interfaces created for implementations solely to enable mocking and unit testing – Moles lets you create mock objects for anything without having to apply this abstraction layer and that also includes static methods even when they’re on system objects.
It also irritates me when I see Service Locators that only ever tie one implementation to an interface – another layer of obfuscation!
Why not just have the repositories expose iqueryable?
I wouldn’t do this because then it wasn’t clear where the execution of the query actually happens. I know, since Rob Conery showed this approach in his StoreFront series it was picked up by a lot of developers, but I still find it _very_ bad. Second, it violates the definition of a repository (if you take Fowlers definition, which is considered to be _the_ definition). Third, what value would these repository provide then at all?
I don’t think it’s simple. It’s simple to write, but hard to maintain and hard to understand. You provided an example with simple ‘forwarded calls’ between layers, but missed the point of SRP. And I think SRP makes the whole architecture simple. It’s not about how much line of code you write (one line in each of your layer, in the example), it’s about the separation of concerns. Yes, it’s faster to write a direct nhib query inside a controller, at the time. But after that, you cannot rely on SRP. You have to look up similar functionalities in completely different layers. Following your assumption, you could write @Model.GetById(42).Username in your razor template. Because it’s simple. It’s not simple, it’s just a short-term cut on development time, without concept behind it. SRP is not just about classes, you can freely apply SRP to components, layers, whatever. And I think it’s very important, because this is how you can model a real world problem with software tools.
Thank for your feedback. I understand your point but obviously I don’t agree with that. In my opinion SRP isn’t violated when you call a NH or RavenDB function directly without putting additional layers of abstraction in between. Ultimately, that’s what a controller action is supposed to do, isn’t it? If you find yourself writing the same queries over and over again, you can always refactor them out to some sort of queryable extension or query object.
I don’t think query construction should be a responsibility of a controller. Also, a controller should not be aware of the actual ORM layer. But this is my point. In a trivial application, there is no problem with this.