Possible bug in ASP.NET MVC with form values being replaced

Question

I appear to be having a problem with ASP.NET MVC in that, if I have more than one form on a page which uses the same name in each one, but as different types (radio/hidden/etc), then, when the first form posts (I choose the 'Date' radio button for instance), if the form is re-rendered (say as part of the results page), I seem to have the issue that the hidden value of the SearchType on the other forms is changed to the last radio button value (in this case, SearchType.Name).

Below is an example form for reduction purposes.

<% Html.BeginForm("Search", "Search", FormMethod.Post); %>
  <%= Html.RadioButton("SearchType", SearchType.Date, true) %>
  <%= Html.RadioButton("SearchType", SearchType.Name) %>
  <input type="submit" name="submitForm" value="Submit" />
<% Html.EndForm(); %>

<% Html.BeginForm("Search", "Search", FormMethod.Post); %>
  <%= Html.Hidden("SearchType", SearchType.Colour) %>
  <input type="submit" name="submitForm" value="Submit" />
<% Html.EndForm(); %>

<% Html.BeginForm("Search", "Search", FormMethod.Post); %>
  <%= Html.Hidden("SearchType", SearchType.Reference) %>
  <input type="submit" name="submitForm" value="Submit" />
<% Html.EndForm(); %>

Resulting page source (this would be part of the results page)

<form action="/Search/Search" method="post">
  <input type="radio" name="SearchType" value="Date" />
  <input type="radio" name="SearchType" value="Name" />
  <input type="submit" name="submitForm" value="Submit" />
</form>

<form action="/Search/Search" method="post">
  <input type="hidden" name="SearchType" value="Name" /> <!-- Should be Colour -->
  <input type="submit" name="submitForm" value="Submit" />
</form>

<form action="/Search/Search" method="post">
  <input type="hidden" name="SearchType" value="Name" /> <!-- Should be Reference -->
  <input type="submit" name="submitForm" value="Submit" />
</form>

Please can anyone else with RC1 confirm this?

Maybe it's because I'm using an enum. I don't know. I should add that I can circumvent this issue by using 'manual' input () tags for the hidden fields, but if I use MVC tags (<%= Html.Hidden(...) %>), .NET MVC replaces them every time.

Many thanks.

Update:

I've seen this bug again today. It seems that this crops its head when you return a posted page and use MVC set hidden form tags with the Html helper. I've contacted Phil Haack about this, because I don't know where else to turn, and I don't believe that this should be expected behaviour as specified by David.

Solution 1

Yes, this behavior is currently by design. Even though you're explicitly setting values, if you post back to the same URL, we look in model state and use the value there. In general, this allows us to display the value you submitted on postback, rather than the original value.

There are two possible solutions:

Solution 1

Use unique names for each of the fields. Note that by default we use the name you specify as the id of the HTML element. It's invalid HTML to have multiple elements have the same id. So using unique names is good practice.

Solution 2

Do not use the Hidden helper. It seems like you really don't need it. Instead, you could do this:

<input type="hidden" name="the-name" 
  value="<%= Html.AttributeEncode(Model.Value) %>" />

Of course, as I think about this more, changing the value based on a postback makes sense for Textboxes, but makes less sense for hidden inputs. We can't change this for v1.0, but I'll consider it for v2. But we need to think through carefully the implications of such a change.

Solution 2

Same as others I would have expected the ModelState to be used to fill the Model and as we explicitly use the Model in expressions in the view, it should use the Model and not ModelState.

It's a design choice and I do get why: if validations fail, the input value might not be parseable to the datatype in the model and you still want to render whatever wrong value the user typed, so it's easy to correct it.

The only thing I don't understand is: why isn't it by design that the Model is used, which is set explicitly by the developer and if a validation error occurred, the ModelState is used.

I have seen many people using workarounds like

• ModelState.Clear(): Clears all ModelState values, but basically disables usage of default validation in MVC
• ModelState.Remove("SomeKey"): Same as ModelState.Clear() but needs micromanagement of ModelState keys, which is too much work and it doesn't feel right with the auto binding feature from MVC. Feels like 20 years back when we were also managing Form and QueryString keys.
• Rendering HTMLthemselves: too much work, detail and throws away the HTML Helper methods with the additional features. An example: Replace @Html.HiddenFor by <input type="hidden" name="@NameFor(m => m.Name)" id="@Html.IdFor(m=>m.Name)" value="@Html.AttributeEncode(Model.Name)">. Or replace @Html.DropDownListFor by ...
• Create custom HTML Helpers to replace default MVC HTML Helpers to avoid the by-design issue. This is a more generic approach then rendering your HTML, but still requires more HTML+MVC knowledge or decompiling System.Web.MVC to still keep all other features but disable ModelState precedence over Model.
• Apply the POST-REDIRECT-GET Pattern: this is easy in some environments, but harder in the ones with more interaction/complexity. This pattern has it's pros and cons and you shouldn't be forced to apply this pattern because of a by-design choice of ModelState over Model.

Issue

So the issue is that the Model is filled from ModelState and in the view, we set explicitly to use the Model. Everybody expects the Model value (in case it changed) to be used unless there's a validation error; then the ModelState can be used.

Currently, in the MVC Helper extensions, the ModelState value gets precedence over the Model value.

Solution

So the actual fix for this issue should be: for each expression to pull the Model value the ModelState value should be removed if there is no validation error for that value. If there's a validation error for that input control the ModelState value shouldn't be removed and it will be used like normal. I think this solves the issue exactly, which is better than most workarounds.

The code is here:

    /// <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);
        }
    }

And then we create our own HTML Helper extensions todo this before calling the MVC extensions:

    public static MvcHtmlString TextBoxForModel<TModel, TProperty>(this HtmlHelper<TModel> htmlHelper,
        Expression<Func<TModel, TProperty>> expression,
        string format = "",
        Dictionary<string, object> htmlAttributes = null)
    {
        RemoveStateFor(htmlHelper, expression);
        return htmlHelper.TextBoxFor(expression, format, htmlAttributes);
    }

    public static IHtmlString HiddenForModel<TModel, TProperty>(this HtmlHelper<TModel> htmlHelper,
        Expression<Func<TModel, TProperty>> expression)
    {
        RemoveStateFor(htmlHelper, expression);
        return htmlHelper.HiddenFor(expression);
    }

This solution removes the issue but doesn't require you to decompile, analyze, and rebuild whatever MVC is offering you normally (don't forget also managing changes over-time, browser differences, etc.).

I think the logic of "Model value unless validation error then ModelState" should have been by-design. If it was, it wouldn't have bitten so many people, but still covered what MVC was intended todo.

Solution 3

Heads-up - this bug still exists in MVC 3. I'm using the Razor markup syntax (like that really matters), but I encountered the same bug with a foreach loop that produced the same value for an object property every single time.

Solution 4

I just ran into same issue. Html helpers like TextBox() precedence for passed values appear to behave exactly opposite what I inferred from the Documentation where it says:

The value of the text input element. If this value is null reference (Nothing in Visual Basic), the value of the element is retrieved from the ViewDataDictionary object. If no value exists there, the value is retrieved from the ModelStateDictionary object.

To me, I read that the value, if passed is used. But reading TextBox() source:

string attemptedValue = (string)htmlHelper.GetModelStateValue(name, typeof(string));
tagBuilder.MergeAttribute("value", attemptedValue ?? ((useViewData) ? htmlHelper.EvalString(name) : valueParameter), isExplicitValue);

seems to indicate that the actual order is the exact opposite of what is documented. Actual order seems to be:

1. ModelState
2. ViewData
3. Value (passed into TextBox() by caller)

Solution 5

This would be the expected behavoir - MVC doesn't use a viewstate or other behind your back tricks to pass extra information in the form, so it has no idea which form you submitted (the form name is not part of the data submitted, only a list of name/value pairs).

When MVC renders the form back, it is simply checking to see if a submitted value with the same name exists - again, it has no way of knowing which form a named value came from, or even what type of control it was (whether you use a radio, text or hidden, it's all just name=value when its submitted through HTTP).

View more solutions

Comments

[Kelly]

i have also noticed this behavior with the Hidden input and have resulted to changing the value in the controller :(

[E Rolnicki]

I have also spent a lot of time trying to debug this Html.Hidden() issue. for some reason this helper is not able to read an enum value which will play nicely with a model binder. i was forced to manually write the hidden input field (which worked as desired/expected)

[Dacker]

@Dan Atkinson Please check my solution that removes ModelState value for each input control unless when there's a validation error. This results in exactly what developer's want and what Microsoft should have done by-design. It's a simple fix of exactly what is going wrong, without you having to decompile and rewrite the logic in MVC HTML Helper extensions.

[Dan Atkinson]

@Dacker Thanks but my use case is no longer relevant to this question.

[Dan Atkinson]

I would agree with what you're saying, except that I'm explicitly requesting the hidden form field values, and MVC shouldn't have any problem setting these value to what I'm setting them to. When the form has been posted, these value shouldn't change, but they are.

[Dan Atkinson]

Put another way, if I wrote that form in simple HTML, it would work as expected, and set the hidden form fields as I requested in my ascx file. If I render them through the Html.Hidden(), .NET MVC messes them up.

[Dan Atkinson]

Yes, I think it does make more sense for textboxes to be changed, but not so much for hidden form fields. I can't imagine a situation where, if I had more than one form on a page (like the example in my reduction), I would want it to be anything other than the value I set it to.

[Dan Atkinson]

Ack... Damn these character limits. :) I think that I might make an extension for Html.Hidden, enforcing the requirement of an id (rather than just use the htmlAttribute object), as I don't like the idea of tag soup. That should effectively be solution 1. :) Thanks for your reply!

[edoloughlin]

Another way to set the id is: <%= Html.Hidden("name", value, new {id="my-id"}) %> (or something like that)

[Dan Atkinson]

This is true, but I believe that developers might accidentally forget to do this. Although I can't help but feel that this is coding around a bug! :)

[Dan Atkinson]

I would look at creating your own hidden helper instead.

[bkaid]

still is "broken" in mvc3 preview 1.

[Pop Catalin]

@Haacked, This is very bad, design decision and not documented as such!

[Nathan Kurz]

I don't understand this behaviour at all. Is this documented somewhere??

[Dan Atkinson]

I've tagged the issue as MVC3 as well. Thanks.

[Schotime]

If I use model binding to pass in a Amount property via the query string and I have an Amount property on my viewmodel it uses the value from the query string, not the one on my viewmodel. This doesn't seem right. Especially for a GET scenario.

[Marko]

If you have a ViewBag with the same name then the values will overwrite each other. For example, in your case, if you have a ViewBag.Remote set in your controller then that could overwrite your input field value.

[Darbio]

The solution below using ModelState.Clear() solves the problem when binding back to a partial view.

[A.R.]

I haven't seen this much lipstick on a pig in a while. It isn't behaviour by design, it is a bug plain and simple.

[cateyes]

I think MSDN should fully document the mechanism of how the fields get populated with values when rendering a view after a post back. This will help a lot when people just learn ASP.NET MVC. It is important concept.

[Dacker]

This works great, but it takes a lot of decompiling and rebuilding todo the same for TextBoxFor, CheckBoxFor, DropDownListFor, etc. Please check my solution where you can simply remove ModelState value of a input control, but only if there is no validation error. If there is a validation error you would want ModelState to be used like by-design.

[Dacker]

@Haacked Shouldn't the logic have been "Use Model value unless there is a validation error"? In general there is a consensus that this design choice is unexpected. The ModelState value should only be used if it's necessary, otherwise the Model value should be used since it's set explicitly in code.

[Omu]

it makes sense when you need to change the value of a hidden input on postback (partial save, set the Id field)

[jmoreno]

@lomaxx: it is now, at least for TextBox's (not hidden). See the comment for msdn.microsoft.com/en-us/library/dd492984(v=vs.100).aspx

[Sailing Judo]

I just spent 24 hours trying to debug our apps because of this "feature".

[Dan Atkinson]

In all fairness, there have been a large number of workarounds over the last 5 years.

[Admin]

It's not fixed in MVC 5. Spent 1.5 days before I stumbled upon someone mentioning the "nuke" option that is ModelState.Clear().

[Carson63000]

@SailingJudo I just spent half a day debugging due to this particular violation of the Law of Least Astonishment. Nice to know that even more than a decade on, it's still wasting developers' time.