Exception handling in Controller (ASP.NET MVC)

Question

When an exception is thrown by your own code that's called from an action in a controller how should that be handled? I see a lot of examples of best practices where there are no try-catch statements at all. For example, accessing data from a repository:

public ViewResult Index()
{
    IList<CustomModel> customModels = _customModelRepository.GetAll();
    return View(customModels);
}

Clearly this code could throw an exception if the call is to a database that it can't access and we are using an ORM like Entity Framework for example.

However all that I can see will happen is that the exception will bubble up and show a nasty error message to the user.

I'm aware of the HandleError attribute but I understand it's mostly used to redirect you to an error page if an exception that's unhandled occurs.

Of course, this code could be wrapped in a try-catch but doesn't separate nicely, especially if you have more logic:

public ViewResult Index()
{
    if (ValidationCheck())
    {
        IList<CustomModel> customModels = new List<CustomModel>();
        try
        {
            customModels = _customModelRepository.GetAll();
        }
        catch (SqlException ex)
        {
            // Handle exception
        }

        if (CustomModelsAreValid(customModels))
            // Do something
        else
            // Do something else
    }

    return View();
}

Previously I have extracted out all code that could throw exceptions like database calls into a DataProvider class which handles errors and returns messages back for showing messages to the user.

I was wondering what the best way of handling this is? I don't always want to return to an error page because some exceptions shouldn't do that. Instead, an error message to the user should be displayed with a normal view. Was my previous method correct or is there a better solution?

Solution 1

I do three things to display more user-friendly messages:

1. Take advantage of the global exception handler. In the case of MVC: Application_Error in Global.asax. Learn how to use it here: http://msdn.microsoft.com/en-us/library/24395wz3(v=vs.100).aspx
2. I subclass Exception into a UserFriendlyException. I do my very best in all of my underlying service classes to throw this UserFriendlyException instead of a plain old Exception. I always try to put user-meaningful messages in these custom exceptions. The main purpose of which is to be able to do a type check on the exception in the Application_Error method. For the UserFriendlyExceptions, I just use the user-friendly message that I've set deep down in my services, like "Hey! 91 degrees is not a valid latitude value!". If it's a regular exception, then it's some case I haven't handled, so I display a more generic error message, like "Oops, something went wrong! We'll do our best to get that fixed!".
3. I also create an ErrorController that is responsible for rendering user-friendly views or JSON. This is the controller whose methods will be called from the Application_Error method.

EDIT: I thought I'd give a mention to ASP.NET Web API since it's closely related. Because the consumer of Web API endpoints won't necessarily be a browser, I like to deal with errors a little differently. I still use the "FriendlyException" (#2 above), but instead of redirecting to an ErrorController, I just let all my endpoints return some kind of base type that contains an Error property. So, if an exception bubbles all the way up to the Web API controllers, I make sure to stick that error in the Error property of API response. This error message will either be the friendly message that has bubbled up from the classes the API controller relies on, or it will be a generic message if the exception type is not a FriendlyException. That way, the consuming client can simply check whether or not the Error property of the API response is empty. Display a message if the error is present, proceed as usual if not. The nice thing is that, because of the friendly message concept, the message may be much more meaningful to the user than a generic "Error!" message. I use this strategy when writing mobile apps with Xamarin, where I can share my C# types between my web services and my iOS/Android app.

Solution 2

With Asp.Net MVC you can also override the OnException method for you controller.

protected override void OnException(ExceptionContext filterContext)
{
    if (filterContext.ExceptionHandled)
    {
        return;
    }
    filterContext.Result = new ViewResult
    {
        ViewName = ...
    };
    filterContext.ExceptionHandled = true;
}

This allow you to redirect to a custom error page with a message that refer to the exception if you want to.

Solution 3

I used an OnException override because I have several projects referenes to one that have a Controller that handle errors:

Security/HandleErrorsController.cs

protected override void OnException(ExceptionContext filterContext) 
{
    MyLogger.Error(filterContext.Exception);   //method for log in EventViewer

    if (filterContext.ExceptionHandled)
        return;

    filterContext.HttpContext.Response.StatusCode = (int)System.Net.HttpStatusCode.InternalServerError;

    filterContext.Result = new JsonResult
    {
        Data = new
        {
            Success = false, 
            Error = "Please report to admin.",
            ErrorText = filterContext.Exception.Message,
            Stack = filterContext.Exception.StackTrace
        },
        JsonRequestBehavior = JsonRequestBehavior.AllowGet
    };
    filterContext.ExceptionHandled = true;
}

Comments

[Askolein]

May downvoters argue a little: what is wrong with this answer?

[Patrick Desjardins]

@Askolein I do not understand either. You can up vote to set it back to 0 if you believe that it is a possible solution like I do.

[NovaJoe]

I also agree with Patrick Desjardins' answer: overriding OnException is a great way to handle errors. Especially if it's in a base controller that all your other controllers inherit from.

[NovaJoe]

I agree that this is a great answer. Override OnException in a custom base controller, and make all your other controllers inherit from it. Combine this with the concept of a user-friendly-exception and you'll have a good solution.

[Justin Skiles]

It's a really bad idea to return raw exception contents like messages and stack traces to the user.

[Jack]

What about this problem similar to this issue?

[EamonnM]

One warning - OnException doesn't catch all exceptions raised by the controller (shame), only those raised in Actions and some action related methods. So exceptions in controller.initialize() will not be caught, but exceptions in controller.OnActionExecuting() will.

[CervEd]

It's bad practice to return exceptions to users, mainly from a security perspective