MVC Bug with Default DropDownList or ListBox HTML Helpers

There are a lot of examples out there on how to pass a SelectList or MultiSelectList object to an Html.DropDownList or Html.ListBox helper.  However, I couldn’t find anything that was explaining the problem I was having.

In my app we have a Show object that represents a podcast.  Each show may have multiple Guest objects, which are accessed by the Show.Guests member.

In the Admin controller I have the following Action method:

public ActionResult EditShow(int id)
{
var show = _repository.Get<Show>(id);
show.Guests.Load();
var guestList = new MultiSelectList(_repository.List<Guest>().OrderBy(g => g.Name), "Id", "Name", show.Guests.Select(g => g.Id));
ViewData["GuestsSelectList"] = guestList;
return View(show);
}

In the Edit View I display the fields for the Show and a ListBox for the Guests.  You can select multiple Guests for a show.  Ideally, when you edit a Show the Guests that are already associated with the Show are pre-selected.  Makes sense, right?

Here is the code called in the View:

<%= Html.ListBox(" GuestListBox ", (MultiSelectList)ViewData[" GuestsSelectList "])%>

If you follow this you’ll notice that I am adding to the ViewData a MultiSelectList object, which contains an IEnumerable collection of all the Guests, that “Id” is the value field, “Name” is the text field, and an IEnumerable collection of the guest.Id values that are already associated with the show.  However, this never caused the ListBox to select the values upon rendering the page.

I used the debugger and, sure enough, my MultiSelectList object was successfully populated with the properly selected items.  What was the deal?  The breakdown must be happening with the Html.ListBox helper.  I spent a couple of hours scouring this myself and scouring the Internet.  I went home determined to re-write the Html.ListBox helper myself and just be done with it. 

However, the next morning I decided to download the MVC source.  Here’s a great and simple article from Steve Sanderson on how to attach it to your project so that you can step through it with the Visual Studio debugger.

One nice thing about Html.ListBox and Html.DropDownList helpers is that you don’t have to provide a collection of SelectItems explicitly.  If you have an item in the Model or ViewData that has the same name as the ListBox or DropDownList then it will find it automatically.  If you do explicitly provide the SelectItem collection you can opt to not provide the default values explicitly.  Again, as long as you have a object (or member of an object) that has the same name as the ListBox or DropDownList it will find it and use it.

Here’s the rub though.  If you explicitly define the SelectList collection and the default values (as I did in my code) the Html helper looks to the Model and ViewData first before your default items.  This normally never causes a problem, until you name your ListBox or DropDownBox the same name as one of your objects or members.

Notice that I called my DropDownList “Guests”? Notice how the Model is a Show object, which has a Guests member variable?  It turns out that the Html helper was grabbing this collection, rather than my values.  Technically these are the guests I want to default to, however, the Html helper was getting a collection of Guest objects not a collection of Guest Id values.  When it compared each guest Id in the full list with the actual Guest objects in the default list it never found a match.  Thus, nothing was ever selected.

What’s the fix?  Simply change the name of your ListBox or DropDownList.  When I changed the name value to “GuestListBox” all worked well, because, as you would expect there is no object or member in the Model or ViewData called “GuestListBox”.  Once it couldn’t find one it dropped through that code and used the default values I provided.

To find exactly where MVC is doing this check out the SelectInternal member in the SelectExtensions.cs file.  This is the internal method that both ListBox and DropDownList eventually call.  You can see commented code that states:

// If we haven’t already used ViewData to get the entire list of items then we need to
// use the ViewData-supplied value before using the parameter-supplied value.

In my opinion this is a bug.  It shouldn’t even check the ViewData or Model if you have already explicitely provided the default values.

So, I added my vote to the bug that was already submitted for this on CodePlex.  I haven’t check to see if this has been resolved in MVC v2.

Getting things done – Sometimes simpler is better

I’m working on a new project for my work.  I’m using S#arp Architecture started by Billy McCafferty, which is a framework using ASP.Net MVC, NHibernate for database persistence and Ninject for dependency injection.  It’s really shaping up to be a great framework and I’m learning a lot.

With NHibernate, you often come to the point where you have to map subclasses to your database.  The two prevailing strategies are one table per class or one table per class hierarchy.  You can look up any several dozen documents on these strategies.  For my project I chose one table per class hierarchy for one of my classes and its subclasses.

If you’re not familiar with Hibernate mapping documents, if you have one table per class hierarchy you have a single table storing the parent class and any subclasses all together.  This has its advantages, however, you have to have a column in the table (the discriminator) which holds a value telling Hibernate which class this record belongs to.  For instance, if I have a parent class of Person with two subclasses of Employee and Client, my discriminator field may hold “P” for Person, “E” for employee and “C” for client.

S#arp Architecture makes use of Fluent Nhibernate, which allows you to create Hibernate mapping documents in code in a fluent style.  Very nice.  It makes full use of generics and has been a pleasure to use once I dusted off my knowledge of mapping documents.

The problem is Fluent has a bug where it puts in discriminator elements for the subclasses.  There should only be one element describing the discriminator field and that goes in the main class definition.  For some reason Fluent also puts it in the subclass definitions causing Hibernate to throw an error.

So, what do you do?  Well, the great thing about open source is you can open up the code and find the problem.  I did just that but I hit another snag.  I love using Generics in my code, however, I’m not experienced at all at creating classes that use them.  After spending about a half an hour to find the problem and think about how to solve it I realized I didn’t have time to refactor the bug and relevant pieces to a logical usage.

So, I did the next best thing.  I added two lines of code to remove the element during the generation and fixed the issue.

The code to generate and return the mapping in xml ends with this:

1: writeTheParts(classElement, visitor);
2: return document;

So, I added the following:

1: writeTheParts(classElement, visitor);
2: foreach (XmlNode discriminator in document.SelectNodes("//subclass/discriminator"))
3:   discriminator.ParentNode.RemoveChild(discriminator);
4: return document;

In the code above, document is an XmlDocument type.  I used an XPath expression to find all the subclass elements that had a child discriminator element and then removed them.  The nice thing is this doesn’t affect any code that doesn’t have the bug.  It’s fairly clean and doesn’t get in the way at all.

While this certainly isn’t a true fix it does solve the problem in a very orderly and understandable way.  I still think it’s a hack but it helped me get back to my project.  I did submit the test (always create a test first!) and the patch to the project incase they wanted to fix the bug.  I also suggested that it still be refactored correctly by someone who is a little more experienced with Generics and the project.

So, in a case like this, I’d advocate simple when the solution is fairly clean, does exactly what it is supposed to do and there may not be time (or experience) for the alternative.

Unit Testing HtmlHelpers

I have a pretty decent project at my work that I am using ASP.Net MVC for.  It is a great framework and it’s my first real development project in a long time so I’m pretty excited.

I’m really stealing myself to use TDD, with very heavy emphasis on the first D, “Driven”.  I’m not allowing myself to write a line of code without a test first.  It’s a little hard because IntelliSense is pretty meaningless in tests if the actual objects haven’t even been built.  When things get a little muddy I go and create a quick empty controller or something before I finish the actual test.  This is just so that I can let VS 2008 and ReSharper work their magic adding namespaces and proper syntax.

Anyway, before I get into the nitty gritty I wrote this post because I was on a search of how to unit test my custom HtmlHelpers.  I found some decent posts and looked at the source for MvcContrib, but I didn’t need anything fancy.  I’m just creating a file input tag. I don’t need to mock the Context or anything.  I didn’t find a easy post anywhere on how to do this.  It turns out if you don’t need anything special it’s quite easy indeed.  If you’re looking for this then read on.

Anyway, I have a form where the user will upload a couple of files.  Using Scott Hanselman’s great blog post on unit testing uploading files using MVC I have a controller that now requires two non-empty files before it returns a success view.

I’ll flesh that out a little more with some actual validation of the upload contents but for now I’m refactoring my initial form.

From Scott’s post what I began with is the following:

This doesn’t make any use of HtmlHelpers so I wanted to refactor this.  There is no FileInput HtmlHelper but you could easily use the Textbox extension as in the following:

<%= Html.TextBox("tableA", null, new { enctype = "multipart/form-data" }) %>

And this would product the following correct HTML:


There are two things about this that bug me however.  First and foremost, did I save any effort?  The HtmlHelper code is actually longer then the straight HTML itself!  In my opinion HtmlHelpers should help in some way, not just be used because they are there.  So, if it doesn’t save me any extra work or protect me from something such as malformed urls or localization formatting, then there’s no point.  Second is that blank value attribute.  Why do I need that?

So, being the developer that I am, I thought I’d create my own HtmlHelper for a File Input textbox.  Using TDD this requires that I write my test first:

namespace Tests.CECWA.Web.Helpers{
  [TestFixture]
  public class FileInputExtensionsTests
  {
    [Test]
    public void CanGetCorrectOutput()
    {
      string html = FileInputExtensions.FileInput(null, "testName");
      const string expected = "";
      Assert.That(html, Is.EqualTo(expected));
    } 
  } 
}

I use NUnit but it’s pretty similar in any testing framework.  FileInputExtensions is my extension class containing my custom HtmlHelper FileInput.  The null parameter is the HtmlHelper class that is actually extended.  Since I’m not making use of the Context or anything else I can simply pass a null in.  If I were doing something fancy such as needing the base path of the app then I’d have to mock this out like the other more extensive posts do.

This test simply calls my extension method and stores the result in the html variable.  I store what I expect in my expected variable.  Then I assert that they are equal.  Quite easy actually.

When I build my project it of course doesn’t compile since there is no FileInputExtensions class.  So I do the bare minimum to compile:

namespace CECWA.Web.Helpers
{    
  public static class FileInputExtensions    
  {        
    public static string FileInput(this HtmlHelper helper, string name)        
    {            
      throw new NotImplementedException();        
    }    
  }
}

Everything now compiles but my test fails.  So, lets complete the method:

public static string FileInput(this HtmlHelper helper, string name)
{
  return String.Format("", name); 
} 

All set.  My test passes and now I’ve got an extension method all ready to use.

After creating another test and extension method for a Label element I have now refactored my View above to the following:

<% Html.BeginForm("ReceiveCASEMISTables", "SELPA", FormMethod.Post, new {enctype = "multipart/form-data"}); %>
  <%= Html.Label("tableA", "Table A Filename:") %>
  <%= Html.FileInput("tableA") %> 
<%= Html.Label("tableB", "Table B Filename:") %> <%= Html.FileInput("tableB") %>
<%= Html.FormHelper().Submit("Upload Files") %> <% Html.EndForm(); %>

OK, if you compare the pure HTML version and this one you don’t see much difference in the “effort”.  However, this is more protected against improper paths in my form action url, typos in my various field ID’s, and just general lousy HTML coding in general.

Multiply this over several dozen pages and you start to feel a real tight management on your html output and maintainability.  For instance, if I want to change the way my FileInput extension renders I simply update my test, update my class to pass and now every place a FileInput helper is used has been updated through out the site.  Nice!

Technorati Tags: ,