ActiveRecord, Caching, and the Single Responsibility Principle

Josh Clayton

I was working on a messaging system earlier this week and noticed a pretty tight coupling between two classes.

class Message < ActiveRecord::Base
  belongs_to :author, :class_name => 'User'
  belongs_to :conversation, :touch => true

  after_create :author_reads_conversation
  after_save :update_answered
  after_save :update_last_student_posted_at
  after_save :update_last_post_at
  after_save :update_participants
  after_save :update_messages_count

  # ...

  private

  def update_answered
    if conversation.kind == Conversation::QUESTION || conversation.kind == Conversation::ASSIGNMENT
      if author.instructor?
        conversation.update_attribute(:answered, true)
      else
        conversation.update_attribute(:answered, false)
      end
    end
  end

  def update_last_post_at
    conversation.update_attribute(:last_post_at, self.created_at)
  end

  def update_last_student_posted_at
    if author.student? || conversation.last_student_posted_at.nil?
      conversation.update_attribute(:last_student_posted_at, self.created_at)
    end
  end

  def update_participants
    participants = {}
    conversation.contributors.each{|user| participants[user.id] = user.profile.full_name }
    conversation.update_attribute(:participants, participants.to_json.to_s)
  end

  def update_messages_count
    conversation.update_attribute(:messages_count, conversation.messages.count)
  end

  def author_reads_conversation
    conversation.read_by!(author)
  end
end

The first thing I noticed was that all of these callbacks are actually interacting with the conversation, not the message. A message shouldn’t be responsible for updating a handful of attributes on another model! The second thing I noticed was that Message#update_messages_count could actually be handled by a counter_cache.

My gut reaction was to move all of this onto Conversation and call that one method in a callback on Message. Why? I want to call one method that handles all the data caching from a conversation’s standpoint.

Based on the conditionals, it looked like I could get away with passing the author and be done with it. After pondering how to test that (it would’ve been pretty nasty), I decided to extract it into a separate class. With a seperate class, I can then stub methods on conversation (because stubbing the system under test is evil) as well as the message.

class ConversationCacher
  def initialize(conversation, message)
    @conversation = conversation
    @message      = message
  end

  def run
    update_last_posted_at
    update_last_student_posted_at if @message.author.student?
    update_answered               if @conversation.answerable?
    cache_participants_as_json
    save_conversation
    author_reads_conversation
  end

  private

  def update_last_posted_at
    @conversation.last_post_at = @message.created_at
  end

  def update_last_student_posted_at
    @conversation.last_student_posted_at = @message.created_at
  end

  def cache_participants_as_json
    @conversation.participants = participants_as_json.to_s
  end

  def update_answered
    @conversation.answered = @message.author.instructor?
  end

  def author_reads_conversation
    @conversation.read_by!(@message.author)
  end

  def save_conversation
    @conversation.save(:validate => false)
  end

  def participants_as_json
    @conversation.contributors.inject({}) {|result, user| result[user.id] = user.profile.full_name; result }.to_json
  end
end

This class deals with all attribute caching on the Conversation. It’s now incredibly easy to test:

describe ConversationCacher, "#run" do
  let(:conversation) { create(:conversation) }
  let(:message)      { create(:message) }
  let(:student)      { create(:student) }
  let(:instructor)   { create(:instructor) }

  subject { ConversationCacher.new(conversation, message) }

  it "updates conversation#last_post_at" do
    subject.run
    conversation.last_post_at.should == message.created_at
  end

  context "when the message author is a student" do
    before { message.author = student }

    it "updates conversation#last_student_posted_at" do
      subject.run
      conversation.last_student_posted_at.should == message.created_at
    end
  end

  context "when the message author is an instructor" do
    before { message.author = instructor }

    it "does not update conversation#last_student_posted_at" do
      subject.run
      conversation.last_student_posted_at.should be_nil
    end
  end

  # ...
end

Here’s what Message looks like now:

class Message < ActiveRecord::Base
  cattr_writer :cacher

  def self.cacher
    @@cacher || ConversationCacher
  end

  belongs_to :author, :class_name => 'User'
  belongs_to :conversation, :touch => true, :counter_cache => true
  has_many :attachments, :dependent => :destroy

  after_create :cache_data_on_conversation

  private

  def cache_data_on_conversation
    self.class.cacher.new(conversation, self).run
  end
end

Notice that I created a Message.cacher class method. If unset, it’ll return my ConversationCacher. Now I won’t have to stub ConversationCacher.new to make sure that my cacher works properly.

describe Message, "after save" do
  let(:author)       { Factory(:user) }
  let(:conversation) { Factory(:conversation) }
  let(:cacher_class) { stub("cacher", :new => cacher) }
  let(:cacher)       { stub("cacher instance", :run => true) }
  subject            { Factory.build(:message, :author => author, :conversation => conversation) }

  before { Message.cacher = cacher_class }
  after  { Message.cacher = nil }

  it "triggers the conversation data be cached for query optimizations" do
    subject.save
    cacher_class.should have_received(:new).with(conversation, subject)
    cacher.should have_received(:run)
  end

  it "caches only on create" do
    2.times { subject.save }
    cacher_class.should have_received(:new).with(conversation, subject)
    cacher.should have_received(:run).once
  end
end

describe Message, ".cacher" do
  after { Message.cacher = nil }

  context "when overridden" do
    let(:cacher) { stub("my awesome cacher") }
    before       { Message.cacher = cacher }
    it           { Message.cacher.should == cacher }
  end

  context "when not set" do
    before { Message.cacher = nil }
    it     { Message.cacher.should == ConversationCacher }
  end
end

One-line methods, small contexts in our tests, and an easy understanding of what’s happening after a message is created. I’d call this a successful refactor. The Message now doesn’t care whatsoever about what data is cached, since that logic is on ConversationCacher. If more data needs to be cached on the conversation when a message is created, it’s immediately obvious where that logic is handled.

In my experience, after_* callbacks on Rails models seem to have some of the most tightly-coupled code, especially when it comes to models with associations. Do yourself a favor and decouple some code!