+972 52-548-6969
Reviewing OSS ProjectWhitboard Chat–overall design
As a reminder, I am reviewing the problems that I found while reviewing the Whiteboard Chat project during one of my NHibernate’s Courses. Here is the method:
[Transaction] [Authorize] [HttpPost] public ActionResult GetLatestPost(int boardId, string lastPost) { DateTime lastPostDateTime = DateTime.Parse(lastPost); IList<Post> posts = _postRepository .FindAll(new GetPostLastestForBoardById(lastPostDateTime, boardId)) .OrderBy(x=>x.Id).ToList(); //update the latest known post string lastKnownPost = posts.Count > 0 ? posts.Max(x => x.Time).ToString() : lastPost; //no updates Mapper.CreateMap<Post, PostViewModel>() .ForMember(dest => dest.Time, opt => opt.MapFrom(src => src.Time.ToString())) .ForMember(dest => dest.Owner, opt => opt.MapFrom(src => src.Owner.Name)); UpdatePostViewModel update = new UpdatePostViewModel(); update.Time = lastKnownPost; Mapper.Map(posts, update.Posts); return Json(update); }
In this post, I’ll just cover some of the easy to notice issues:
The method name is GetLatestPost, but it is actually getting the latest posts (plural). Yes, it is minor, but it is annoying.
Action filter attributes for authorization or transactions shouldn’t be there. To be rather more exact, the [Authorize] filter attribute shows up on all the methods, and transactions with NHibernate should always be used.
Using them as attributes means that you can select to use them or not (and indeed, there are some actions where the [Transaction] attribute is not found).
Stuff that is everywhere should be defined once, and you don’t have to re-define it all over the place.
More posts in "Reviewing OSS Project" series:
- (15 Mar 2011) Whiteboard Chat–The Select N+1 issue
- (14 Mar 2011) Whiteboard Chat–Unbounded Result Sets and Denial of Service Attacks
- (11 Mar 2011) Whiteboard Chat–setup belongs in the initializer
- (10 Mar 2011) Whitboard Chat–overall design
- (09 Mar 2011) Whiteboard Chat
Comments
What should we be doing instead of using action filter attributes for authorization?
Simon - I'd imagine if the Authorize attribute appears on all methods, then you should be by default requiring authorization by applying a behaviour across all actions, and then turning it off for the specific actions that allow anonymous access.
It is a rich man's problem in that it's not really a big deal, unless a developer forgets to put it on an action - but you should be applying configuration to the exception of the rule, not as common practise to everything.
So, how do we fix this? I don't know if it's possible to put [Authorize] on the whole type, but even if it was, I don't think there is an [DontAuthorize] tag to exclude specific methods. I point this out even though I'm not involved in that project, I still hate it when people complain about things and don't offer alternatives. I mean if I knew how to do it better, I would've. If you are going to complain, tell me how I could've done better. Otherwise it's just whining, which in itself is even more annoying that your annoyance an perceived issue.
Furthermore, I have no clue what you mean about Transaction. I cannot parse the meaning out of that part of the post.
Alex,
The issue with [Transaction] is that all database access including reads should be in a transaction so specify it in one place instead of every time that you create an action method.
See this post:
ayende.com/.../...transactions-is-discouraged.aspx
Ayende,
I would like to see how you would rewrite this. I assume you're talking about using an authorize filter with an "opt out" ability that is added globally. Would you do the same thing for all transactions? I have a ton of Actions that don't use NHibernate (they talk to SOLR/Lucene/etc.) and applying the transaction attribute globally (even with the ability to opt out) sounds like a bit of a nightmare.
Any chance of some code to illustrate how this should be done??
Jason,
I would use a global action filter, and the ability to filter stuff out, yes.
I use Transaction attributes because some actions don't use NH. Maybe it'd be easier and less error prone to make a [NoTransaction] attribute for these rare methods....hmmm.
Authorization: The project uses MVC2, so global action filters (which were added with MVC3) aren't available. The only way this could be improved upon would be to apply the Authorize attribute to the HomeController and BoardController classes themselves instead of each method, or perhaps create a base class, apply the attribute there, and inherit from the base class. And that is only applicable because every action method in these two controllers is authorized. But to do this, you'd have to assume that you will never implement any role support where you'd want to authorize only specific roles for certain action methods. Obviously there are no roles in use here, but that's quite an assumption. The only other choice would be to implement a custom global filter mechanism, which is obviously outside the scope of this small project. The authors made the right decision.
Transaction: Obviously they should have used transactions everywhere the session is used, but it appears that this knowledge still is not ubiquitous. But that's my only complaint. Since they do not have any action methods that don't use the session, on controllers that require repository constructor parameters (and therefore cause a session to be created), then I think they could get away with always opening a transaction at the point the session is created (or apply the Transaction method at the class level). But that is only true in this specific case. If you had an action method on either the Home or Board controller that didn't use the session, then you'd create and open a transaction for no reason. They're already doing this for the session, but I think that's OK for session, not OK for transaction, since I believe opening a transaction is a more expensive operation.
Otherwise, I would like to see how you can implement a way to globally use transactions while allowing you to opt out of them on a per-action-method basis. Since the session and therefore transaction would have to be created when the controller is created (given the architecture of the app), I don't see how a NoTransaction filter could be implemented in this case. I suppose it could be implemented with MVC3's global filters - the global Transaction filter could determine whether a NoTransaction filter was also applied (maybe?) and forego its processing if so. But that doesn't apply here since it's MVC2.
Maybe I'm missing something...
Paul
YAGNI
Have a Session property on the base controller, on first access for the action, create the transaction.
Have a global filter that check that session/transaction and handles it correctly on action exit.
How would the repository access the session? Given your preference for using NH more directly and avoiding repositories entirely, you're probably thinking along those lines. But I have a S#arp based app that is similar to this one, so if there's a better way to manage the session/transaction while staying with the repository pattern, I'd like to hear about it. You could give the session to the repository in the controller constructor, but that's sort of defeating the purpose of creating the repositories with the container and injecting them into the controller.
I'd love to see an MVC sample app from you demonstrating your ideas on proper patterns, both for NH and for Raven (and on more than just managing sessions/transactions).
If it were me, I would also move the AutoMapper initialization outside this method. Somehow I got the feeling this method is called quite a lot.
And why is the ViewModel for a Get method called Update?
To nitpick any further the comment for lastKnowPost assignment should be '//Get (date)Time of latest post'
It's a local variable, so there isn't any update performed. The place where the created ViewModel is used, that one performs the update.
Paul,
a) ambient session
b) don't use a repository
c) do a lazy initialization of the session at the UoW level.
How about modelling the domain properly?
var posts = getCurrentUser().ThrowIfNull <authexception()
<notfoundexception()
no need for attributes.
repost as my braces got stripped:
How about modelling the domain properly?
var posts = getCurrentUser().ThrowIfNullAuthException
.AccessibleBoards.SingleOrDefault(b => b.Id == boardId).ThrowIfNullNotFoundException
.LatestPosts();
no need for attributes.
Also isn't the AutoMapper creation wrong? They are creating the mapping for the types Post->PostViewModel on every call, they should be initialized only once on the application StatrtUp as I known.
Paul,
I think you can use MVCTurbine in MVC2 to register global filters.
See http://www.mvcturbine.com/
For the Authorize attribute control, use this
www.codeproject.com/.../FluentFltrsASPNETMVC3.aspx
public static void RegisterGlobalFilters(GlobalFilterCollection filters,
{
<authorizeattribute(a =>
}
Harry,
You now have loaded ALL of the user's boards, what if he have access to all of them?
A problem with any lazy loaded mapped property no? And of course, you've only loaded half of their properties (due to batching - the desired item will be loaded in batch n/2 :)
If this is a problem (and in some systems this isn't a huge problem e.g. when the number of boards per user are typically low) - then you could make the property an IQueryable so the SingleOrDefault is performant, or even just create a user.GetAccessibleBoard(..) method with the query inside it. Or for some situationa
The thing I really wanted to get at is that if you walk the domain, and make the domain a bit cleverer than just a bag of persistent fields, you can reduce some of the framework code you have to write - e.g. authorization not being a problem if you walk from the current user.
When your controllers are talking straight to the repos, its very easy for someone to skip some of the domain rules that ought to have been checked, and a lot harder to be sure that any new rules you add have been added in all the places they should.
On a slightly seperate note, if you're really into being clever, you can get rid of the need for the httppost attribute by writing an interceptor for nh (and all other writeable systems you have) that will assert the current request was a post the moment some data is written - you should be in a post for any thing that makes changes. Why make it dependent on developers remembering to mark up methods?
Harry,
Yes, that is a problem that occur with any collection property, and one of the reasons that I suggest being cautious is that it is an incredibly common mistake with drastic results.
Making a property an IQueryable means that you are mixing domain & persistence concerns. Just make the user make the query directly.
The domain cannot be "smarter" about persistence. It is supposed to be persistence ignorant, after all.
I don't want to make too big a thing out of this, but all I'm trying to suggest is that wrapping the calls into meaningful properties related to the entities rather than external objects can make the domain easier to understand and work with, and result in less business logic ending up in UI concerns like controllers.
The domain doesn't need to know about the precise mechanism used for persistence - it can talk to the abstractions we have like named queries etc. The problem for me is when the UI layer can make calls straight to these named queries by dint of having a reference to the persistence code and can skip the execution of vital business rules by omission.
Comment preview