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:
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:
- (12 Apr 2011) What about transactions?
- (11 Apr 2011) Getting rid of globals
- (10 Apr 2011) The case for the view model
- (09 Apr 2011) A broken home (controller)
- (08 Apr 2011) Limiting session scope
- (07 Apr 2011) Hiding global state
- (06 Apr 2011) The baseline
Comments
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!)
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
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.
@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.
Hence the need for DTOs
The action isnt referencing the newly created NHibernateActionFilter. Therefore the CurrentSession reference will return null.
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.
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.
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.
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.
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