P$
P$

Culling ActiveRecord Callbacks

Save future you some time and effort and use a Service Object instead

Patrick Robertson
January 27, 2014

I recently went on a short rant on the BostonRB Mailing List about avoiding the ActiveRecord Lifecycle events which may or may not be actually related to the question that Denis Haskin posed in the first place. People wanted to view this rant in the form of a blog post and I'm a man that does what the people want.

At Iora we have a rule against introducing ActiveRecord Callbacks to the codebase, but with all of our style related rules they can be broken so long as the developer justifies his/her actions. I think this is generally a healthy way to define practices as we should live and die by getting features to users over practicing the niftiest coding patterns.

Anyways, I'm generally skeptical that adding code that operates within an ActiveRecord Callback is going to to result in a good time. My skepticism is based upon a few characteristics about the general tendencies of code related to AR Callbacks:

  1. AR Callbacks execute each time you perform said callback no matter where you are in the codebase. For create/update events that probably has a side effect of executing on a metric crapton of unrelated test events.
  2. AR Callbacks have eleven distinct events in the callback lifecycle. Knowing many of these events and how they fire in order on a parent/child relationship is often necessary when you start pairing associations and callbacks if you do not want to introduce unintended side effects/create a bunch of bugs.
  3. While it is not totally impossible to separate AR Callbacks from persistence events in your tests, you'll have to jump through a number of hoops in a lot of cases. This turns what would normally be a unit test into an integration test.
  4. It's actually really easy to miss how the business logic is being carried out on a model when all you see in the controller is object.update_attributes(attributes)

Now allow me to convince you point by point!

Unintended side effect factories are probably a poor idea

The reality of web applications is that we really have a finite amount of times that we create/update/destroy an object. As it actually turns out, that number is usually one. ActiveRecord callbacks attempt to DRY that up by giving us a place to ensure we execute some business logic each and every time we persist changes to an ActiveRecord object.

There are two major areas where this goes awry:

  • Conditional Callbacks
  • The flippin' test suite

Conditional Callbacks

When we introduce conditionals to a callback event, we are creating obscurity by shoving distinct business cases in one method. Let's examine some code:

class Article < ActiveRecord::Base
  before_save :update_category_list

  private
  def update_category_list
    if categories.count > 0
      categories_list = categories.collect("") do |cat, list|
        list << "#{cat.name}, "
    else
      category_list = ''
    end
  end
end

In the totally unoptimized and trivial example above I'm updating a cache of the names of categories for each article whenever I save it. There are two use cases there. If I have some categories, I'm building a new list. If I've cleared the categories I need to clear the cache out. If I'm handling each of these business cases, I'd rather have them more explicitly laid out than in an if statement inside a private instance method. Performing an extract method refactoring is only going to get me so far towards being explicit about the business rules.

Testing side-effects

In my integration (and to a lesser extent my unit) test suite I'm going to be creating/updating/deleting objects on a pretty regular basis. The number of times I care about this code executing is probably about one:

class Person <ActiveRecord::Base
  after_save :tweet_to_the_world

  private
  def tweet_to_the_world
    Tweeterific.tweet(self.twitter_handle, 
      "#{name} is Totes McGoats a hottie")
  end
end

Actually, I don't care about that code ever executing. But assuming I did, in the best case I'd have to add a Test Double to my test suite just so that Tweeterific.tweet was a no-op, or I'd probably get rate limited by twitter every time my CI build kicked off. I'd actually just prefer for that method to be called only when I was testing the 'tweet to the world' functionality and avoid it elsewhere.

I'd love to tell you that my unit test suite NEVER EVER reads nor writes to the database. I'd also love to be a billionaire and fly around in a private jet to hang with all my celebrity friends. The reality is that pragmatic development of Rails applications results in occasionally persisting objects to the db. What we can do is make sure that when we have to persist objects that we're not doing a bunch of additional and potentially time consuming tasks.

I save my useless trivia knowledge for stuff that will earn me free beer

I'm never a fan of features that are directly tied to the knowledge of a complicated area of ActiveRecord. It's great if you've source dived and then committed to memory how a has_many: relationship pairs with an after_touch: callback, but I'm going to have to look that crap up each time I look at the code in order to understand the basic premise and order of operations. Let's contrast to wrapping a parent/child relationship in an explicit transaction:

ActiveRecord::Transaction do
  parent.do_something
  children.each { |child| child.do_something_from(parent) }
end

BAM! Now I don't have to be Ernie Fucking Miller to know the order of operations here. Let's also #realtalk for a second. There are areas of ActiveRecord::Associations that work the way they do because of bugs in the code offset each other. Upgrading Rails (even patch releases) is sometimes very fun when you've got associations + callbacks.

I ain't got no time for integration testing in my unit tests

I'm pretty sure that ActiveRecord#save works. I'm pretty confident that when it tells Postgres to commit a record to the db, it's going to happen. Why I need to go ahead and do it about fifty six thousand times in my 'unit' testing suite is beyond me.

Introducing a collaborating object isn't really tough here. Instead of injecting your business logic in the persistence event of a DB record, you can just delegate
to some object that calls save.

class CategoryCacheClearer < Struct.new(:article)
  def call
    article.category_list = ''
    article.save
  end
end

describe CategoryCacheClearer do
  describe '#call' do
    let(:article) { mock(:save, :category_list=) }

    it 'clears the category list'
    it 'calls save on the article'
  end
end

The CategoryCacheClearer only performs commands on the collaborating Article object, so we really don't care about what either category_list= or save does, just that we have indeed made those specific calls which makes our tests really simple because we can use a test double with an appropriate interface to represent the Article in the tests.

If we were mixing commands and queries in our callback, things would be trickier and we'd probably have to get fancier than a simple delegation system.

Intention revealing entry points save future me a lot of code diving

I've spent a lot of time in my days as a Rails programmer doing a deep dive into the relationships of each of the models in my domain to figure out what triggers a field update when the codebase entirely relies on AR callbacks to perform such activities. This is the controller entry point for an AR lifecycle event that ends up touching about 18 different models:

if  article.update_attributes(params[:article])
  redirect_to article_path(article), notice: 'BOOMSHAKALAKA'
end

Here's one where I don't perform any AR callbacks:

if category.update_attributes(params[:category])
  redirect_to category_path(category), notice: 'TROLOLOLOLOLO'
end

Can you tell the difference? Neither can I.

When AR callbacks are abolished -- my code dive consists of one and only one class. As it turns out, when I name a class CategoryCacheClearer and another one CategoryCacheSetter I can more or less reason out what's going to go down without either looking at the tests or looking at the implementation.

My future self wants to hug my past self when I use explicit entry points for business rules. The self between past self and future self writes a lot of code in a number of different applications and has a tendency to destroy his brain cells with beer consumption. Fixing issues and/or extension are much easier when an excellent memory isn't required.

Patrick, you are so smart. How do we get rid of these pesky callbacks??!?!!!?

Getting rid of ActiveRecord Callbacks is generally pretty easy! Follow my three step program below:

First step: Create a Collaborating Object

class TweetToTheWorld < Struct.new(:person)
  extend Forwardable
  def_delegators :person, :twitter_handle, :name

  def call
    Tweetirific.tweet(handle, twitter_message)
  end

  private

  def twitter_message
    "#{name} is Totes McGoats a hottie"
  end
end

Second step: Use it in the Controller

def update
  if person.update_attributes(params[:person])
    TweetToTheWorld.new(person).call
  end
end

Third step: There is no third step.

Go kill some of those now unneeded brain cells with beer.

Wrapping things up

I'm not going to lie to you folks, I vastly enjoy writing code with a lot of collaborating objects over code with coalesced responsibilities. This whole blog post promotes my architectural astronaut agenda. On the other hand, I've enjoyed making hard, fast cash on Rails rescue projects and replacing all the AR callbacks is usually the quickest way to thin out the dreaded God Object.

In all seriousness though, the time/effort spent on creating an intention revealing object is pretty negligible by following the advice outlined above. The code you've got to write is always going to be harder than my contrived samples here, but the reward is the same. People who have to read your code later will be able to extend the functionality without needing to get a Ph.D in ActiveRecord.

comments powered by Disqus