Ayende @ Rahien

It's a girl

Refactoring toward frictionless & odorless code: The baseline

Originally posted at 3/30/2011

This is part of an exercise that I give in my course. The context is an MVC3 ASP.Net application. Here is how we start things:

public class MvcApplication : System.Web.HttpApplication
{
    private static readonly ISessionFactory sessionFactory = BuildSessionFactory();

    public static ISession CurrentSession
    {
        get{ return HttpContext.Current.Items["NHibernateSession"] as ISession;}
        set { HttpContext.Current.Items["NHibernateSession"] = value; }
    }

    public MvcApplication()
    {
        BeginRequest += (sender, args) =>
        {
            CurrentSession = sessionFactory.OpenSession();
        };
        EndRequest += (o, eventArgs) =>
        {
            var session = CurrentSession;
            if (session != null)
            {
                session.Dispose();
            }
        };
    }

    protected void Application_Start()
    {
        AreaRegistration.RegisterAllAreas();

        RegisterGlobalFilters(GlobalFilters.Filters);
        RegisterRoutes(RouteTable.Routes);
    }

    private static ISessionFactory BuildSessionFactory()
    {
        return new Configuration()
            .Configure()
            .BuildSessionFactory();
    }
}

And in the controller, we have something like this:

public class HomeController : SessionController
{
    public ActionResult Blog(int id)
    {
        var blog = MvcApplication.CurrentSession.Get<Blog>(id);

        return Json(blog, JsonRequestBehavior.AllowGet);
    }
}

This code is valid, it works, it follows best practices and I actually recommend using something very similar here.

It also annoyed me when I wrote it now, enough to write a series of blog posts detailing how to fix this.

Comments

Robert M&#252;hsig
04/06/2011 10:26 AM by
Robert Mühsig

Is this code still unit-testable? HttpContext and the static seems to be a problem if you try to write a test for the controller action. How do you solve this?

Thanks,

Robert

Chris Hynes
04/06/2011 01:55 PM by
Chris Hynes

I'm very interested to see what you recommend. With your recent rantings on the demise of the Repository pattern, I looked at your code samples. The only things I could find were client projects, which require rather different architectures than web. MVC3 + NHibernate3 is an excellent combo.

Scott White
04/06/2011 02:02 PM by
Scott White

In other words Open Session in View (OSIV). The next step I assume will be to configure NHibernate to use web session scope?

Glenn Morton
04/06/2011 02:36 PM by
Glenn Morton

Is there much of an overhead opening a session for every request that hits the application - feels a little excessive. Would it not be better to place an filter attribute on the controller to restrict the creation of sessions to only requests that require it?

Ryan McDonald
04/06/2011 02:43 PM by
Ryan McDonald

In the case of a RavenDB session object, how could you provide a database name to the call to OpenSessoin() in BeginRequest ?

Tim
04/06/2011 02:46 PM by
Tim

Can't wait for the rest of this series.

Ayende Rahien
04/06/2011 02:46 PM by
Ayende Rahien

Ryan,

Relevance for the post?

There are ways to handle that, based on however you determine which tenant you are in, but it isn't relevant for this scenario

jhigh2000
04/06/2011 03:30 PM by
jhigh2000

As you say, It's really not bad at all, but you can definitely tuck some of the noise away by using a global action filter and your own base controller to manage your ISession.

alexander
04/06/2011 04:04 PM by
alexander

You are crazy idiot. This code is holy shit!

Josh (@rebootd)
04/06/2011 04:38 PM by
Josh (@rebootd)

Curious to see the upcoming series. I use code a lot like this in my MS MVC3 app(s).

joe
04/06/2011 09:31 PM by
joe

There might be some overhead if running that in Cassini, which runs every request (including static content) through asp.net.

I expect to see a lazy loaded UnitOfWork in the next post :)

Tyler Burd
04/07/2011 05:49 AM by
Tyler Burd

Generally I prefer taking a direct dependency on ISession rather than using some sort of "CurrentSession" accessor. Easy enough to set up ISession as a service in Windsor, at least, with a PerWebRequest scope.

Adam B
04/07/2011 07:52 AM by
Adam B

I like it, but I was using the castle nhibernate facility and replacting that with this broke my app. Some requests used multiple sessions if certain filters were being invoked. Do you think that's actually a problem with my code? Should all requests come under one session?

Ayende Rahien
04/07/2011 08:30 AM by
Ayende Rahien

Adam,

In general, yes, a single session for the entire request is the way to go.

As for replacing this. Please wait, we have much better coming up

Rob Kent
04/07/2011 09:31 AM by
Rob Kent

It's nice and simple. I have two comments though.

In MVC 2, at least, we get a BeginRequest for every static resource, including images, Javascript, and CSS files. Maybe there is little overhead to opening an NH Session and I could be being excessively defensive, but I wrote a routine to check that we were processing an ASP request before I did anything like authenticating the user or opening an ISession.

Secondly, do you always need an ISession? I prefer to use a simple 'repository' that opens the session on demand and uses NH CurrentSessionContext.

Ayende Rahien
04/07/2011 09:34 AM by
Ayende Rahien

There is no cost for opening a new NH session.

And yes, you always want a session available, see my previous posts about why I dislike the repo pattern

Scooletz
04/07/2011 10:03 AM by
Scooletz

A IoC container is the way to go. That's all about this code

Jimmy Zimms
04/07/2011 06:36 PM by
Jimmy Zimms

I'm not a fan of singletons (well not container managed) and especially the infamous HttpContext.Current property but this is one place where when it does make sense to go down that route to leverage MSR Moles for testing purposes. Again, I usually try to design around that but there's times when it's just the best approach when complexity vs "purity" is balanced out in a solution.

Comments have been closed on this topic.