0

I have the problem documented here and here and here and especially here where, in an ASP.NET MVC4 view, the html helper function @Html.HiddenFor(m => m.ID) will use the "wrong" data and cause data corruption.

This is a massive problem, and the accepted "solution" is to simply be aware of this and design around it. I think that is a poor solution. (Incidentally, we use something like this instead: <input type="hidden" value="@Model.ID" />)

Every few months, I or a colleague forget and use that html helper, and all hell breaks loose before we track down the problem... once again.

How can I eliminate the use of this function, to avoid mistakes in the future?
- it's not our code, so we cannot mark it with [Obsolete]
- we can override the HtmlHelper, but that's a lot of work just to eliminate one function
- can I write a unit test to pick out this code from the view? (and fail test if that call is detected)
- other ideas?

Community
  • 1
  • 1
Bobby B
  • 2,287
  • 2
  • 24
  • 47
  • 2
    You want a code analysis tool such as FxCop, and to make a rule that will run as part of your build process. I doubt there is a way to do it in the compilation such as an attribute. – Jace Rhea Apr 18 '13 at 15:45

4 Answers4

3

This is not a "problem", it's the way it's supposed to work. And it's not just HiddenFor that does it, every helper will do it. it's the way MVC is designed to work. The fact that you don't seem to understand how MVC works is the real problem.

This is part of the ModelState subsystem, and you are wise to know exactly how this works. Yes, it's not always intuitive, but it's far better that you actually know how things work than to pretend they don't exist, because you'll end up bitten in other ways with other helpers.

But, if you choose otherwise, I would suggest configuring StyleCop to deal with this issue

Erik Funkenbusch
  • 92,674
  • 28
  • 195
  • 291
  • We know how MVC is designed to work, and as others have pointed out, this is unintuitive and can lead to data corruption, very easily. For a new team member, he could blow up our system very easily. The problem is a poor design in the tools we a using. So we need to work around them. – Bobby B Apr 18 '13 at 15:49
  • Stylecop is an interesting idea. – Bobby B Apr 18 '13 at 15:50
  • @BobbyB - but like I said, the problem affects *ALL* helpers, not just Hidden, so I don't see what you're gaining by forbidding Hidden. – Erik Funkenbusch Apr 18 '13 at 16:09
  • We never have this problem with any helper other than `HiddenFor`. Maybe something to do with our workflow or our variable-naming style. Despite knowing better, it is consistently a trap we fall into. – Bobby B Apr 18 '13 at 16:23
  • Even Phil Haack way back in MVC1 admitted it makes no sense for hidden fields: [see here](http://stackoverflow.com/a/619762/1711500) – Bobby B Apr 18 '13 at 16:35
  • @BobbyB - Yes, in *most* cases, it doesn't make sense for hiddens, but it does in some. Particularly, when using Hiddens as a sort of ViewState or when the hidden is updated by a javascript action, so that's why they don't have a special case for hiddens. Also, Phil says it "makes less sense" not "makes no sense". And since it hasn't changed, they obviously realized that there are cases that do make sense. I question why this is actually an issue for you in your normal workflow. I can see how it might be an issue security-wise, but this shouldn't be something that constantly bites you. – Erik Funkenbusch Apr 18 '13 at 17:42
  • Because the property name is ID, and a value called ID is also in the modelstate or formcollection. Very common situation. – Bobby B Apr 18 '13 at 17:55
  • @BobbyB - that just doesn't make sense. Why would you be calling the URL with an id if that's not what you actually want? That seems like broken design. That is, why would your url have a querystring value with an ID, but you write out a different ID to the view? That is what I would forbid. – Erik Funkenbusch Apr 18 '13 at 17:57
  • Dont know if I explained properly: the form is POSTed by AJAX, the model has an ID property, and it is possibly changed before it hits the view again. The view assumes by design, that it is used after validation, but that may not be the case. This is *the* well-reported scneario against this html helper. It's not just me. I won't change my design significantly to fit in with the tool. Unless I have to. But I don't if I use old-school HTML. But then I want to somehow make the codebase fail to build if some new dev, or me, forgets and uses it again. – Bobby B Apr 18 '13 at 18:00
  • I agree with @MystereMan. If your view accepts an id, that should be the id of whatever model your editing. Anything else is a bastardization. Follow proper REST design and you have no problems. Personally I wasn't even aware of this issue because it has never ever affected me. – Chris Pratt Apr 18 '13 at 18:01
  • @BobbyB - I consider that a broken design. Not because of MVC, but because it's confusing and poor practice. Changing your id's around like that is a recipe for disaster in more ways than just this, and you will likely run into them someday. I know I've had to fix code like that before. if you're changing your ID, use a Post/Redirect/Get system. – Erik Funkenbusch Apr 18 '13 at 18:05
  • What if the form has multiple relates fields that refer to different entities' IDs, or there are multiple bits of data submitted in the AJAX POST, or... , One-size-fits-all solution fails. Check my links. I'm not the only one complaining about this helper. Besides, we are now arguing design rather than the original question. I will look into static analysis to pick out this at build-time and integrate into CI. – Bobby B Apr 18 '13 at 18:08
  • Still I didnt get point across.. the ID is chosen from the routes, but the model also has ID related to an entity; the view is reusable widget which renders what could be different viewmodels based on customised AJAX panels on the page... Like I said, it can get complicated; but the html helpers by design assume they are used after validation which may not be the case; the PRG pattern has nothing to do with our use case... For anyone else with this problem: use static analysis to enforce the rule. – Bobby B Apr 18 '13 at 18:13
  • @BobbyB - Nope, still broken. Passing entities to your view is a huge no-no in any non-trivial app (and this is certainly non-trivial), and MVC is designed to deal with the problem of nested items with the same name, and uses Container naming to allow the model binder to differentiate. I'm sorry, but this is busted design. You may like it, and it may work for you, but I promise it will bite you in more ways than this someday (in fact, this is a symptom that it's a busted design). But in any event, I'm glad it (mostly) works for you.. – Erik Funkenbusch Apr 18 '13 at 18:27
  • I never said we pass entities. I said we pass viewmodels: "...different viewmodels based on ...". Argumentative much? – Bobby B Apr 18 '13 at 19:08
1

If you want some static analysis tool, you my look on CodeContracts available also via Visual Studio Gallery + there are other tools (payed, as much as I'm aware of).

The main idea is: you define via attributes conditions in the code and that conditions are validate before binary generation, or at runtime by calling associated method. But considering fact that you're not allowed or can not change the code that uses that function, may be

Roslyn would be more suitable for you. So you can investigate your AST and find out if there are some calls on that non desirable function.

Note: depends on architecture it can become fairly complex issue, but, by the way these are options that you can consider imo.

Tigran
  • 61,654
  • 8
  • 86
  • 123
0

I am not sure if this will help, but what I have done to remove HiddenFors in my code is to implement a kind of ViewState. The model is serialized to the client and my model binder is set to deserialize this. I put up the code at https://github.com/willseitz/ModelViewState. This might help you come up with a general solution.

WillSeitz
  • 75
  • 1
  • 2
0

Erik is right, it is supposed to work like that when doing HTTP POST. Here is a good explanation: Blog link. The blog suggests several solutions including yours Bobby B :-)

Quote from the blog: "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. ... 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."

Thanks to nemesv and Rhys Stephens (stackoverflow link)

Community
  • 1
  • 1
Sigja
  • 93
  • 2
  • 5