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: 5,970 | Comments: 44,493

filter by tags archive

Refactoring toward frictionless & odorless codeA 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?

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

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

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

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

@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

Hence the need for DTOs

Andy Knight

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

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

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

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

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

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

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. RavenDB On Linux–Status Update - 2 hours from now

There are posts all the way to Jul 31, 2015

RECENT SERIES

  1. Production postmortem (5):
    29 Jul 2015 - The evil licensing code
  2. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
  3. API Design (7):
    20 Jul 2015 - We’ll let the users sort it out
  4. What is new in RavenDB 3.5 (3):
    15 Jul 2015 - Exploring data in the dark
  5. The RavenDB Comic Strip (3):
    28 May 2015 - Part III – High availability & sleeping soundly
View all series

RECENT COMMENTS

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats