Rule of thumb: Never have [Ignore] properties in a ContentType/model

This applies to WebForms and MVC scenarios alike, but it’s mostly MVC users that are tempted in doing it: Never put request specific data into the ContentType model class! It may sound obvious to some, and not so obvious to others. Regardless, it’s an easy mistake to make. Worse is, doing it may survive development, QA, UAT… But NOT production use!

Just imagine the panic after just going live with your site and you get user reports that they occasionally see other user’s data, such as order confirmation pages, profile pages… Or that pages sometimes contain a mash from multiple users’ requests.

This can happen if you store request-specific data into the model that you pass onto the view, if the model class instance is shared between multiple threads.

This is a pretty typical example of a page base class. It’s used as the starting point creating more specialized page types.

When it goes wrong

Someone gets the assignment to create a page to display a product. They create the Controller to handle a new ProductPage:

And then the model is updated so it can store the extra data:

And lastly, a view that uses the model that they pass in:

Satisfied with the creation, it’s tested and checked in.

What’s wrong with this?

The ProductPage  instance passed into the Controller’s action method is cached. That sounds good and fast, right?

bapelsin_61469224Yes, but it also means it is shared. All requests in your application will get the same copy of it. The change that the controller makes to it on line 12 (  model.CurrentProduct = ... ) also changes the current product for all other threads including those currently in the stage of rendering the view. Halfway through the rendering, another thread just simply changes the CurrentProduct property into something else, and suddenly you have a page that shows the product name “Banana” and the description of an orange!

The reason it’s not spotted during test is that no two requests are running simultaneously, neither under the development nor QA phase. Later tests are unlikely to have enough traffic for it to happen, at least not frequent enough for someone to pay attention. But when the code goes live, it seems to happen constantly!

How it’s done right… or not wrong.

It seems that the primary motivation for doing it the wrong way is to save the application from one more class. Instead, add a class to carry the page and whatever additional information you need into the View. This class is simply acts as a light weight data carrier. Correspondingly, you would implement your Controller’s Action method along these lines, instead:

and then access the page properties via the ProductPageViewModel:

I have done exactly this, the site is live and now I’m in panic!!1

Okay, calm down. There’s a hack you can implement before you take the time to properly refactor your code.

Just promise me you won’t be leaving this code around for long…

 

5 thoughts on “Rule of thumb: Never have [Ignore] properties in a ContentType/model”

  1. Cool, I have to refactor a 7.5 based site. When on going live, In a load balanced site Properties defined in above mentioned way was returning null occasionally. (long story short) Lesson was learned, “Never have [Ignore] properties in a ContentType”.

  2. “Saving the application from one more class” => You shall rather have many small, simple classes than a few big do-it-alls 🙂

    Makes things easier to change, easier to keep track of, less risky to modify….

    1. Yes, it was an example of faulty reasoning. I agree with you 🙂

      // object DoStuff(param object[] data)

  3. Thx for sharing. This is a very easy to made mistake. A noted that I always sharing with colleague is to using page view model instead of content data object as view model 🙂

Leave a Reply

Your email address will not be published. Required fields are marked *