Condividi tramite

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

  • Articolo

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

  • Anonymous
    May 05, 2010
    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.

  • Anonymous
    May 05, 2010
    Good link - thanks Dan! Simon

  • Anonymous
    July 07, 2010
    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)...

  • Anonymous
    October 05, 2010
    Thanks DAN!!!! yOu are the best!!!!

  • Anonymous
    October 05, 2010
    You rock Dan!!!! That is very very interesting.........You saved my friend Herman a bunch loads of time.

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

  • Anonymous
    October 05, 2010
    @ Herman, no problem - pleased you like it :-) Simon

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

  • Anonymous
    December 29, 2010
    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.

  • Anonymous
    January 03, 2011
    The comment has been removed

  • Anonymous
    January 28, 2011
    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?

  • Anonymous
    January 29, 2011
    @ 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

  • Anonymous
    February 14, 2011
    // 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");

  • Anonymous
    February 15, 2011
    @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

  • Anonymous
    March 09, 2011
    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.

  • Anonymous
    March 09, 2011
    @ 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

  • Anonymous
    February 20, 2013
    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!

  • Anonymous
    March 02, 2013
    Thanks for this features. I wasted a few hours on it!

  • Anonymous
    September 03, 2013
    The comment has been removed

  • Anonymous
    February 26, 2014
    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.

  • Anonymous
    June 18, 2014
    Thanks Simon. Spent a couple of hours on this problem till I found your article. Thank you !

  • Anonymous
    September 02, 2014
    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.

  • Anonymous
    September 30, 2014
    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!

  • Anonymous
    June 04, 2015
    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);        }

  • Anonymous
    June 07, 2015
    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