Ayende @ Rahien

It's a girl

Reviewing OSS Project: Whitboard 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.

Comments

Simon Bartlett
03/10/2011 01:20 PM by
Simon Bartlett

What should we be doing instead of using action filter attributes for authorization?

Rob Ashton
03/10/2011 02:13 PM by
Rob Ashton

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.

Alex K
03/10/2011 06:45 PM by
Alex K

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.

Daniel
03/10/2011 08:56 PM by
Daniel

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

Jason
03/10/2011 09:14 PM by
Jason

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??

Ayende Rahien
03/10/2011 09:17 PM by
Ayende Rahien

Jason,

I would use a global action filter, and the ability to filter stuff out, yes.

joe
03/10/2011 09:17 PM by
joe

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.

Paul Wideman
03/10/2011 10:13 PM by
Paul Wideman

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...

Ayende Rahien
03/10/2011 10:21 PM by
Ayende Rahien

Paul

But to do this, you'd have to assume that

YAGNI

I would like to see how you can implement a way to globally use transactions while allowing you to opt out

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.

Paul Wideman
03/10/2011 11:02 PM by
Paul Wideman

Have a Session property on the base controller, on first access for the action, create the transaction.

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).

Dave Mertens
03/10/2011 11:05 PM by
Dave Mertens

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.

Ayende Rahien
03/11/2011 12:04 AM by
Ayende Rahien

Paul,

a) ambient session

b) don't use a repository

c) do a lazy initialization of the session at the UoW level.

Harry
03/11/2011 01:40 AM by
Harry

How about modelling the domain properly?

var posts = getCurrentUser().ThrowIfNull <authexception()

  .AccessibleBoards.SingleOrDefault(b => b.Id == boardId).ThrowIfNull

<notfoundexception()

.LatestPosts();

no need for attributes.

Harry
03/11/2011 01:42 AM by
Harry

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.

Walter Poch
03/11/2011 02:08 AM by
Walter Poch

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.

jalchr
03/11/2011 09:27 AM by
jalchr

For the Authorize attribute control, use this

www.codeproject.com/.../FluentFltrsASPNETMVC3.aspx

public static void RegisterGlobalFilters(GlobalFilterCollection filters,

               FluentFilterCollection fluentFilters)

{

filters.Add(new HandleErrorAttribute());


           fluentFilters.Add

<authorizeattribute(a =>

        {

            a.Require(new AreaFilterCriteria("Admin"))

                .Or(new ControllerFilterCriteria("Home"))

                .Or(new ControllerFilterCriteria("Client"))

                .Or(new ControllerFilterCriteria("Account"))

                .Or(new ControllerFilterCriteria("Transaction"));


            // Exclude option

            a.Exclude(new ControllerFilterCriteria("Account")).And(new ActionFilterCriteria("Logon"));

        });


});

}

Ayende Rahien
03/12/2011 08:43 AM by
Ayende Rahien

Harry,

You now have loaded ALL of the user's boards, what if he have access to all of them?

Harry
03/12/2011 10:56 PM by
Harry

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?

Ayende Rahien
03/13/2011 07:28 AM by
Ayende Rahien

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.

Harry M
03/13/2011 12:32 PM by
Harry M

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.

Comments have been closed on this topic.