Ayende @ Rahien

Refunds available at head office

Refactoring toward frictionless & odorless code: A broken home (controller)

Originally posted at 3/30/2011

Previous, our HomeController looked like this:

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

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

My model is defined as:

image

Remember that the previous post we have changed the session management from the request scope to the action scope?

Well, that meant that we have just broken this codeā€¦

But can you see how?

Comments

Mike Minutillo
04/09/2011 09:51 AM by
Mike Minutillo

The Json serializer will visit the Users property and trigger it's lazy load and this will happen outside of the action (and no session now). This means that the developer has to explicitly include or exclude this data now (which is good!)

Daniel Hoelbling
04/09/2011 10:25 AM by
Daniel Hoelbling

Like Mike said, he has not reduced the object to what it should actually be to the user..

I like using anonymous types in that case (or IEnumerable <t.Select() for that matter) to feed to the Json serializer..

return Json(new { blog.Id, blog.Title, blog.Subtitle, blog.CreatedAt });

greetings

Nick Delany
04/09/2011 10:35 AM by
Nick Delany

Is it? Why have lazy loading then? It sounds like the end result of what you are saying is that lazy loading is not appropriate in a web application, but that doesn't fit with what I've read here before.

Mike Minutillo
04/09/2011 11:49 AM by
Mike Minutillo

@Nick - Lazy loading and serializers is the problem. Imagine if the Users "Blogs" property also lazy loaded (and it might). It is very easy to end up sending your whole data store over the wire this way.

Jean
04/09/2011 12:58 PM by
Jean

Hence the need for DTOs

Andy Knight
04/09/2011 04:14 PM by
Andy Knight

The action isnt referencing the newly created NHibernateActionFilter. Therefore the CurrentSession reference will return null.

Simon Bartlett
04/09/2011 05:15 PM by
Simon Bartlett

If there's a two way relationship between Blog and User, then there's a circular reference - which will make the JSON serialiser blow up. Unless you use ScriptIgnoreAttribute on one/both ends.

Simon Bartlett
04/09/2011 05:22 PM by
Simon Bartlett

In regards to my previous comment;

When using the new action filter it'll blow up for the reason given by others above.

I was just pointing out an issue that was there before the action filter was implemented.

Scooletz
04/09/2011 05:41 PM by
Scooletz

The good part about it is that the code uses standard Json method which returns a JsonResult. The serialization problem can be easly mitigated with a global action filter (run as the last before executing the result), which OnActionExecuted will take the data passed to the result and replace the result object with a proper one. The other way is the way you described it guys: a specific class for a particular case.

Linkgoron
04/09/2011 07:43 PM by
Linkgoron

While it's true that you've broken this action (technically), it's just pointing to a bigger problem, which is what I believe you're trying to point out.

While the technical problem would be fixed with a simple lazy-loading fix, there's a huge potential problem here for sending out your passwords and user names!

You're transferring your domain objects outside of your application, which is always a bad practice. This is exactly the place for a DTO. Returning domain objects is just asking for security problems. returning passwords, user names and other properties that may show users option for over-posting etc.

Linkgoron
04/09/2011 08:05 PM by
Linkgoron

In addition to my last comment:

Oh, and not to mention the fact that you also have the potential problem of sending your whole database over the wire

Comments have been closed on this topic.