Close
Glad You're Ready. Let's Get Started!

Let us know how we can contact you.

Thank you!

We'll respond shortly.

LABS
Stop leaking ActiveRecord throughout your application

Extending ActiveRecord::Base leaks a powerful API throughout an application which can lead to tempting code which breaks good design. Take the classic blog example where you may want to retrieve the latest posts by a given author. You may have seen, or even written code that gets the dataset you need straight into the controller or view:

Post.where(author_id: author_id).limit(20).order("created_at DESC").each { ... }

For me this is a design violation as well as breaking the “Law of Demeter”[Edit: Current Pivot Adam Berlin and former Pivot John Barker pointed out that chaining with the same object was not a Demeter violation]. The example above tells me structure of the schema that the calling class has no business knowing. It also makes testing using stubs ugly and encourages testing against the database directly. A test would have to chain three methods to stub a return value. It’s brittle, as in it’s susceptible to breaking due to changes outside of the class. For me it also fails from a narrative perspective in that it doesn’t succinctly reveal the intent of this part of the application.

If we were testing this and attempting to use stubs, we’d have to write something like the below. You can see how this is at best cumbersome, but also fragile.

where = stub(:where)
limit = stub(:limit)
order = stub(:order)

Post.stub(:where).with(author_id: author_id) { where }
where.stub(:limit).with(20) { limit }
limit.stub(:order).with("created_at DESC").and_yield(post1, post2, post3)

You may be forgiven for thinking you could chain the stubs like below, but the arguments are ignored and this just serves to highlight the breaking of the ‘Law of Demeter’.

Post.stub_chain(:where, :limit, :order).and_yield(post1, post2, post3)

I’d much rather see that as a message to the Post class.

def self.latest_for_author id
  where(author_id: id).limit(20).order("created_at DESC")
end

Post.latest_for_author(1)

If there were variations of the limit and perhaps offset, they can be passed as option parameters of as an options hash:

def self.latest_for_author id, limit = 20, offset = 0
  where(author: id).limit(limit).offset(offset).order("created_at DESC")
end

Post.latest_for_author(1)
Post.latest_for_author(1, 20, 0)

or

def self.latest_for_author id, options
  limit = options[:limit] || 20
  offset = options[:offset] || 0
  where(author: id).limit(limit).offset(offset).order("created_at DESC")
end

Post.latest_for_author(1, offset: 20)

In order to get the dataset the call looks like the following, and I think is more informative than using the ActiveRecord DSL directly.

Post.latest_for_author(author_id).each { ... }

Testing is also easier, as it puts more emphasis on the messages being sent to objects rather than a chain of calls having to be correct.

Post.should_receive(:latest_for_author).with(1).and_yield(post1, post2, post3)

There are a few advantages to this refactor:

  • Only the Post class knows about the schema
  • Any changes to the implementation of what latest_for_author are encapsulated in one place
  • The method describes the intent more than the implementation
  • Stubbing in the tests are easier as there is one clear dependency
  • Testing the database is encouraged only in the class hitting the database

One further refactor could be done here, and that is to move the query logic out of the Post class once more, but this time into a purpose built query Object:

class LatestPosts
  attr_reader :author_id

  def initialize author_id
    @author_id = author_id
  end

  def find_each(&block)
    Post.where(author_id: author_id).limit(20).order("created_at DESC").find_each(&block)
  end

end

Where using the class looks like:

LatestPosts.new(author_id).find_each { ... }

Here’s what Bryan Helmkamp has to say on query objects in his excellent write up on fat ActiveRecord models. Bryan here rightfully points out that once in a single purpose object, they warrant little attention to unit testing. Now is the right time to use the database to ensure the right data set is being returned and that N+1 queries are not being performed. This means that database testing would only occur within the class actually hitting the database and not the rest of application which has a dependency on the database.

All of these techniques discussed serve to improve the design of an application by preventing leaking responsibilities from one class throughout the rest of the application. I’m also not saying that developers shouldn’t be using ActiveRecord or even Rails, but to use the tools responsibly.

Comments
  1. Jared Carroll says:

    Great post Robbie.

    I’m definitely on board with moving queries out of controllers and views and into models.

    I’ve introduced query objects for view-specific queries i.e., queries that would never be used between models. A good technique is to ask yourself “Is this query only for a view?”. However, I do unit test, more like mini-integration test, these query objects even though Bryan Helmkamp’s post says that he doesn’t.

  2. Robbie Clutton says:

    Hey Jared,

    Thanks for the thumbs up. I’m interested in your use of query objects, do you tend to build lots of them and use them once or have fewer and use them in more places? I’m definitely interested in best ways to move ActiveRecord DSL usage out of everywhere but within classes which extend ActiveRecord::Base.

    Cheers

    Robbie

  3. Jared Carroll says:

    I tend to use few query objects. It’s hard to justify a new class who’s only responsibility is a query. If a query is only used in one place (in a presenter, a Rake task, a service object, etc), then I’ll leave it there until I need it somewhere else. When someone else needs it, then I might experiment with a query object, instead of pushing it into an ActiveRecord.

  4. Robbie Clutton says:

    I buy not wanting to create one off classes, but on the other hand from an object design point of view, being able to send a message to a query object rather than having that embedded elsewhere, say in a view or controller, has it’s benefits. However, I definitely get the refactor on the third reuse pattern.

Post a Comment

Your Information (Name required. Email address will not be displayed with comment.)

* Copy This Password *

* Type Or Paste Password Here *