hiddenFor helper is pulling id from action parameter, not the viewModel

Question

I've stumbled upon a really weird situation with ASP.Net MVC, passing an "id" as an action parameter and an "id" hidden form element. I have an action that has an "id" parameter. This id value represents a project. I have a controller action where I'm creating a data entry form for assigning an employee to the passed in project (I create a drop down list where administrator selects an employee to be assigned to passed project). By assigning an employee to a project, I create a ProjectEmployee record (project id, employee id, and an id that represents the combination which is an identity column in the database). The identity column (which is named "id") is also a hidden form element. This is necessary because I need to be able to edit the project/employee assignment at a later date.

Anyways, when creating a new employee assignment to a project, the project id (which is the "id" being passed to the action) is being applied to the "id" hidden form element.

I'm passing 117 into the action. 117 is the projectId. It's being set to the "id" hidden field which should be 0 because 0 represents a new project/employee assignemnt.

View Model

id  - unique id that represents the Project/Employee combination
projectId - the "Id" being passed to action
EmployeeId - what the admin is selecting from drop down
rate
startdate
endDate
...

Data Entry Form

@Html.HiddenFor(m => m.Id) 
@Html.HiddenFor(m => m.ProjectId)
...
Id: @Model.Id <br />
ProjectId: @Model.ProjectId<br />

So, @Html.HiddenFor(m => m.Id) renders a hidden form element with a value of 117. @Model.Id renders 0 to the UI. Stepping through the code I can visually see the value of the Id property to be 0. How come HiddenFor is getting it's wires crossed and pulling the value of 117?

This bug has made it's way into production so I have a mess on my hands with data getting messed up because instead of creating a new record in the database table, I'm actually UPDATING existing records because the "Id" property is erroneously getting set from 0 (which represents a new record) to 117 (which is the projectId) and therefore am updating a different record.

score 7 · Accepted Answer · answered Jun 25 '12 at 14:18

7

How come HiddenFor is getting it's wires crossed and pulling the value of 117?

That's by design. All HTML helpers such as TextBoxFor and HiddenFor first use the value of ModelState when binding and then the value of the model. I presume your controller action looks like this:

public ActionResult Foo(int id)
{
    SomeModel model = ...
    // At this stage model.Id = 0
    return View(model);
}

The thing is that the default model binder adds a value into the ModelState with the key id which is used by the helper and the model property value is ignored. This is not a bug. It is how the HTML helpers are designed. It's a bit confusing in the beginning when you don't know it, but once you get accustomed to this behavior you don't fall in the trap a second time.

One possibility to fix the problem is to remove this value from ModelState:

public ActionResult Foo(int id)
{
    ModelState.Remove("id");
    SomeModel model = ...
    // At this stage model.Id = 0
    return View(model);
}

Or rename the Id property inside your model to something else to avoid the conflict.

answered Jun 25 '12 at 14:18

Darin Dimitrov

1,023,142
271
3,287
2,928

1

thank you for the information. I've removed hiddenFor in all of my code and am using instead. I'm using Id as the identity column in all of my database tables, so I'm not able to easily change the model. On the surface, this seems like a bug. It seems to me the model should override the ModelState. I'll have to ponder on this for a while because it doesn't seem intuitive. Anyways, thanks for the reply. – Tom Schreck Jun 25 '12 at 14:28
IMHO this is not a good fix. The next developer that comes on this project won't certainly be aware of this and fall into the trap because using HTML helpers is the standard way to generate input fields in ASP.NET MVC. You would probably be the only one *aware* of *the fix*. – Darin Dimitrov Jun 25 '12 at 14:30
1

I can make the same argument that the html helper should not be overriding the value of the "id" property of the view model. The view model should be the record of authority because it represents data in the database. It's common practice for a value of 0 to represent a new record. My database logic looks for a value of 0 for an identity column then it does an insert. Otherwise, it does an update. Because MVC changed the value of the hidden field from 0 to 117, several records have been updated erroneously. This is very bad. – Tom Schreck Jun 25 '12 at 14:37
1

ModelState.Remove("id") makes the code smell bad and should not be used. Since MVC does what MVC does in regards to overriding ViewModel data, using traditional hidden html data entry form elements is a lot cleaner and is an acceptable solution to the shortcomings MVC is introducing. – Tom Schreck Jun 25 '12 at 14:40
I've gone back through my code and I've been using @Html.HiddenFor(m => m.Id) in several locations since Id is my primary key for my database tables. Every place I use @Html.HiddenFor(m => m.Id) is introducing a scenario where unrelated records are getting updated erroneously when data should have been inserted. I highly recommend anyone who uses "Id" as a record identifier to not use @Html.HiddenFor(m => m.Id) because you seriously run the risk of getting your data ruined because MVC is changing the value of the Id to something unexpected when rendering hidden data entry form elements. – Tom Schreck Jun 25 '12 at 14:58

Charlie · Answer 2 · 2017-04-20T07:22:52.460

As an alternative to changing your field names, you can make a new helper method that doesn't use the route values when evaluating what to put in the value attribute.

I created this helper for the extremely common id case:

public static MvcHtmlString HiddenIdFor<TModel, TValue>(this HtmlHelper<TModel> html,
        Expression<Func<TModel, TValue>> expression)
    {
        var value = ModelMetadata.FromLambdaExpression(expression, html.ViewData).Model.ToString();
        var builder = new TagBuilder("input");
        builder.MergeAttribute("type", "hidden");
        builder.MergeAttribute("name", ExpressionHelper.GetExpressionText(expression));
        builder.MergeAttribute("value", value);
        return MvcHtmlString.Create(builder.ToString(TagRenderMode.SelfClosing));

}

Then you can use this in the template:

@Html.HiddenIdFor(m => m.Id) 
@Html.HiddenIdFor(m => m.ProjectId)

hiddenFor helper is pulling id from action parameter, not the viewModel

2 Answers2

Linked