Reviewing OSS ProjectWhiteboard Chat–setup belongs in the initializer

time to read 3 min | 471 words

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:

public ActionResult GetLatestPost(int boardId, string lastPost)
    DateTime lastPostDateTime = DateTime.Parse(lastPost);
    IList<Post> posts = 
        .FindAll(new GetPostLastestForBoardById(lastPostDateTime, boardId))

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

More posts in "Reviewing OSS Project" series:

  1. (15 Mar 2011) Whiteboard Chat–The Select N+1 issue
  2. (14 Mar 2011) Whiteboard Chat–Unbounded Result Sets and Denial of Service Attacks
  3. (11 Mar 2011) Whiteboard Chat–setup belongs in the initializer
  4. (10 Mar 2011) Whitboard Chat–overall design
  5. (09 Mar 2011) Whiteboard Chat