Error Handling - Controller's OnException and Application_Error

Question

I'm pretty new to MVC. Below is the code that I have added to an existing MVC 5 application. The code is using log4net to log any run-time error/exception.

NOTE: I could override OnException() in MVC filter "HandleErrorAttribute" but I preferred to have this functionality in my custom Controller base class.

Can anyone please review and provide feedback?

BaseController.cs

public class BaseController: Controller
{
 private log4net.ILog logger;
 protected override void OnException(ExceptionContext filterContext)
 {
 //Log error
 logger = log4net.LogManager.GetLogger(filterContext.Controller.ToString());
 logger.Error(filterContext.Exception.Message, filterContext.Exception);
 //If the request is AJAX return JSON else redirect user to Error view.
 if (filterContext.HttpContext.Request.Headers["X-Requested-With"] == "XMLHttpRequest")
 {
 //Return JSON
 filterContext.Result = new JsonResult
 {
 JsonRequestBehavior = JsonRequestBehavior.AllowGet,
 Data = new { error = true, message = "Sorry, an error occurred while processing your request." }
 };
 }
 else
 {
 //Redirect user to error page
 filterContext.ExceptionHandled = true;
 filterContext.Result = this.RedirectToAction("Index", "Error");
 }
 base.OnException(filterContext);
 }
}

HomeController.cs

public class HomeController : BaseController
{
 ...
}

Global.asax.cs

public class MvcApplication : System.Web.HttpApplication
{
 private static readonly ILog log = LogManager.GetLogger(typeof(MvcApplication));
 protected void Application_Error(object sender, EventArgs e)
 {
 //Log exception
 Exception exception = Server.GetLastError();
 log.Error(exception);
 //Clear error from response stream
 Response.Clear();
 Server.ClearError();
 //Redirect user
 Context.Server.TransferRequest("~/Error/");
 }
 ...
}

For displaying errors, I have a basic ErrorController.cs with just Index() Action along with its associated Error/Index.cshtml.

Some blogs suggests different error pages for different type of errors (such as 404 error). Do I need to handle them separately?

Please share if you have any ideas to improve upon my code in terms of usability and logging details.

Answer 1

The OnException() methods does 2 things.

1. It logs the exception
2. It creates an return value

So let us refactor these two points to two separate methods.

private void LogException(ExceptionContext exceptionContext)
{
 logger = log4net.LogManager.GetLogger(exceptionContext.Controller.ToString());
 logger.Error(exceptionContext.Exception.Message, exceptionContext.Exception);
}

---

private const String XMLHttpRequest = "XMLHttpRequest";
private const String XRequestedWithHeadername= "X-Requested-With";
private const String JSONErrorMessage = "Sorry, an error occurred while processing your request.";
private void CreateExceptionContextResult(ExceptionContext exceptionContext)
{
 if (exceptionContext.HttpContext.Request.Headers[XRequestedWithHeadername] == XMLHttpRequest )
 {
 //Return JSON
 exceptionContext.Result = new JsonResult
 {
 JsonRequestBehavior = JsonRequestBehavior.AllowGet,
 Data = new { error = true, message = JSONErrorMessage }
 };
 }
 else
 {
 //Redirect user to error page
 exceptionContext.ExceptionHandled = true;
 exceptionContext.Result = this.RedirectToAction("Index", "Error");
 }
}

If we want to handle the different error types, we should create a new class which only keeps the values for actionName and controllerName.

public class ActionControllerName
{
 public String ActionName { get; private set; }
 public String ControllerName { get; private set; }
 public ActionControllerName(String actionName, String controllerName)
 {
 ActionName = actionName;
 ControllerName = controllerName;
 }
} 

Now we add a Dictionary and a method to fill this dictionary, which should be called in the constructor, or injected by an property.

private const int DefaultEntryKey = -1;
private Dictionary<int,ActionControllerName> redirectDictionary = new Dictionary<int,ActionControllerName>();
private void FillRedirectDictionary()
{
 redirectDictionary.Add(DefaultEntryKey , new ActionControllerName("Index", "Error"));
 // more to add
} 

Next we implement a method to get a RedirectToRouteResult passing the exception as parameter

private RedirectToRouteResult GetRedirect(Exception exception)
{
 int errorCode = DefaultEntryKey;
 HttpException httpException = exception as HttpException;
 if (httpException != null) 
 {
 errorCode = httpException.ErrorCode;
 if (!redirectDictionary.ContainsKey(errorCode))
 {
 errorCode = DefaultEntryKey;
 }
 }
 ActionControllerName acn = redirectDictionary[errorCode];
 return this.RedirectToAction(acn.ActionName , acn.ControllerName);
}

We go back to this else part

else
{
 //Redirect user to error page
 exceptionContext.ExceptionHandled = true;
 exceptionContext.Result = this.RedirectToAction("Index", "Error");
} 

and change it to

else
{
 //Redirect user to error page
 exceptionContext.ExceptionHandled = true;
 exceptionContext.Result = GetRedirect(exceptionContext.Exception);
}

• Riz: @heslacher this creates a major problem that I'm having to deal with now: it just binds the original url to the error redirect url forever even after you get rid of the exception : stackoverflow.com/questions/40143304/…