1

The system is in Asp.Net MVC 4, C#.

The exception if being thrown before the controller method is executed. I don't know how to handle it - I'd like to redirect the user to an error page but I couldn't.

  • I have a base ViewModel class that contains a SelectList to be used in a dropdown. In its constructor, the ViewModel gets its SelectListItems from the database. This is the source of the exception.

  • The index method takes the viewmodel as a parameter.

  • Here it's a sketch of the code:

    class MyViewModel{
  public SelectList SelectListModel { get; set; }
  public MyViewModel()
  {
      List<X> xs = GetItemsFromDB(); // <= Exception thrown here
      List<SelectListItem> SelectListContent = new List<SelectListItem>();
      foreach(X x in xs)
      {
           SelectListContent.Add(new SelectListItem( Value = x.value,Text=x.text); 
      } 
      SelectListModel = new SelectList(SelectListContent , "Value", "Text"); 
  }  
}
 public class MyController : Controller
{

   public ActionResult Index(MyViewModel model) //<< Exception thrown before entering method
   { 
    //do something
   }
}

I've tried to put a try-catch inside the contructor with the following code in the catch: 

            var context = new HttpContextWrapper(HttpContext.Current);
            var rc = new RequestContext(context, new RouteData());
            var urlHelper = new UrlHelper(rc);
            context.Response.Redirect(urlHelper.Action("Index", "Error", new { messagem = x.Message }), false);
            HttpContext.Current.ApplicationInstance.CompleteRequest();

I took this from other SO answers, but its not working. When this block is executed the user is not redirected to the error page. Instead, MyControllers Index method continues its execution.

galmeida
  • 221
  • 2
  • 10
  • 1
    You should not have a model as a parameter in a GET method. And why in the world are you creating one `SelectList` and then creating another duplicate one from it. And a view model should never have code that accesses a database. –  Dec 10 '15 at 12:16
  • What's the exception? – n1FF Dec 10 '15 at 12:32
  • to Stephen: but now that I have this problem, do you know a workaround? to Niff: The exception is a failure in opeining the database ("Cannot open database etrc"). – galmeida Dec 10 '15 at 12:52
  • 1
    @galmeida, You problem is that everything your doing here is wrong. A view model is a dumb class containing properties only and should never access your data base - you can never even unit test this code. It is the responsibility of the controller to call your service and populate the view model. And a GET method should not have your model as a parameter - you initialize the model in the controller –  Dec 10 '15 at 23:31
  • @StephenMuecke Thanks for the comment. Would you point some guidelines for using ViewModels, please? I've put this way because many pages use the same dropdown, so this viewmodel is a base class for other viewmodels (and I've simplified the code here, it has 6 fields, including arrays and an instance of another class). Would it be bad to move the 'populate' to a method on that viewmodel or to a base controller? – galmeida Dec 11 '15 at 11:49

2 Answers2

4

The best approach to catch this is to create an ExceptionFilter 

public class CustomExceptionFilter : IExceptionFilter
{    
        public void OnException(ExceptionContext filterContext)
        {

            if (filterContext.ExceptionHandled)
                return;    

            //Do yout logic here
        }
}

and register it global, in the RegisterGlobalFilters from FilterConfig.cs

public static void RegisterGlobalFilters(GlobalFilterCollection filters)
{
    filters.Add(new CustomExceptionFilter());
}
Pedro Benevides
  • 1,970
  • 18
  • 19
1

While you could use an ExceptionFilter, its is unnecessary. The real problem here is you incorrect use of a view model. A view model should contain only properties your need for display/edit in the view and should not access the database. Two reasons for this are

  1. You can not unit test the model or any component of your app, including the controller, that uses the model. Even though its clear your not yet into unit testing, you should at least design for it (I guarantee that once you do, you it will become an integral part of your development).
  2. Because your will be posting back your view model, it means that the DefaultModelBinder will initialize the model and call its constructor which in turn calls the database to populate your SelectList. The only reason you should need the SelectList in the POST method is because ModelState is invalid and you need to return the view. If client side validation is enabled, this would be rare, so you unnecessarily degrading performance by making database calls for data that wont be used.

Suggest you read the answers in What is ViewModel in MVC?

Next, your GET method should not contain a parameter for you model. Two reasons for this are

  1. The DefaultModelBinder is initializing your model and it adds the values of your model properties to ModelState and if your properties contain any validation attributes, then ModelState will be invalid. The side affect will be that any validation errors will be displayed in the initial view, and any attempt to set the value of your properties in the GET method will be ignored by HtmlHelper methods because they use the values from ModelState in preference to the model properties. To overcome this, you would need to use the ModelState.Clear() hack, effectively undoing what the ModelBinder has just done. Again its just pointless extra overhead.
  2. Because you cannot have the same signature for the GET and POST method, you need to rename the POST method and use the overload of BeginForm() that specifies the action method name.

Instead, you should be initializing an instance of the view model inside the GET method.

Finally the code in your models constructor to generate the SelectList is generating one IEnumerable<SelectListItem> and then creating a second identical IEnumerable<SelectListItem> from the first one (again its just unnecessary extra overhead).

From your comments, you have indicated that this will be a base view model, so I would suggest that you have a BaseController with the following method

protected void ConfigureBaseViewModel(BaseVM model)
{
  List<X> xs = GetItemsFromDB();
  model.SelectListModel = new SelectList(xs, "value", "text");
  // or model.SelectListModel = xs.Select(x => new SelectListItem{ Value = x.value, Text=x.text });
}

where BaseVM is

public abstract class BaseVM
{
  [Required(ErrorMessage = "Please select an item")] // add other display and validation attributes as necessary
  public int SelectedItem { get; set; } // or string?
  public IEnumerable<SelectListItem> SelectListModel { get; set; }
  .... // other common properties
}

and then in the concrete controller

public ActionResult Index()
{
  var model = new yourConcreteModel();
  ConfigureBaseViewModel(model);
  return View(model);
}
[HttpPost]
public ActionResult Index(yourConcreteModel model)
{
  if (!ModelState.IsValid)
  {
    ConfigureBaseViewModel(model);
    return View(model);
  }
  // save and redirect
}

Similarly you might have aprivate void ConfigureConcreteViewModel(yourConcreteModel model) method in each concrete controller that assigns common values such as SelectLists that are needed in the GET method and POST method if the view needs to be returned.

Community
  • 1
  • 1