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 am going to discuss the SELECT N+1 issue that exists in this method.

Can you see the issue? It is actually hard to figure out.

Yep, it is in the Mapper.Map call, for each post that we return, we are going to load the post’s owner, so we can provide its name.

So far, I have had exactly 100% success rate in finding SELECT N+1 issues in any application that I reviewed. To be fair, that also include applications that I wrote.

And now we get to the real point of this blog post. How do you fix this?

Well, if you were using plain NHibernate, that would have been as easy as adding a Fetch call. But since this application has gone overboard with adopting repository and query objects mode, it is actually not an issue of just fixing this.

Welcome to the re-architecting phase of the project, because your code cannot be fixed to work efficiently.

I’ll discuss this in more details in my next post…

Originally posted at 3/8/2011

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 want to focus on a very common issue that I see over & over. The problem is that people usually don’t notice those sort of issues.

The problem is quite simple, there is no limit to the amount of information that we can request from this method. What this mean is that we can send it 1900-01-01 as the date, and force the application to get all the posts in the board.

Assuming even a relatively busy board, we are talking about tens or hundreds of thousands of posts that are going to be loaded. That is going to put a lot of pressure on the database, on the server memory, and on the amount of money that you’ll pay in the end of the month for the network bandwidth.

There is a reason why I strongly recommend to always use a limit, especially in cases like this, where it is practically shouting at you that the number of items can be very big.

On the next post, we will analyze the SELECT N+1 issue that I found in this method (so far, my record is 100% success in finding this type of issue in any application that I reviewed)…

Originally posted at 3/8/2011

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 want to focus on the usage of AutoMapper, in the form of Mapper.CreateMap() call.

It was fairly confusing to figure out why that was going on there. In fact, since I am not a regular user of AutoMapper, I assumed that this was the correct way of doing things, which bothered me. And then I looked deeper, and figured out just how troubling this thing is.

Usage of Mapper.CreateMap is part of the system initialization, in general. Putting it inside the controller method result in quite a few problems…

To start with, we are drastically violating the Single Responsibility Principle, since configuring the Auto Mapper really have very little to do with serving the latest posts.

But it actually gets worse, Auto Mapper assumes that you’ll make those calls at system initialization, and there is no attempt to make sure that those static method calls are thread safe.

In other words, you can only run the code in this action in a single thread. Once you start running it on multiple thread, you are open to race conditions, undefined behavior and plain out weirdness.

On the next post, I’ll focus on the security vulnerability that exists in this method, can you find it?

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.

I am currently giving a course, and one of the things that we do during the course is put an OSS project on the board and analyze it. The project for this course is Whiteboard Chat Project.

Overall, it seems to be a nice project, there are some problems, but most of them are “rich men’s problems”, the kind of problems that are sort of good to have. That said, I intend to write a series of blog posts on the following 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);
}

This method is a pretty good example of a multitude of anti patterns and other issues that annoys me.

That said, I would like to clarify that I am merely using this method as an example of some bad issues, I wouldn’t want to give the impression that this is any sort of attack of the project or its authors. I have literally looked at the project for the first time today, and I haven’t even checked who the authors are.

More on that, in the next posts, and in the meantime, I’ll let you figure out how many issues there are there that I am going to talk about…