ASP.NET MVC’s Html Helpers Render the Wrong Value!


First things first – oh no they don’t J


But it can look like a bug if you’re not used to MVC, so I thought it worth calling out.


Scenario


Imagine we have a pair of controller actions like this;


[HttpGet]


public ActionResult Index()


{


    var model = new MyModel


    {


        Count = 1


    };


    return View(model);


}


 


[HttpPost]


public ActionResult Index(MyModel model)


{


    if (!model.Name.StartsWith(“Mr”))


        model.Name = “Mr “ + model.Name;


 


    model.Count++;


 


    return View(model);


}


 


What this is doing should be obvious – we want to increment a counter every time a POST to the Index action occurs, and if they didn’t specify a prefix of “Mr” for their name we add it for them. Easy. The “Index” View to render this looks like this;


<% using (Html.BeginForm())


    { %>       


    <%= Html.HiddenFor(m => m.Count)%>


    Name: <%= Html.TextBoxFor(m => m.Name)%>


    <input type=”submit” value=”Submit” />


<% } %>


This is pretty simple – we’re outputting the Counter as a hidden form field, and letting them type a Name into a Text Box.


The Problem


If you paste these into a solution and give it a try you’ll notice a problem – Name never gets the “Mr” prefix, and our Counter never increments! Instead, they just display exactly as they were POST-ed to the server. Set a breakpoint in the action and inspect the model – you’ll see it is being updated as expected within the action, yet still the changes are not rendered. So what’s going on?


Why?


ASP.NET MVC assumes that if you’re rendering a View in response to an HTTP POST, and you’re using the Html Helpers, then you are most likely to be redisplaying a form that has failed validation. Therefore, the Html Helpers actually check in ModelState for the value to display in a field before they look in the Model. This enables them to redisplay erroneous data that was entered by the user, and a matching error message if needed.


Since our [HttpPost] overload of Index relies on Model Binding to parse the POST data, ModelState has automatically been populated with the values of the fields. In our action we change the Model data (not the ModelState), but the Html Helpers (i.e. Html.Hidden and Html.TextBox) check ModelState first… and so display the values that were received by the action, not those we modified.


To prove this, make a tiny temporary change to the second action;


[HttpPost]


public ActionResult Index(MyModel model)


{


    if (!model.Name.StartsWith(“Mr”))


        model.Name = “Mr “ + model.Name;


 


    model.Count++;


 


    ModelState.Clear();


 


    return View(model);


}


This call to clear the ModelState will mean that the View now works as we expected. However, I wouldn’t recommend this as a long term solution. In reality, there are a few possible solutions;


1.       If we want to display a confirmation page, we should be using the Post-Redirect-Get pattern, and not displaying this content in response to a POST. This means a view render during a POST is always a validation failure, which is what the MVC framework expects.


2.       If we don’t want to do that, but we don’t want to perform any validation of the fields, we shouldn’t be using the Html Helpers. They only exist to assist with MVC framework functionality – if all you want to do is render simple HTML, guess what you should use? HTML! Just render data using <%= Model.Count %> etc.


3.       If you don’t like that, you could consider avoiding Model Binding, and therefore avoiding ModelState from being populated. If we removed MyModel from the Index parameter list and instead accessed POST data using Request.Form this would work… but this fails to take advantage of MVC programming constructs, and complicates testability, so I would avoid it again.


4.       If you have a more complex scenario (perhaps validation on some POSTs, and manual manipulation of posted data on others) you may need to consider calling ModelState.Remove for a specific field – but exercise caution; I do not recommend this approach. Complex action interactions that don’t quite fit with the framework are much more likely to cause you problems later.


There may be other solutions – such as writing your own Model Binder, but fundamentally they’re likely to change how MVC works, so I’d recommend avoiding them for this particular problem. If you’ve come up with something that fits well do let me know though.


So to go with my recommendation of taking control of our HTML, the revised Index view looks like this;


<% using (Html.BeginForm())


    { %>       


    <input type=”hidden” name=”Count” value=”<%: Model.Count %> />


    Name: <input type=”text” name=”Name” value=”<%: Model.Name %> />


    <input type=”submit” value=”Submit” />


<% } %>


Give it a whirl and you’ll see that it works this time!


* if you’re not using Visual Studio 2010, note that you must use <%= Html.Encode(field) %> instead of <%: field %>

Comments (25)

  1. Dan Atkinson says:

    I think I raised a similar issue last March, and Phil Haack commented on it – http://stackoverflow.com/questions/594600/possible-bug-in-asp-net-mvc-with-form-values-being-replaced

    In the end, I created my own Html.Hidden() helper, although this was all for MVC v1.

  2. simonince says:

    Good link – thanks Dan!

    Simon

  3. Gudmundur says:

    Thanks – saved (at least part of) my day! I'm just using a manual Html.Hidden now. I'll just have to get my fellow devs not to convert it back to HiddenFor (which looks much better)…

  4. Herman says:

    Thanks DAN!!!! yOu are the best!!!!

  5. Carl says:

    You rock Dan!!!! That is very very interesting………You saved my friend Herman a bunch loads of time.

  6. Herman says:

    Sorry Simon, I read the comments and thought your name was Dan. Very cool article Simon, yours looks cool too Dan!!

  7. simonince says:

    @ Herman,

    no problem – pleased you like it 🙂

    Simon

  8. Carl says:

    Hi Simon, sorry I also looked quickly at the comments and thought your name was Dan!! Very nice article Simon!!!

  9. Dmitry says:

    Hello.

    "ASP.NET MVC assumes that if you’re rendering a View in response to an HTTP POST, and you’re using the Html Helpers, then you are most likely to be redisplaying a form that has failed validation. Therefore, the Html Helpers actually check in ModelState for the value to display in a field before they look in the Model. ".

    It's illogically! At first, this assertion based on particular case. At second, If ModelState and Model values aren't modified, they are similar so if programmer want to redisplay values he shouldn't modify Model and if Model is displayed than behavior is correct. And at last, EditorFor(model => model.Value) It MUST Editor for model.value but not for value from ModelState with same name.

    Side effects are in Microsoft style. Hgrrrr, I spent about 4 hour on this issue.

  10. simonince says:

    @ Dmitry,

    That's awful – so sorry you lost time to this. But having said that (and remembering I'm not on the product team so my perspective was similar to yours when I first started using this) I've come to find that this is a great assumption to have made. Once you realise what is happening it saves huge amounts of time – it's convention over configuration I guess.

    The reason you can't just redisplay Model values (and hence it must check ModelState) is because some values entered by a user may be impossible to store in the Model.

    For example, if I have a Model with an Int32 property that the user edits in a <input type="text" … />, if they entered a non-numeric value (perhaps their name by mistake) the Model Binder cannot set the Int32 property to the string value. Therefore, in order to redisplay the invalid data the user entered the HtmlHelpers *must* check ModelState.

    I hope that makes sense!

    Simon

  11. Randall Tomes says:

    This happens on HTTPGet also not just HTTPPost. That is strange and seems like a bug. MS assumes it is a post even when you explicity use a get method?

  12. simonince says:

    @ Randall,

    interesting point; I didn't intentionally call out "POST" specifically as a part of this… I guess it's because in 99% of cases I use a POST to pass data to the server. In reality you can set the <form /> tag's method to GET or POST and it will have the same effect. In other words, I think it is entirely logical that it happens for a GET as well, as MVC shouldn't force you to use one verb or the other when both are supported in HTTP for passing data.

    I'm curious about whether there is a use case that involves this GET that is worth documenting? e.g. if it's a common task (e.g. ordering a list on a grid) then writing up the preferred approach could be useful.

    Simon

  13. Jorge says:

    // for each hidden field updated on controller posted to the same page

    // Html.HiddenFor cancels modifications if it finds a value on ModelState

    if (ModelState.HasKey("hiddenFieldName") ModelState.Remove("hiddenFieldName");

  14. simonince says:

    @Jorge;

    that's exactly what I refer to in point (4) in my post. I've also seen a good blog post that describes this approach… but can't find it now.

    I don't like it though – it's usually a sign you're misusing the HTML helper.

    Simon

  15. Steve Mark says:

    Thank you so much!  I just wasted 3 hours and lost significant amounts of hair trying to figure this out.  What is the "correct" way then to return a view?  My app takes input from a user, and based on what they entered displays different controls for further input, and does this for 5 levels, so I want to return a view.  And, since the input is in the form of droplist's, I don't need to validate.

  16. simonince says:

    @ Steve;

    The post-redirect-get pattern is preferred… but failing that, I'd use my recommendation of taking control of the HTML at the foot of this post.

    Simon

  17. Chris Willis says:

    I can't believe how much time and money you just saved me.  My computer was partly out the window when it got wedged.  I came across your site just in time.  Thanks!

  18. Linux Chata says:

    Thanks for this features. I wasted a few hours on it!

  19. David Ching says:

    Thanks for this post, I was pulling my hair out!  Very unintuitive.

    I need to use model binding (e.g. TextBoxFor) because I want data validation.  if data is not valid (tested by ModelState.IsValid), I return View(model).  Only if data is valid  do I proceed to change my model and want to display the changed results.  So I end up calling ModelState.Clear() to achieve that.  Instead of this it would be helpful if I could code:

    if ( !ModelState.IsValid )

     return View();   // Show ModelState data errors

    model.foo = <changed value>;

    return View(model);

    without any of that silly ModelState.Clear().

    Thanks,

    David

  20. Gerard ONeill says:

    Nice post!

    I have no problem with convention over configuration…  But I hate *magic*.  Knowing the flow of your data seems to me to be a prime concern, not something to be brushed under the covers like model binding algorithms, etc.

  21. Ajit says:

    Thanks Simon. Spent a couple of hours on this problem till I found your article.

    Thank you !

  22. A.R. says:

    You can say it over and over, but it is a bug.  The design is poor, unintuitive and based on silly assumptions about what the end user is or isn't doing.

    FTA:

    "ASP.NET MVC assumes that if you’re rendering a View in response to an HTTP POST, and you’re using the Html Helpers, then you are most likely to be redisplaying a form that has failed validation."

    Very, very bad choice.

  23. simonince says:

    AR – I thought that at first. In truth I think it fits very well with REST concepts, so it bothers me less now. I agree with your assertion that poor design = bug, even if something functions according to spec though!

  24. Anonymous says:

    ASP.Net assumption forces you to work exactly according to the design pattern accepting the issues that are by-design. For me the ModelState should be used to fill the Model and the Model values should preceed over ModelState values.

    I do get that for some validation errors the original value should be shown on postback. Hence we have created all of our own extension methods for all MVC HtmlHelper extensions doing the following:

    Check whether there is a validation error for this input control. If so, MVC will use the ModelState.

    If no validation error: Remove the ModelState value for this input control, so ModelState will not get precedence over Model.

    In my opinion this additional check should have been the default behaviour, which results in Model values being used when they need to and ModelState values being used when they need to.

           /// <summary>

           /// Removes the ModelState entry corresponding to the specified property on the model if no validation errors exist.

           /// Call this when changing Model values on the server after a postback,

           /// to prevent ModelState entries from taking precedence.

           /// </summary>

           public static void RemoveStateFor<TModel, TProperty>(this HtmlHelper helper,  

               Expression<Func<TModel, TProperty>> expression)

           {

               //First get the expected name value. This is equivalent to helper.NameFor(expression)

               string name = ExpressionHelper.GetExpressionText(expression);

               string fullHtmlFieldName = helper.ViewContext.ViewData.TemplateInfo.GetFullHtmlFieldName(name);

               //Now check whether modelstate errors exist for this input control

               ModelState modelState;

               if (!helper.ViewData.ModelState.TryGetValue(fullHtmlFieldName, out modelState) ||

                   modelState.Errors.Count == 0)

               {

                   //Only remove ModelState value if no modelstate error exists,

                   //so the ModelState will not be used over the Model

                   helper.ViewData.ModelState.Remove(name);

               }

           }

           public static IHtmlString HiddenForRad<TModel, TProperty>(this HtmlHelper<TModel> htmlHelper,

               Expression<Func<TModel, TProperty>> expression)

           {

               RemoveStateFor(htmlHelper, expression);

               return htmlHelper.HiddenFor(expression);

           }

  25. Anonymous says:

    In the thread Dan Atkinson created I posted that the actual design should have been:

    Use Model value since it is set explicitly in the view, unless there are validation errors for that input control.

    This would result in expected behaviour for everybody in case there are no errors and the desired behaviour as Microsoft has intended in case there are validation errors. It avoids workarounds and their cons like clearing ModelState, but is a more specific alternative that might be exactly what you need.

    stackoverflow.com/…/30698787