AutoFixture 1.0 RC2

Wednesday, 20 January 2010 22:59:39 UTC

AutoFixture 1.0 Release Candidate 2 is now available on the CodePlex site! Compared to Release Candidate 1 there are very few changes, but the test period uncovered the need for a few extra methods on a recent addition to the library. RC2 contains these additional methods.

This resets the clock for the Release Candidate trial period. Key users have a chance to veto this release until a week from now. If no-one complains within that period, we will promote RC2 to version 1.0.

The RC2 release page has more details about this particular release.


Enabling DI for Lazy Components

Wednesday, 20 January 2010 18:08:36 UTC

My previous post led to this comment by Phil Haack:

Your LazyOrderShipper directly instantiates an OrderShipper. What about the dependencies that OrderShipper might require? What if those dependencies are costly?

I didn't want to make my original example more complex than necessary to get the point across, so I admit that I made it a bit simpler than I might have liked. However, the issue is easily solved by enabling DI for the LazyOrderShipper itself.

As always, when the dependency's lifetime may be shorter than the consumer, the solution is to inject (via the constructor!) an Abstract Factory, as this modification of LazyOrderShipper shows:

public class LazyOrderShipper2 : IOrderShipper
{
    private readonly IOrderShipperFactory factory;
    private IOrderShipper shipper;
 
    public LazyOrderShipper2(IOrderShipperFactory factory)
    {
        if (factory == null)
        {
            throw new ArgumentNullException("factory");
        }
 
        this.factory = factory;
    }
 
    #region IOrderShipper Members
 
    public void Ship(Order order)
    {
        if (this.shipper == null)
        {
            this.shipper = this.factory.Create();
        }
        this.shipper.Ship(order);
    }
 
    #endregion
}

But, doesn't that reintroduce the OrderShipperFactory that I earlier claimed was a bad design?

No, it doesn't, because this IOrderShipperFactory doesn't rely on static configuration. The other point is that while we do have an IOrderShipperFactory, the original design of OrderProcessor is unchanged (and thus blissfully unaware of the existence of this Abstract Factory).

The lifetime of the various dependencies is completely decoupled from the components themselves, and this is as it should be with DI.

This version of LazyOrderShipper is more reusable because it doesn't rely on any particular implementation of OrderShipper - it can Lazily create any IOrderShipper.


Comments

We could indicate optional dependencies by using Func<TResult> - http://msdn.microsoft.com/en-us/library/bb534960.aspx - delegates for the given constructor parameter. The given DI framework could be configured to resolve any Func<TResult> as x => Container.Resolve<TResult>()

This way the lazy/optional nature of the parameter is obvious to clients of the class, and there is no need to generate lazy implementation classes manually.

Note: I haven't had any practical experience using DI frameworks, so the above might not be possible at all :)
2010-01-20 20:44 UTC
Yes, Func<T> is sometimes a viable option. In general, I consider delegates to be anonymous interfaces, so Func<T> is really just an Abstract Factory. In other words, IOrderShipperFactory is functionally equivalent to Func<IOrderShipper>.

I had a period where I used a lot of delegates as injected dependencies, but I have more or less abandonded that approach again. While it technically works fine, it makes unit testing a bit harder because it's harder to test that a given object contains a specific type of Strategy if it's just a Func<T> or similar.

In any case, I'm mostly familiar with Castle Windsor at the moment. Although I have yet to try it out, I think the new Typed Factory Facility fits the bill very well - with that, we would never have to code a real implementation of IOrderShipperFactory because Windsor would be able to dynamically emit one for us.
2010-01-20 21:56 UTC
I have the feeling I did not set the context. Let me do that, and tell me if I the issues you raised still hold - both of them are important!

What I meant to propose is that we change Jeffrey Palermo's original example like the below:
- private readonly IOrderShipper _shipper;
+ private readonly Func< IOrderShipper > _shipperFactory;


- public OrderProcessor(IOrderValidator validator, IOrderShipper shipper)
+ public OrderProcessor(IOrderValidator validator, Func< IOrderShipper > shipperFactory)

- _shipper = shipper;
+ _shipperFactory = shipperFactory;

- _shipper.Ship(order);
+ _shipperFactory().Ship(order);

The change to the tests should be straightforward as well,

- new OrderProcessor(validator, mockShipper)
+ new OrderProcessor(validator, () => mockShipper)
2010-01-20 22:29 UTC
I'm surprised no one has mentioned .NET 4's Lazy<T>.

To communicate intent, it's clearer than Func<T>:

public UsesOrderShipper(Lazy<IOrderShipper> orderShipper)

There's a more complete example using Lazy<T> with Autofac.

Cheers,
Nick
2010-01-21 10:59 UTC
To be fair, Alwin mentioned it over on Jeffrey Palermo's original post before I posted my response.

That would definitely be an option as well, but I rather wanted to show the route involving absolutely no redesign of the original OrderProcess, and I couldn't do that purely with Lazy<IOrderShipper>. The most important point I wanted to make is that you can solve this problem using basic tools available since .NET 1.0.

It would, however, make a lot of sense to implement LazyOrderShipper by injecting Lazy<IOrderShipper> into it instead of inventing a new IOrderShipperFactory interface.
2010-01-21 17:47 UTC
I like the Func&lt;T&gt; and Lazy&lt;T&gt; solutions for addressing any real performance issues, but based upon the original example I still submit that the cleanest approach would be to just register the type with a singleton lifestyle to begin with. After the first two valid orders, it's more efficient.
2010-01-22 15:25 UTC
@Derek Greer: Aggreed, and that was also my initial point in my previous post.
2010-01-22 23:54 UTC
Thomas
Mark,

While it's easy to get it work with Typed Factory Facility and Castle, how do you implement the factory :

- without static configuration ?
- without passing the container in ?

Or I missed something ?

Thanks,

Thomas
2012-03-14 21:52 UTC
Thomas, who said anything about a DI Container or Castle Windsor for that matter?
2012-03-15 06:47 UTC
Thomas
Mark,

I was refering to your first comment. If I have no problem with the pattern I would like to know how you would do from the implementation point of view.

Thanks,

Thomas
2012-03-15 08:50 UTC
Ah, sorry... I'm not sure I entirely understand the question. With Windsor's Typed Factory Facility, you'd reqister the IOrderShipperFactory as being auto-generated by Windsor. I can't recall the exact syntax for this right now, but that registration would happen as part of the Registration phase of RRR.
2012-03-15 09:26 UTC
Thomas
Mark,

With Windsor there is no problem as TypedFactoryFacility provides implementation on the fly. However if you take another container you have to provide the implementation of IOrderShipperFactory on your own. Now the question is. How my implementation of the factory will pull the IOrderShipper implementation from the container ? I see two choices :

- configure staticaly (like Jeffrey did in his post)
- pass the container into the factory that it could resolve IOrderShipper.
- third choice that I don't know :)

I hope it's clearer now. Let me know if it doesn't make sense.

Thanks,

Thomas
2012-03-15 10:04 UTC
Thomas, I wrote a new blog post to answer your question.

HTH
2012-03-15 21:04 UTC
Thomas
Thanks Mark, I go to read it :)
2012-03-16 14:11 UTC

Rebuttal: Constructor over-injection anti-pattern

Wednesday, 20 January 2010 16:28:03 UTC

Jeffrey Palermo recently posted a blog post titled Constructor over-injection anti-pattern - go read his post first if you want to be able to follow my arguments.

His point seems to be that Constructor Injection can be an anti-pattern if applied too much, particularly if a consumer doesn't need a particular dependency in the majority of cases.

The problem is illustrated in this little code snippet:

bool isValid = _validator.Validate(order);  
if (isValid) 
{
    _shipper.Ship(order);
}

If the Validate method returns false often, the shipper dependency is never needed.

This, he argues, can lead to inefficiencies if the dependency is costly to create. It's not a good thing to require a costly dependency if you are not going to use it in a lot of cases.

That sounds like a reasonable statement, but is it really? And is the proposed solution a good solution?

No, this isn't a reasonable statement, and the proposed solution isn't a good solution.

It would seem like there's a problem with Constructor Injection, but in reality the problem is that it is being used incorrectly and in too constrained a way.

The proposed solution is problematic because it involves tightly coupling the code to OrderShipperFactory. This is more or less a specialized application of the Service Locator anti-pattern.

Consumers of OrderProcessor have no static type information to warn them that they need to configure the OrderShipperFactory.CreationClosure static member - a completely unrelated type. This may technically work, but creates a very developer-unfriendly API. IntelliSense isn't going to be of much help here, because when you want to create an instance of OrderProcessor, it's not going to remind you that you need to statically configure OrderShipperFactory first. Enter lots of run-time exceptions.

Another issue is that he allows a concrete implementation of an interface to change the design of the OrderProcessor class - that's hardly in the spirit of the Liskov Substitution Principle. I consider this a strong design smell.

One of the commenters (Alwin) suggests instead injecting an IOrderShipperFactory. While this is a better option, it still suffers from letting a concrete implementation influence the design, but there's a better solution.

First of all we should realize that the whole case is a bit construed because although the IOrderShipper implementation may be expensive to create, there's no need to create a new instance for every OrderProcessor. Instead, we can use the so-called Singleton lifetime style where we share or reuse a single IOrderShipper instance between multiple OrderProcessor instances.

The beauty of this is that we can wait making that decision until we wire up the actual dependencies. If we have implementations of IOrderShipper that are inexpensive to create, we may still decide to create a new instance every time.

There may still be a corner case where a shared instance doesn't work for a particular implementation (perhaps because it's not thread-safe). In such cases, we can use Lazy loading to create a LazyOrderShipper like this (for clarity I've omitted making this implementation thread-safe, but that would be trivial to do):

public class LazyOrderShipper : IOrderShipper
{
    private OrderShipper shipper;
 
    #region IOrderShipper Members
 
    public void Ship(Order order)
    {
        if (this.shipper == null)
        {
            this.shipper = new OrderShipper();
        }
        this.shipper.Ship(order);
    }
 
    #endregion
}

Notice that this implementation of IOrderShipper only creates the expensive OrderShipper instance when it needs it.

Instead of directly injecting the expensive OrderShipper instance directly into OrderProcessor, we wrap it in the LazyOrderShipper class and inject that instead. The following test proves the point:

[TestMethod]
public void OrderProcessorIsFast()
{
    // Fixture setup
    var stopwatch = new Stopwatch();
    stopwatch.Start();
 
    var order = new Order();
 
    var validator = new Mock<IOrderValidator>();
    validator.Setup(v => 
        v.Validate(order)).Returns(false);
 
    var shipper = new LazyOrderShipper();
 
    var sut = new OrderProcessor(validator.Object,
        shipper);
    // Exercise system
    sut.Process(order);
    // Verify outcome
    stopwatch.Stop();
    Assert.IsTrue(stopwatch.Elapsed < 
        TimeSpan.FromMilliseconds(777));
    Console.WriteLine(stopwatch.Elapsed);
    // Teardown
}

This test is significantly faster than 777 milliseconds because the OrderShipper never comes into play. In fact, the stopwatch instance reports that the elapsed time was around 3 ms!

The bottom line is that Constructor Injection is not an anti-pattern. On the contrary, it is the most powerful DI pattern available, and you should think twice before deviating from it.


Comments

Thanks for publishing this response. When I read the post yesterday evening, I had a lot of thoughts I was chewing on, and you tackled most of them. I couldn't tell if the "smell" was that the dependency was only sometimes needed, or that the dependency was only sometimes needed, and was expensive to create.

It seemed like the need to create a set of factories and have them make calls into the IOC container directly was a bigger smell. And the fact that the OrderShipper would be comming from a factory setup somewhere else, making code maintenance more difficult, and the application more prone to errors.

Decorating the implementation with a lazy load implementation is very clever. I hadn't thought of that solution, but the singleton lifecycle had.

Well done.
2010-01-20 16:50 UTC
I like this approach, but you've also made a contrived example. Your LazyOrderShipper directly instantiates an OrderShipper. What about the dependencies that OrderShipper might require? What if those dependencies are costly? Is it lazy turtles all the way down?
2010-01-20 17:14 UTC
Constructor over-injection is the problem. I rather like constructor injection
2010-01-20 17:20 UTC
Thank you all for your comments.

The point raised by Phil Haack is valid, so I've addressed it in a second blog post.
2010-01-20 18:09 UTC
SP
Maybe I'm missing something, but why isn't LazyOrderShipper thread safe as it stands? Your shipper member variable isn't static.
2010-01-20 18:44 UTC
Why isn't LazyOrderShipper thread-safe as it is?

When I wrote that, my thinking was that if the Ship method is hit by two or more threads simultaneously, more than one thread may evaluate the member field to null and start creating a new instance of OrderShipper. At least one of those instance will created, but never permanently assigned, since another thread may overwrite the value.

In this case, it may not lead to any exceptions (that I can think of), but it's inefficient use of resources.

However, every time I post something about threading on this blog, someone will point out my errors, so I'm looking forward to learn which mistake I made this time around :)

I'm no threading expert, in case you were wondering :)
2010-01-20 19:00 UTC
SP
As yes, your right -- I was thinking the calling code could lock against the object itself, but you would never know when LazyOrderShipper was writing to the property. (I'm not threading guru by any means, either).

Ok, back to DI... :-)
2010-01-20 19:35 UTC
Thomas
Great post as well as your book
2010-01-20 20:21 UTC
This would be a good solution if the problem wasn't so contrived. I've never run into a scenario where it would take me seven seconds to instantiate an object. It might take me 7 seconds to execute a method, but never instantiate the object itself.

My point is: It is like arguing that my unicorn needs a better saddle, and that saddle will let me ride it better.

I commend you anyways for your effort.
2010-01-22 21:52 UTC
I agree, but I just couldn't let the claim that Constructor Injection is an anti-pattern go by without reacting.
2010-01-22 23:56 UTC

AutoFixture 1.0 RC1

Wednesday, 13 January 2010 22:48:13 UTC

AutoFixture 1.0 Release Candidate 1 is now available on the CodePlex site! AutoFixture is now almost ten months old and has seen extensive use internally in Safewhere during most of this time. It has proven to be very stable, expressive and generally a delight to use.

If all goes well, the Release Candidate period will be short. Key users have a chance to veto the this version, but if no-one complains within a week from now, we will promote RC1 to version 1.0.

The RC1 release page has more detail about this particular release.


Anonymous Get

Monday, 04 January 2010 20:53:24 UTC

In a previous post I described how AutoFixture's Do method can let you invoke commands on your SUT with Dummy values for the parameters you don't care about.

The Get method is the equivalent method you can use when the member you are invoking returns a value. In other words: if you want to call a method on your SUT to get a value, but you don't want the hassle of coming up with values you don't care about, you can let the Get method supply those values for you.

In today's example I will demonstrate a unit test that verifies the behavior of a custom ASP.NET MVC ModelBinder. If you don't know anything about ASP.NET MVC it doesn't really matter. The point is that a ModelBinder must implement the IModelBinder interface that defines a single method:

object BindModel(ControllerContext controllerContext,
    ModelBindingContext bindingContext);

In many cases we don't care about one or the other of these parameters, but we still need to supply them when unit testing.

The example is a bit more complex than some of my other sample code, but once in a while I like to provide you with slightly more realistic AutoFixture examples. Still, it's only 10 lines of code, but it looks like a lot more because I have wrapped several of the lines so that the code is still readable on small screens.

[TestMethod]
public void BindModelWillReturnCorrectResult()
{
    // Fixture setup
    var fixture = new Fixture();
    fixture.Customize<ControllerContext>(ob =>
        ob.OmitAutoProperties());
 
    var value = fixture.CreateAnonymous("Value");
    var bindingContext = new ModelBindingContext();
    bindingContext.ValueProvider = 
        new Dictionary<string, ValueProviderResult>();
    bindingContext.ValueProvider["MyValue"] = 
        new ValueProviderResult(value, value, 
            CultureInfo.CurrentCulture);
 
    var expectedResult = 
        new string(value.Reverse().ToArray());
    var sut = fixture.CreateAnonymous<MyModelBinder>();
    // Exercise system
    var result = fixture.Get((ControllerContext cc) =>
        sut.BindModel(cc, bindingContext));
    // Verify outcome
    Assert.AreEqual(expectedResult, result, "BindModel");
    // Teardown
}

The first part simply creates the Fixture object and customizes it to disable AutoProperties for all ControllerContext instances (otherwise we would have to set up a lot of Test Doubles for such properties as HttpContext, RequestContext etc.).

The next part of the test sets up a ModelBindingContext instance that will be used to exercise the SUT. In this test case, the bindingContext parameter of the BindModel method is important, so I explicitly set that up. On the other hand, I don't care about the controllerContext parameter in this test case, so I ask the Get method to take care of that for me:

var result = fixture.Get((ControllerContext cc) =>
    sut.BindModel(cc, bindingContext));

The Get method creates a Dummy value for the ControllerContext, whereas I can still use the outer variable bindingContext to call the BindModel method. The return value of the BindModel method is returned to the result variable by the Get method.

Like the Do methods, the Get methods are generic methods. The one invoked in this example has this signature:

public TResult Get<T, TResult>(Func<T, TResult> function);

There are also overloads that works with the versions of the Func delegate that takes two, three and four parameters.

As the Do methods, the Get methods are convenience methods that let you concentrate on writing the test code you care about while it takes care of all the rest. You could have written a slightly more complex version that didn't use Get but instead used the CreateAnonymous method to create an Anonymous Variable for the ControllerContext, but this way is slightly more succinct.


MEF TechTalk with me

Sunday, 20 December 2009 19:56:33 UTC

I'll be doing a TechTalk on the Managed Extensibility Framework and Dependency Injection at Microsoft Denmark January 20th 2010.

The talk will be in Danish. Details and sign-up here.


Wiring ASP.NET MVC Error Handling with Castle Windsor

Monday, 14 December 2009 06:59:32 UTC

In my previous posts I discussed how to enable global error handling in ASP.NET MVC and how to inject a logging interface into the error handler. In these posts, I simplified things a bit to get my points across.

In production we don't use a custom ErrorHandlingControllerFactory to configure all Controllers with error handling, nor do we instantiate IExceptionFilters manually. What I described was the Poor Man's Dependency Injection (DI) approach, which I find most appropriate when describing DI concepts.

However, we really use Castle Windsor currently, so the wiring looks a bit different although it's still exactly the same thing that's going on. Neither ErrorHandlingActionInvoker nor LoggingExceptionFilter are any different than I have already described, but for completeness I wanted to share a bit of our Windsor code.

This is how we really wire our Controllers:

container.Register(AllTypes
    .FromAssemblyContaining(representativeControllerType)
    .BasedOn<Controller>()
    .ConfigureFor<Controller>(reg => 
        reg.LifeStyle.PerWebRequest.ServiceOverrides(
            new { ActionInvoker = typeof(
                ErrorHandlingControllerActionInvoker)
                .FullName })));

Most of this statement simply instructs Windsor to scan all types in the specified assembly for Controller implementations and register them. The interesting part is the ServiceOverrides method call that uses Windsor's rather excentric syntax for defining that the ActionInvoker property should be wired up with an instance of the component registered as ErrorHandlingControllerActionInvoker.

Since ErrorHandlingControllerActionInvoker itself expects an IExceptionFilter instance we need to configure at least one of these as well. Instead of one, however, we have two:

container.Register(Component.For<IExceptionFilter>()
    .ImplementedBy<LoggingExceptionFilter>());
container.Register(Component.For<IExceptionFilter>()
    .ImplementedBy<HandleErrorAttribute>());

This is Windsor's elegant way of registering Decorators: you simply register the Decorator before the decorated type, and it'll Auto-Wire everything for you.

Finally, we use a ControllerFactory very similar to the WindsorControllerFactory from the MVC Contrib project.

To reiterate: You don't have to use Castle Windsor to enable global error handling or logging in ASP.NET MVC. You can code it by hand as I've demonstrated in my previous posts, or you can use some other DI Container. The purpose of this post was simply to demonstrate one way to take it to the next level.


LoggingExceptionFilter

Monday, 07 December 2009 06:20:27 UTC

In a previous post I described how to enable global error handling in ASP.NET MVC applications. Although I spent some time talking about the importance of DRY, another major motivation for me was to enable Dependency Injection (DI) with exception handling so that I could log stack traces before letting ASP.NET MVC handle the exception.

With the ErrorHandlingControllerActionInvoker I described, we can inject any IExceptionFilter implementation. As an example I used HandleErrorAttribute, but obviously that doesn't log anything. Once again, it would be tempting to derive from HandleErrorAttribute and override its OnException method, but staying true to the Single Responsibility Principle as well as the Open/Closed Principle I rather prefer a Decorator:

public class LoggingExceptionFilter : IExceptionFilter
{
    private readonly IExceptionFilter filter;
    private readonly ILogWriter logWriter;
 
    public LoggingExceptionFilter(IExceptionFilter filter,
        ILogWriter logWriter)
    {
        if (filter == null)
        {
            throw new ArgumentNullException("filter");
        }
        if (logWriter == null)
        {
            throw new ArgumentNullException("logWriter");
        }        
 
        this.filter = filter;
        this.logWriter = logWriter;
    }
 
    #region IExceptionFilter Members
 
    public void OnException(ExceptionContext filterContext)
    {
        this.logWriter.WriteError(filterContext.Exception);
        this.filter.OnException(filterContext);
    }
 
    #endregion
}

The LoggingExceptionFilter shown above is unabridged production code. This is all it takes to bridge the gap between IExceptionFilter and some ILogWriter interface (replace with the logging framework of your choice). Notice how it simply logs the error and then passes on exception handling to the decorated IExceptionFilter.

Currently we use HandleErrorAttribute as the decorated filter so that behavior stays as expected.

c.ActionInvoker = 
    new ErrorHandlingControllerActionInvoker(
        new LoggingExceptionFilter(
            new HandleErrorAttribute(), logWriter));

This is not too different from before, except that a LoggingExceptionFilter now decorates the HandleErrorAttribute instance.


Building and assigning arrays with AutoFixture

Saturday, 05 December 2009 00:41:45 UTC

A reader asked me how AutoFixture can deal with arrays as fields on a class. More specifically, given a class like MyClassA, how can AutoFixture assign a proper array with initialized values to Items?

public class MyClassA
{
    public MyClassB[] Items;
    public MyClassC C;
    public MyClassD D;
}

Ignoring the bad practice of publicly exposing fields, the main problem is that AutoFixture has no inherent understanding of arrays, so if we try to create a new instance of MyClassA by invoking the CreateAnonymous method, we would end up with Items being an array of nulls.

Obviously we want a populated array instead. There are at least a couple of ways to reach this goal.

The simplest is just to create it and modify it afterwards:

var mc = fixture.CreateAnonymous<MyClassA>();
mc.Items = fixture.CreateMany<MyClassB>().ToArray();

Although the CreateAnomymous method will assign an unitialized array, we immediately overwrite the value with an initialized array. The CreateMany method returns an IEnumerable<MyClassB> on which we can use the ToArray extension method to create an array.

The next option is to do almost the same thing, but as a single operation:

var mc = fixture.Build<MyClassA>()
    .With(x => x.Items, 
        fixture.CreateMany<MyClassB>().ToArray())
    .CreateAnonymous();

Besides the different syntax and the lower semicolon count, the biggest difference is that in this case the Items field is never assigned an unitialized array because the With method ensures that the specified value is assigned immediately.

If you get tired of writing this Builder sequence every time you want to create a new MyClassA instance, you can Customize the Fixture:

fixture.Customize<MyClassA>(ob =>
    ob.With(x => x.Items, 
        fixture.CreateMany<MyClassB>().ToArray()));

With this customization, every time we invoke

var mc = fixture.CreateAnonymous<MyClassA>();

we will get an instance of MyClassA with a properly initialized Items array.

I hope you find one or more of these methods useful.


Comments

Murali
Thanks Mark for your very quick answer to my question.

Murali
2009-12-07 14:33 UTC

Global Error Handling in ASP.NET MVC

Tuesday, 01 December 2009 06:37:01 UTC

ASP.NET MVC comes with an error-handling feature that enables you to show nice error Views to your users instead of the dreaded Yellow Screen of Death. The most prominently described technique involves attributing either you Controller or a single Controller action, like we see in the AccountController created by the Visual Studio project template.

[HandleError]
public class AccountController : Controller { }

That sure is easy, but I don't like it for a number of reasons:

  • Even though you can derive from HandleErrorAttribute, it's impossible to inject any dependencies into Attributes because they must be fully constructed at compile-time. That makes it really difficult to log errors to an interface.
  • It violates the DRY principle. Although it can be applied at the Controller level, I still must remember to attribute all of my Controllers with the HandleErrorAttribute.

Another approach is to override Controller.OnException. That solves the DI problem, but not the DRY problem.

Attentive readers may now point out that I can define a base Controller that implements the proper error handling, and require that all my Controllers derive from this base Controller, but that doesn't satisfy me:

First of all, it still violates the DRY principle. Whether I have to remember to apply a [HandleError] attribute or derive from a : MyBaseController amounts to the same thing. I (and all my team members) have to remember this always. It's unnecessary project friction.

The second thing that bugs me about this approach is that I really do favor composition over inheritance. Error handling is a cross-cutting concern, so why should all my Controllers have to derive from a base controller to enable this? That is absurd.

Fortunately, ASP.NET MVC offers a third way.

The key lies in the Controller base class and how it deals with IExceptionFilters (which HandleErrorAttribute implements). When a Controller action is invoked, this actually happens through an IActionInvoker. The default ControllerActionInvoker is responsible for gathering the list of IExceptionFilters, so the first thing we can do is to derive from that and override its GetFilters method:

public class ErrorHandlingActionInvoker :
    ControllerActionInvoker
{
    private readonly IExceptionFilter filter;
 
    public ErrorHandlingActionInvoker(
        IExceptionFilter filter)
    {
        if (filter == null)
        {
            throw new ArgumentNullException("filter");
        }
 
        this.filter = filter;
    }
 
    protected override FilterInfo GetFilters(
        ControllerContext controllerContext,
        ActionDescriptor actionDescriptor)
    {
        var filterInfo = 
            base.GetFilters(controllerContext, 
            actionDescriptor);
        filterInfo.ExceptionFilters.Add(this.filter);
        return filterInfo;
    }
}

Here I'm simply injecting an IExceptionFilter instance into this class, so I'm not tightly binding it to any particular implementation - it will work with any IExceptionFilter we give it, so it simply serves the purpose of adding the filter at the right stage so that it can deal with otherwise unhandled exceptions. It's pure infrastructure code.

Notice that I first invoke base.GetFilters and then add the injected filter to the returned list of filters. This allows the normal ASP.NET MVC IExceptionFilters to attempt to deal with the error first.

This ErrorHandlingActionInvoker is only valuable if we can assign it to each and every Controller in the application. This can be done using a custom ControllerFactory:

public class ErrorHandlingControllerFactory : 
    DefaultControllerFactory
{
    public override IController CreateController(
        RequestContext requestContext,
        string controllerName)
    {
        var controller = 
            base.CreateController(requestContext,
            controllerName);
 
        var c = controller as Controller;
        if (c != null)
        {
            c.ActionInvoker = 
                new ErrorHandlingActionInvoker(
                    new HandleErrorAttribute());
        }
 
        return controller;
    }
}

Notice that I'm simply deriving from the DefaultControllerFactory and overriding the CreateController method to assign the ErrorHandlingActionInvoker to the created Controller.

In this example, I have chosen to inject the HandleErrorAttribute into the ErrorHandlingActionInvoker instance. This seems weird, but it's the HandleErrorAttribute that implements the default error handling logic in ASP.NET MVC, and since it implements IExceptionFilter, it works like a charm.

The only thing left to do is to wire up the custom ErrorHandlingControllerFactory in global.asax like this:

ControllerBuilder.Current.SetControllerFactory(
    new ErrorHandlingControllerFactory());

Now any Controller action that throws an exception is gracefully handled by the injected IExceptionFilter. Since the filter is added as the last in the list of ExceptionFilters, any normal [HandleError] attribute and Controller.OnException override gets a chance to deal with the exception first, so this doesn't even change behavior of existing code.

If you are trying this out on your own development machine, remember that you must modify your web.config like so:

<customErrors mode="On" />

If you run in the RemoteOnly mode, you will still see the Yellow Screen of Death on your local box.


Comments

Very nice article..
2009-12-04 16:25 UTC
Excellent! I like this for the same reason of DRY as well! I've implemented this in our project now as well!

One question, just to confirm your dependency injection concern: During your CreateController() override, I would have to obtain an instance of ErrorHandlingActionInvoker() from my IoC container, right?

Feels kind of dirty doing it there. Well, it would feel even dirtier if i had to reference a property from the controller to get an instance of what I want.

Just wondering what you ended up with for your dependency injection pattern of this error handler.

2010-01-30 00:23 UTC
You are correct that in our production applications, we don't manually request the ErrorHandlingActionInvoker from the container and assign it to the property. Rather, we simply configure the container to automatically set the ActionInvoker property as part of wiring up any Controller.

This follow-up post provides an example.
2010-01-30 09:27 UTC
I've tried extending the view model to have some of my custom data needed by master page and I keep getting empty result. Any ideas?
2010-03-15 21:42 UTC
I'm not sure I understand how your question relates particularly to this blog post, so I'll need more details than that to be able to say anything meaningful.

May I suggest asking your question on Stack Overflow? That site has good facilities for asking development-related questions, including syntax highlighting. You are welcome to point me to any questions you may have, and I'll take a look and answer if I have anything to add.
2010-03-15 22:17 UTC
petrux
Great post!!! I was wondering... is it possible to handle the authentication issue the same way? I mean: is it possible to handle it dealing with controllers? Or is it a bad practice and shall I deal with this stuff in the service/busines layer?

Bye bye and... sorry for my broken English! :-)
petrux from Italy
2011-03-01 08:56 UTC
RonJ
Mark, Your code works up to a point. When an exception occurs in a controller's HttpGet method the method SystemError is reached. However, when an exception occurs in a controller's HttpPost method the exception "A public action method 'SystemError' was not found on controller 'Cmrs_Web.Controllers.CalmErrorController'." is raised on the line of code "httpHandler.ProcessRequest(this.Context);" in global.asa.cs

Here is the global.asa.cs code:
protected void Application_Error(object sender, EventArgs e)
{
// Log the exception to CALM
Guid errorLogNumber = new Guid();
var error = Server.GetLastError();
string logMessage = Calm.Logging.Utilities.FormatException(error);
Response.Clear();
Server.ClearError();
if (Logger.IsFatalOn)
{
errorLogNumber = Logger.Fatal(error);
}

// Put CALM Error Id into session so CalmError controller can get it for displaying in view
Session.Add("CalmErrorId", errorLogNumber.ToString());

// Display Error View
string path = Request.Path;
this.Context.RewritePath("~/CalmError/SystemError");
IHttpHandler httpHandler = new MvcHttpHandler();
httpHandler.ProcessRequest(this.Context);
this.Context.RewritePath(path, false);
}

Here is simplified code for the Controller (device is a simple object with 6 properties - forceErr I manually set to false/true then ran a Debug session)
[HttpGet]
public ActionResult CreateManualAsset()
{
bool forceErr = false;
if (forceErr)
{
throw new Exception("forced error in CreateManualAsset post");
}
return this.View("Manual Asset");
}

[HttpPost]
public ActionResult CreateManualAsset(Device device)
{
bool forceErr = false;
if (forceErr)
{
throw new Exception("forced error in CreateManualAsset post");
}
return this.View("Manual Asset");
}
2012-07-27 18:53 UTC

Page 32 of 38

"Our team wholeheartedly endorses Mark. His expert service provides tremendous value."
Hire me!