Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 10 | Comments: 37

filter by tags archive

Refactoring toward frictionless & odorless codeThe case for the view model

time to read 5 min | 814 words

Originally posted at 3/30/2011

Remember, we are running on an action scoped session, and this is the code that we run:

public class HomeController : SessionController
{
    public ActionResult Blog(int id)
    {
        var blog = Session.Get<Blog>(id);

        return Json(blog, JsonRequestBehavior.AllowGet);
    }
}

The error that we get is a problem when trying to serialize the Blog. Let us look again the the class diagram:

image

As you can see, we have a collection of Users, but it is lazily loaded. When the Json serializer (which I use instead of writing a view, since it makes my life easier) touches that property, it throws, because you cannot iterate on a lazily loaded collection when the session has already been closed.

One option that we have is to modify the code for Blog like so:

public class Blog
{
    public virtual int Id { get; set; }

    public virtual string Title { get; set; }

    public virtual string Subtitle { get; set; }

    public virtual bool AllowsComments { get; set; }

    public virtual DateTime CreatedAt { get; set; }

    [ScriptIgnore]
    public virtual ISet<User> Users { get; set; }

    public Blog()
    {
        Users = new HashedSet<User>();
    }
}

I don’t like it for several reasons. First, and least among them, serialization duties aren’t one of the responsibilities of the domain model. Next, and most important, is the problem that this approach is incredibly brittle. Imagine what would happen if we added a new lazily loaded property. Suddenly, unless we remembered to add [ScriptIgnore] we would break everything that tried to serialize a Blog instance.

I much rather use an approach that wouldn’t break if I breathed on it. Like the following:

public class HomeController : SessionController
{
    public ActionResult Blog(int id)
    {
        var blog = Session.Get<Blog>(id);

        return Json(new
        {
            blog.AllowsComments,
            blog.CreatedAt,
            blog.Id,
            blog.Subtitle
        }, JsonRequestBehavior.AllowGet);
    }
}

By projecting the values out into a known format, we can save a lot of pain down the road.

But wait a second, this looks quite familiar, doesn’t it? This is the view model pattern, but we arrived at it through an unusual journey.

I don’t really care if you are using anonymous objects or named classes with something like AutoMapper, but I do think that a clear boundary make it easier to work with the application. And if you wonder why you need two models for the same data, the avoidance of accidental queries and the usage of the action scoped session is another motivator.

More posts in "Refactoring toward frictionless & odorless code" series:

  1. (12 Apr 2011) What about transactions?
  2. (11 Apr 2011) Getting rid of globals
  3. (10 Apr 2011) The case for the view model
  4. (09 Apr 2011) A broken home (controller)
  5. (08 Apr 2011) Limiting session scope
  6. (07 Apr 2011) Hiding global state
  7. (06 Apr 2011) The baseline

Comments

wayne-o

The other reason to do this is security. Imagine doing:

return Json(user, JsonRequestBehavior.AllowGet);

Say helo to the users credentials - I'm not saying it's something most would do but this could easilly be a mistake made by some new bod on the project. Or perhaps a slip in concentration - but the result is the users credentials getting serialized.

You want to control what gets sent down the wire and you want to minimise the risk of something bad happening.

tobi

Also the amount of work needed for separate view models is very low. I usually reuse them as input parameters in post methods.

Linkgoron

I have to agree with wayne and I'll add that while performance (and bug-free software) is extremely important, security is more. lazy loading is only exposing a bigger problem with your architecture,.

Let's say lazy loading worked perfectly and was blazingly fast. You still have a major problem here, as you're not explicilty controlling what your application exposes to the outside world, you expose information that should never get out of your system.

So, it's true that there's a technical problem because of Lazy Loading, yet I wish you'd touch the bigger issue at hand. Looking at this in a OO sort of way, you're basically violating encapsulation.

Scooletz

@Ayende,

have you tried in any of your application implicit solution, by replacing the data of JsonResult in an action filter? I mean using sth like Automapper or your own convention.

Ayende Rahien

Scooletz,

Yuck, that sounds like a recipe for disaster, to tell you the truth.

Adam

The only problem I've found so far with this is that if you are using an IoC container, such as Windsor, you cannot inject classes into your controller that have a dependency on ISession, such as the Rhino Security services. The session isn't opened until after the controller is instantiated so it fails.

You could use a service locator pattern to get around that but that's quite an unpopular approach.

You could also change the Rhino Security services to have a dependency on some sort of ISession provider, what do yo think?

naraga

@Scooletz,

never tried solution like automapper but doesnt it take you back? i mean when you decide for something like viewmodel you are telling that you want to be explicit about what is going to be send.

the only problem you solve with implicit mapping is the lazyloading. in this article ayende is dealing only with the loading problem so it is appropriate to mention your way of mapping but in general you are getting exposed to all kind of aforementioned issues like security etc....

Scooletz

@Ayende,

Even for trivial cases, when all but the proxy loaded data can be passed? I don't agree. Using convention by default, getting code explicit as you need to overwrite it (as you showed omitting Title property).

@naraga

There's no security hole in conventional json serialization for cases where you know, you can go with it. Security checks and so on can be applied to the actions as well. If you write about sending some passwords, etc. - be explicit then.

naraga

@Scooletz,

yeah i was writing about credentials example mentioned by @wayne-o.

Scooletz

@naraga

It'd be very unwise of me, advocating using implicit paradigm in all the cases ;-) What I meant, was rather sth like 20% coverage.

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. Production postmortem: The case of the memory eater and high load - about one day from now
  2. Production postmortem: The case of the lying configuration file - 3 days from now
  3. Production postmortem: The industry at large - 4 days from now
  4. The insidious cost of allocations - 5 days from now
  5. Find the bug: The concurrent memory buster - 6 days from now

And 4 more posts are pending...

There are posts all the way to Sep 10, 2015

RECENT SERIES

  1. Find the bug (5):
    20 Apr 2011 - Why do I get a Null Reference Exception?
  2. Production postmortem (10):
    14 Aug 2015 - The case of the man in the middle
  3. What is new in RavenDB 3.5 (7):
    12 Aug 2015 - Monitoring support
  4. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats