Web Api 2 - Returning NotFound(); vs throwing an exception with global exception handler

Question

I'm working on an API being developed with .net Web Api 2. I've seen many blog posts and SO questions about Web Api version 1, but answers using the changes made in version 2 seem to be scarce by comparison.

Compare these two ways of handling 'errors' in a controller ItemsController

A. Using methods that create objects from System.Web.Http.Results

// GET api/user/userID/item/itemID
[Route("{itemID:int}", Name="GetItem")]
[ResponseType(typeof(ItemDTO))]
public IHttpActionResult Get(int userID, int itemID)
{
    if (userID < 0 || itemID < 0) return BadRequest("Provided user id or item id is not valid");
    ItemDTO item = _repository.GetItem(itemID);

    if (item == null) return NotFound();

    if (item.UserID != userID) return BadRequest("Item userID does not match route userID");

    return Ok<ItemDTO>(item);
}

B. Throwing exceptions that can be caught by registering a custom Global Exception Handler

// ex) in WebApiConfig.cs
// config.Services.Replace(typeof(IExceptionHandler), new GlobalExceptionHandler());
public class GlobalExceptionHandler : ExceptionHandler
{
    public override void Handle(ExceptionHandlerContext context)
    {
        Exception exception = context.Exception;

        HttpException httpException = exception as HttpException;

        if (httpException != null)
        {
            context.Result = new SimpleErrorResult(context.Request, (HttpStatusCode)httpException.GetHttpCode(), httpException.Message);
            return;
        }

        if (exception is RootObjectNotFoundException)
        {
            context.Result = new SimpleErrorResult(context.Request, HttpStatusCode.NotFound, exception.Message);
            return;
        }

        if (exception is BadRouteParametersException || exception is RouteObjectPropertyMismatchException)
        {
            context.Result = new SimpleErrorResult(context.Request, HttpStatusCode.BadRequest, exception.Message);
            return;
        }

        if (exception is BusinessRuleViolationException)
        {
            context.Result = new SimpleErrorResult(context.Request, (HttpStatusCode)422, exception.Message);
            return;
        }

        context.Result = new SimpleErrorResult(context.Request, HttpStatusCode.InternalServerError, exception.Message);
    }
}

GET api/user/userID/item/itemID
[Route("{itemID:int}", Name="GetItem")]
[ResponseType(typeof(ItemDTO))]
public IHttpActionResult Get(int userID, int itemID)
{
    if (userID < 0 || itemID < 0)
        throw new BadRouteParametersException("Provided user or item ID is not valid");
    ItemDTO item = _repository.GetItem(itemID);

    if (item.UserID != userID)
        throw new RouteObjectPropertyMismatchException("Item userID does not match route userID");

    return Ok<ItemDTO>(item);
}

Both of these seem like valid options. Since I am able to return System.Web.Http.Results objects it seems like solution A. is the best one.

But consider when in my _repository my GetItem method is implemented like so

public ItemDTO GetItem(int itemId)
{
    ItemInfo itemInfo = ItemInfoProvider.GetItemInfo(itemId);

    if (itemInfo == null) throw new RootObjectNotFoundException("Item not found");

    ItemDTO item = _autoMapper.Map<ItemDTO>(itemInfo);
    return item;
}

Here I can skip calling the autoMapper on null in GetItem and also skip checking for null in the controller.

Questions

• Which way makes more sense?
• Should I attempt a combination of A & B?
• Should I try to keep my Controllers thin or should this type of validation & processing logic be kept there since I have access to the NotFound() and BadRequest() methods?
• Should I be performing this type of logic somewhere else in the framework pipeline?

I realize my question is more architectural rather than 'how do i use this feature' but again, I haven't found too many explanations of how and when to use these different features.

Answer 1

From my standpoint, a global exception handler makes unit testing each action easier (read: more legible). You're now checking against a specific [expected] exception versus (essentially) comparing status codes. (404 vs. 500 vs. etc.) It also makes changes/logging of error notifications (at a global/unified level) much easier as you have a single unit of responsibility.

For instance, which unit test do you prefer to write?

[Test]
public void Id_must_not_be_less_than_zero()
{
    var fooController = new FooController();

    var actual = fooController.Get(-1);

    Assert.IsInstanceOfType(actual, typeof(BadRequestResult));
}

[Test]
[ExpectedException(typeof(BadRouteParametersException))]
public void Id_must_not_be_less_than_zero()
{
    var fooController = new FooController();

    var actual = fooController.Get(-1);
}

Generally speaking, I would say this is more a preference than a hard-and-fast rule, and you should go with whatever you find to be the most maintainable and easiest to understand from both an on-boarding perspective (new eyes working on the project) and/or later maintenance by yourself.

Answer 2

As Brad notes, this partly comes down to preference.

Using HTTP codes is consistent with how the web works, so it's the way I lean.

The other consideration is that throwing exceptions has a cost. If you're OK with paying that cost, and take that into account in your design, it's fine to make that choice. Just be aware of it, particularly when you're using exceptions for situations that aren't really exceptional but rather are things you know you may encounter as part of normal application flow.

It's an older post, but there's an interesting discussion on the topic of exceptions and performance here:

http://blogs.msdn.com/b/ricom/archive/2006/09/14/754661.aspx

and the follow-up:

http://blogs.msdn.com/b/ricom/archive/2006/09/25/the-true-cost-of-net-exceptions-solution.aspx