NHibernate unit of work in MVC using Ninject (part 2)

In part 1 I talked about a problematic NHibernate Unit of Work implementation that we had in our MVC application. In part 2 I will talk about why this approach turned out to be problematic and the ultimate solution that we came up with.

If you recall, the original solution was to use an HttpModule to subscribe to the BeginRequest and EndRequest events. When we upgraded to Ninject 2.1, none of our entities would get saved. Reads all worked fine, but no CUD actions worked. The problem turned out to be due to a new component of Ninject called the OnePerRequestModule. This module subscribes to the EndRequest event and disposes any objects that were bound in request scope. The problem was that this module would dispose of our session, before we had a chance to commit the transaction and end the session ourselves. It was also registering first (which meant it was first to handle EndRequest) and there appeared to be no hook to turn it off.

The solution that we came up with was to use a filter attribute in order to encapsulate our unit of work. The first part of the refactor was to create an IUnitOfWork interface:

public interface IUnitOfWork
{
 void Begin();
 void End();
}

That's all a unit of work really should look like. Second we moved our BeginSession and EndSession code into an implementation of IUnitOfWork:

public class NHibernateUnitOfWork : IUnitOfWork, IDisposable
{
 private readonly ISession _session;

 public NHibernateUnitOfWork(ISession session)
 {
  _session = session;
 }

 public void Begin()
 {
  _session.BeginTransaction();
 }

 public void End()
 {
  if (_session.IsOpen)
  {
   CommitTransaction();
   _session.Close();
  }
  _session.Dispose();
 }

 private void CommitTransaction()
 {
  if (_session.Transaction != null && _session.Transaction.IsActive)
  {
   try
   {
    _session.Transaction.Commit();
   }
   catch (Exception)
   {
    _session.Transaction.Rollback();
    throw;
   }
  }
 }

 public void Dispose()
 {
  End();
 }
}

This separated repository logic from the unit of work implementation. The binding for this implementation was bound in request scope. Finally we created our filter attribute:

public class UnitOfWorkAttribute : FilterAttribute, IActionFilter
{
 [Inject]
 public IUnitOfWork UnitOfWork { get; set; }

 public UnitOfWorkAttribute()
 {
  Order = 0;
 }

 public void OnActionExecuting(ActionExecutingContext filterContext)
 {
  UnitOfWork.Begin();
 }

 public void OnActionExecuted(ActionExecutedContext filterContext)
 {
  UnitOfWork.End();
 }
}

The order was set to 0 to ensure that this filter was always run first. We also did not have to explicitly call End on the unit of work since it would be disposed at the end of the request and would call End itself. However, it's good practice to be explicit with these things. Finally you can apply this filter at the controller level or the action level. So you can decorate only those actions that actually perform data access.
8USTXZWHRBTH

NHibernate unit of work in MVC using Ninject (part 1)

We just released our third and (likely) final release of the application we've been working on for the last 11 months. During that time I've gone through two implementations of the NHibernate Unit of Work pattern. I will talk about it in two parts. The first part will talk about our first implementation, that turned out to be a little bit flawed and very incompatible with Ninject 2.1. The second one will talk about a better approach that we shipped with this final release.

I've previous blogged about NHibernate and the unit of work pattern here. The approach, I first decided to take was to create a generic Repository class. This class was not only responsible for CRUD operations and transaction management, but it also had 2 special methods on it called BeginRequest and EndRequest. Here's a simple implementation:

public class Repository : IRepository
{
    private ISession _session;
    public Repository(ISession session) { _session = session; }

    public Save<T>(T entity) where T : Entity
    {
        _session.SaveOrUpdate(entity);
    }
    public Get<T>(int id) where T : Entity
    {
        return _session.Get<T>(id);
    }
    public Delete<T>(T entity) where T : Entity
    {
        _session.Delete(entity);
    }

    public BeginTransaction() { _session.BeginTransaction(); }
    public CommitTransaction() { _session.Transaction.Commit(); }
    public RollbackTransaction() { _session.Transaction.Rollback(); }

    public BeginRequest() { BeginTransaction(); }
    public EndRequest() { CommitTransaction(); }
}

Obviously the real class was more robust and had much more error checking around transaction management, but those details aren't important for this discussion.

In addition to this Repository we also had an HttpModule that called Repository.BeginRequest() on BeginRequest and Repository.EndRequest() on EndRequest. It looked something like this:

public class DataModule : IHttpModule
{
 public void Init(HttpApplication context)
 {
  context.BeginRequest += BeginRequest;
  context.EndRequest += EndRequest;
 }

 public void Dispose()
 {
 }

 public void BeginRequest(object sender, EventArgs e)
 {
  var app = (WebApplication)sender;
  var repository = app.Kernel.Get<IRepository>().BeginRequest();
 }

 public void EndRequest(object sender, EventArgs e)
 {
  var app = (WebApplication)sender;
  var repository = app.Kernel.Get<IRepository>().EndRequest();
 }
}

Our Ninject bindings for these classes were as follows:

Bind<ISession>()
       .ToMethod(context => NHibernateSessionFactory.Instance.OpenSession())
       .InRequestScope();
Bind<IRepository>().To<Repository>().InTransientScope();

An important note is that the NHibernate session was bound in request scope, while the repository was bound in transient scope. So all instances of Repository in request scope, shared the same session. (NHibernateSessionFactory is our singleton in charge of configuring the SessionFactory and creating new sessions.)

This approach worked fine with Ninject 2.0, but suffered from several problems. First of all, we would be creating a new session and starting/commiting a new transaction on every http request. This means any request, even for content files like javascript or css, would trigger the DataModule. The second problem was, more architectural/stylistic. We had code that dealt with unit of work implementation inside a repository. We could have moved the Begin/End Request logic off to the DataModule and that would have alleviated this problem some what. However, we still would have been stuck with the first problem.

The kick in the pants for the rewrite came, when we upgraded to Ninject 2.1 and Ninject.Web.Mvc 2.1. Ninject 2.1 introduced a very eager way to ensure objects bound in Request scope didn't hang around after the EndRequest event. I'll talk about this and our implementation rewrite in part 2.

NInject, NHibernate, and unit testing

I recently came across an interesting problem in my tests. In the application I am working on, I have recently created a framework for generating tag numbers for pieces of equipment. I put unit tests around the code and decided that the system was complex enough to warrant some integration tests.

The set up for the integration tests is quite complex as I have to not only set up an NHibernate session, but also the Ninject kernel. In my app the way I bind the NHibernate session is through the following line:

Bind<ISession>().ToMethod(c => NHibernateSessionFactory.Instance.OpenSession()).InRequestScope();

In my integration test SetUp I create a session for the test to use and start a transaction. In the TearDown I rollback the transaction and dispose the session. So I needed to manually change the binding to ISession in Ninject. I did that using the following code

foreach (var binding in _kernel.GetBindings(typeof (ISession)))
{
 _kernel.RemoveBinding(binding);
}
_kernel.Bind<ISession>().ToMethod(c => _session).InRequestScope();

(I would love if anyone suggested a better way to do this)

The integration test was working fine. Until I started working on another completely unrelated module for doing auditing on certain changes to domain objects. Once the module was finished I ran the entire build again and the integration tests started failing with a message that the ISession object was closed. If they were run independently or even as part of an entire integration tests run, they passed. I finally figured out that they would only fail, when they were being run together with the tests for this new module.

The culprit turned out to be this line of code I was running in the TestFixtureSetUp for the auditing tests:

HttpContext.Current = new HttpContext(new HttpRequest(string.Empty, "http://domain.com", string.Empty), new HttpResponse(new StringWriter()));

Since the HttpContext.Current was staying the same, the integration tests were only ever providing one ISession instance for all of the tagging integration tests. So once the first test ran and disposed of the session, any other components that required an ISession were still getting the old disposed instance of the ISession. The fix was simple, I just had to set HttpContext.Current back to null in the TestFixtureTearDown for the audit module test.