Scott Hanselman

The Weekly Source Code 38 - ASP.NET MVC Beta Obscurity - ModelState.IsValid is False because ModelBinder pulls values from RouteData

December 4, '08 Comments [18] Posted in ASP.NET | ASP.NET MVC | Source Code
Sponsored By

I found a bug in an application I'm working on. I couldn't decide if it's a bug in the ASP.NET MVC Framework or a feature. I knew this, however. Both their code and my code runs exactly as it was written. ;)

First, the behavior I'm seeing, then a pile of unnecessary technical context because I like to hear myself talk, then my conclusion. Regardless, it's great fun!

UPDATE: I've been promptly teased/beaten by the MVC team, as they pointed out, pointedly, is the same behavior pointed out by Ayende like 15 years ago. It was prompted fixed/changed, and the new behavior changes the order (listed below) to 3, 1, 2 as I understand it. I hang my head in shame. ;) It's been totally fixed in the Release Candidate, so this post acts only as my own CSI:ASPNETMVC.

The Behavior

My application does CRUD (Create, Read, Update, Delete) on Dinners. When you are entering a new Dinner object, you fill out a form and POST your HTML form.

We take in the Dinner and save it (I've removed goo for clarity):

[AcceptVerbs(HttpVerbs.Post)]
[Authorize]
public ActionResult New([Bind(Prefix = "")]Dinner item)
{
if (ModelState.IsValid)
{
item.UserName = User.Identity.Name;
_dinnerRepository.Add(item);
_dinnerRepository.Save();
TempData["Message"] = item.Title + " Created";
return RedirectToAction("List");
}
}

The behavior I'm seeing is that ModelState.IsValid is ALWAYS false.

Note at this point that I'm not clever enough to go digging into the ModelState object for exactly why. More on my stupidity later. ;)

The Context

The Form POST looks like this (RAW HTTP):

Title=Foo&EventDate=2008-12-10&EventTime=21%3A50&Description=Bar&HostedBy=shanselman&LocationName=SUBWAY&MapString=1050+SW+Baseline+St+Ste+A1%2C+Hillsboro%2C+OR&ContactPhone=%28503%29+601-0307&LocationLatitude=45.519978&LocationLongitude=-123.001934

See how there's a bunch of Name/Value pairs in there? They mostly line up with the names of properties in my class as seen below.

Dinner Class Diagram

However, note that ID isn't in the Form POST. It's not there because it's an identity, and it'll be created when we save the Dinner to the database.

The New() method takes a Dinner as a parameter. That Dinner is create by the system because using the DefaultModelBinder. That binder looks at the values in the HTTP POST and tries to line them up with the properties on the object. Notice that there's no "ID" in the Form POST. Why is ModelState.IsValid false?

If I look in the ModelState.Values, I can see that the first value says "A value is required." ModelState.Keys tells me that's "ID" and that "" was passed in.

Watch Window showing error in ModelState.Values

Where did MVC get an ID from and why is it ""? I don't need an ID, I'm in the middle of making a Dinner, not editing one.

Well, it turns out that the DefaultValueProvider, the thing that, ahem, provides values, to the DefaultModelBinder looks in a few places for its values. From the source on CodePlex, note the nice comments:

public virtual ValueProviderResult GetValue(string name) {
if (String.IsNullOrEmpty(name)) {
throw new ArgumentException(MvcResources.Common_NullOrEmpty, "name");
}

// Try to get a value for the parameter. We use this order of precedence:

// 1. Values from the RouteData (could be from the typed-in URL or from the route's default values)

// 2. URI query string

// 3. Request form submission (should be culture-aware)


object rawValue = null;
CultureInfo culture = CultureInfo.InvariantCulture;
string attemptedValue = null;

if (ControllerContext.RouteData != null && ControllerContext.RouteData.Values.TryGetValue(name, out rawValue)) {
attemptedValue = Convert.ToString(rawValue, CultureInfo.InvariantCulture);
}
else {
HttpRequestBase request = ControllerContext.HttpContext.Request;
if (request != null) {
if (request.QueryString != null) {
rawValue = request.QueryString.GetValues(name);
attemptedValue = request.QueryString[name];
}
if (rawValue == null && request.Form != null) {
culture = CultureInfo.CurrentCulture;
rawValue = request.Form.GetValues(name);
attemptedValue = request.Form[name];
}
}
}

return (rawValue != null) ? new ValueProviderResult(rawValue, attemptedValue, culture) : null;
}

Seems that while I assumed that the Form POST was the only place that a Model Binder would go looking, in fact, it looks in three places:

  1. Values from the RouteData
  2. URI query string
  3. Request form submission (should be culture-aware)

The emphasis is mine. At this point I know I'm not seeing a bug, but rather an uncomfortable side-effect of my overly generic naming. I created an ID property on Dinner, but is also have the default route in my Global.asax.cs:

routes.MapRoute(
"Default", // Route name
"{controller}/{action}/{id}", // URL with parameters
new { controller = "Home", action = "List", id = "" } // Parameter defaults
);

Note the default value for ID. This can be confirmed in the debugger at the same breakpoint I used before. The RouteData collection shows an ID name/value pair, where the value is "".

Debuger showing the RouteData object

The DefaultModelBinder saw that an ID was available, but it was set to "", which is different than complete omission. If it wasn't there at all, it wouldn't have been required. If it was set to "A" as in /controller/action/A then I'd have seen a different error of "The value 'A' is invalid." as "A" isn't an int.

The Conclusion

I'm going to change my model to use Dinner.DinnerID instead of Dinner.ID so that there's no reuse of tokens between the Route/URL and the Model's property names. Fun stuff! It was so nice to just check the ASP.NET MVC source code to solve my problem. Kudos also to the ASP.NET MVC team for making the source easy to read.

About Scott

Scott Hanselman is a former professor, former Chief Architect in finance, now speaker, consultant, father, diabetic, and Microsoft employee. He is a failed stand-up comic, a cornrower, and a book author.

facebook twitter subscribe
About   Newsletter
Sponsored By
Hosting By
Dedicated Windows Server Hosting by ORCS Web
Thursday, December 04, 2008 7:30:05 AM UTC
Very interesting - I didn't know that! thank you very much!
Thursday, December 04, 2008 7:49:59 AM UTC
Forgive my ignorance...haven't begun using MVC yet. When you say you're going to change your model to use Dinner.DinnerID, does that mean in your class you'll map DinnerID in the underlying ID column in the database? I guess my question is, are you going to be changing the underlying database schema's ID column to be named DinnerID? If so, what if you can't change the schema of the database? I'm assuming this is just at the code level, but just want to be sure. Thanks Scott.
Thursday, December 04, 2008 7:55:39 AM UTC
Hi Scott,

Ayende ran into a similar problem a month ago :)
http://ayende.com/Blog/archive/2008/11/07/reproducing-a-bug.aspx

Regards,
Bogdan
Bogdan Brinzarea
Thursday, December 04, 2008 8:23:33 AM UTC
Ayende beat you to it, and Bogdan beat me too. ;)
Thursday, December 04, 2008 8:58:45 AM UTC
Personally, I think this is a bug. I think the look up should be

1. Values from the RouteData (could be from the typed-in URL)
2. URI query string
3. Request form submission (should be culture-aware)
4. Values from the RouteData default values.

The only time it should use the RouteData's default values is if it can't be found anywhere else. Otherwise, you model is influenced by the technology that you are using. It's one thing to be influenced because of persistence, but a web framework mucking with you model?
Thursday, December 04, 2008 9:07:02 AM UTC
Hm....JustRudd...I'm starting to think you're right. It makes sense in RETROSPECT, but still it feels bad.
Thursday, December 04, 2008 11:01:44 AM UTC
+1 to JustRudd's comments. BTW, there's a bug registered for it at CodePlex, so those that don't like the way the current behavior is, that's the place to go and add your 2 cents..

http://www.codeplex.com/aspnet/WorkItem/View.aspx?WorkItemId=2549
Thursday, December 04, 2008 11:56:51 AM UTC
I've ran into this a few times and would class it as a bug. It doesn't just affect Id, it has the potential to affect any property name that matches a value in the route dictionary.
Thursday, December 04, 2008 1:07:22 PM UTC
Can't you just tell the ModelBinder to not try and bind the 'ID' value (or explicitly what to try and bind)? I know you can using UpdateModel, I assume you could do the same thing when accepting a Model as a parameter?
huey
Thursday, December 04, 2008 1:18:04 PM UTC
It's not a bug. The best way to handle this is to use a "ViewModel" object to represent your form. This isn't fancy. It's basically a DTO that the MVC framework binds from the form for your controller action.

We're object oriented right? A dinner isn't the same thing as an input form that represents a dinner. We should model reality and model the form itself, bind from it and pass it as a message into a service that manages Dinners.

You have allowed the technical implementation of form inputs to impact your domain model in a way that makes changing your model object (Dinner) difficult. Even worse, to a new developer working on the Dinner object, the fact that its identifier must be named DinnerId so that the UI works is non-obvious. After gaining nothing but brittleness, you lose reuse as deriving your model objects from a layer supertype (to share an Id property for example) becomes impossible.

By using a ViewModel form object you gain more: now the UI can use authorization and validation etc. to protect the domain model from corruption.
Matt H
Thursday, December 04, 2008 1:20:53 PM UTC
Yeah, I've run into this before a number of times, and it gets quite annoying. RouteData should be the last thing you check. A form post value should always override it in my opinion.

I ended up doing _exactly_ what you did, and as somewhat of an object bigot, I don't like constraints forced upon my objects. :)
Thursday, December 04, 2008 1:40:18 PM UTC
Amazingly enough, it looks like GetValue is declared as virtual, so anyone is free to create their own implementation. Of course, that assumes that the DefaultModelBinder provides a nice clean way to substitute a different value provider. Otherwise you have to go all the way and make your own DefaultModelBinder2.
Thursday, December 04, 2008 3:05:04 PM UTC
I know this is nitpicky, but this 'Id' vs 'NnnnId' naming convention is something I've wondered about for a while. Personally, I prefer the name 'Id', since a) it's shorter and b) including the class name is redundant. For example, you wouldn't want 'Customer.CustomerFirstName'. I also agree with the other posters that changing your model to suit the UI smells bad.
Daniel
Thursday, December 04, 2008 4:42:10 PM UTC
This is the exact same issue that Ayende wrote about a while ago.

We already checked a change into our repository based on his report. Scott, you could have simply asked us, you're part of the Big Blue Monster now, whether you like it or not. Heck, you have access to look at the source in progress if you want.
Thursday, December 04, 2008 9:14:05 PM UTC
The update to this has me on the floor - too funny.
Thursday, December 04, 2008 10:13:51 PM UTC
I ran into this issue sometime ago, Ayende did also. And for the life of me, I remember that in the first previews, the way it worked was that both the query and form fields would override the default value of the route data. At some point that changed since demos I had working stopped doing so.
Thursday, December 04, 2008 11:08:01 PM UTC
Scott, I think this is wrong:
I'm going to change my model to use Dinner.DinnerID instead of Dinner.ID so that there's no reuse of tokens between the Route/URL and the Model's property names.


Instead you should change how DefaultValueProvider behaves or override default behaviour with ScottValueProvider.
You don't want to rewrite your business model and the whole application just because of there's inconsistency in some framework.

Cheers.
Friday, December 05, 2008 10:56:08 AM UTC
Correct me if I'm wrong, but even with the change to '3 1 2' it would still not solve your problem. You're not passing the ID in the POST data (the 3) so it will still fall back to route defaults (the 1) and you'll be back at ID="". It will fix Ayende's example (as he's passing the ID in the POST) but I'm afraid it won't fix yours (or mine - I got bitten by it too). Or am I missing something?
The way I solved it was to declare a concrete route for my Save action and don't include the 'id' part in it.
Marcin
Comments are closed.

Disclaimer: The opinions expressed herein are my own personal opinions and do not represent my employer's view in any way.