ActiveRecord is perhaps the most compelling component of Ruby on Rails. ActiveRecord makes it easy to create a record, or to find one, or even to migrate the database schema. Unfortunately those “find” methods can be a little too seductive. Let’s see why.

Our example project is a basic social network. It will start with profiles and friends, and add from there. We’ve implemented “Profile” and “Friend” features and now our customer asks for “Search by State”. In our “Profiles” controller, we implement:

1 2 3 4 5 class ProfilesController def search @profiles = Profile .find_all_by_state(params[ :state ]) end end

At first, this is wonderful. It does not burden the caller with unnecessary knowledge, and keeps the code simple and clean. It’s also simple to test through either state-based or interaction testing. At the customer’s request, we add an admin listing, a friend list, and the profile page itself. It’s working beautifully.

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 class ProfilesController def search @profiles = Profile .find_all_by_state(params[ :state ]) end def show @profile = Profile .find(params[ :id ]) end end class ProfilesAdminController def index @profiles = Profile .find( :all ) end end class FriendsController def index @user = User .find(params[ :user_id ]) @friends = @user .friends end end

Then our customer comes back and says we need the ability to have private profiles. A profile should only show up if it’s public. We make the changes as follows:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 class ProfilesController def search @profiles = Profile .find_all_by_state_and_public(params[ :state ], true ) end def show @profile = Profile .find(params[ :id ], :conditions => { :public => true }) end end class ProfilesAdminController def index @profiles = Profile .find( :all ) end end class FriendsController def index @user = User .find(params[ :user_id ]) @friends = @user .friends.find_all_by_public( true ) end end

We know it’s not ‘DRY’, but we move on in the interest of ‘Getting Things Done’. We run our integration tests, but one is failing. A user can no longer view their own profile. After investigating, we realize that in reality the rule should be “A profile should be visible if it’s public or belongs to the current user”.

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 class ProfilesController def search conditions = [ " state = ? and (public = ? or user_id = ?) " , params[ :state ], true , current_user.id ] @profiles = Profile .find( :all , :conditions => conditions) end def show conditions = [ " public = ? or user_id = ? " , true , current_user.id] @profile = Profile .find(params[ :id ], :conditions => conditions) end end class ProfilesAdminController def search @profiles = Profile .find( :all ) end end class FriendsController def index @user = User .find(params[ :user_id ]) conditions = [ " public = ? or user_id = ? " , true , current_user.id] @friends = @user .friends.find( :all , :conditions => conditions) end end

This has gotten out of hand. It’s just not ok to leave SQL lying around in our controller, even if it’s just condition arrays. The caller should specify intent, but not worry about details like privacy.

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 class Profile < ActiveRecord :: Base def self .find_all_by_state(state, current_user) conditions = [ " state = ? and (public = ? or user_id = ?) " , state, true , current_user.id ] find( :all , :conditions => conditions) end def self .find_all_for_admin find( :all ) end def self .find_by_id(id, current_user) conditions = [ " public = ? or user_id = ? " , true , current_user.id] find(id, :conditions => conditions) end def self .find_friend_profiles_of(user, current_user) conditions = [ " public = ? or user_id = ? " , true , current_user.id] user.friend_profiles.find( :all , :conditions => conditions) end end class ProfilesController def search @profiles = Profile .find_all_by_state(params[ :state ], current_user) end def show @profile = Profile .find_by_id(params[ :id ], current_user) end end class ProfilesAdminController def search @profiles = Profile .find_all_for_admin end end class FriendsController def index @user = User .find(params[ :user_id ]) @friends = Profile .find_friend_profiles_of( @user , current_user) end end

Now all of the actions that need profiles can find them safely by calling one of our methods, and the privacy logic is handled for us. Something doesn’t feel quite right, but we’ll try it for a bit to make sure we’re not just suffering from indigestion.

A month later, the customer asks for a different kind of search. Now we need to search by last name. By this time, there’s a second pair on the project, and they implement this feature.

1 2 3 4 5 6 7 8 9 10 11 12 13 class ProfilesController def search_by_state @profiles = Profile .find_all_by_state(params[ :state ], current_user) end def search_by_last_name @profiles = Profile .find_all_by_last_name(params[ :last_name ]) end def show @profile = Profile .find_by_id(params[ :id ], current_user) end end

Their integration test passes and they check it in. After we deploy, the customer complains. It seems the new form of search doesn’t take privacy into account. Why did this happen? Intelligent programmers following project conventions and acting conscientiously will still make these mistakes. Is it a communication breakdown? I contend that it’s a code flaw. When finding rules are implemented, they need to be designed to allow later programmers to forget the details until they need them. The programmers should not be filling their minds with privacy while implementing the controller action. But how?

In my experience, finder logic very often gets complicated very quickly. The responsibility for finding a specific kind of entity is a big deal, and should not be given to a class that already fills another responsibility (persisting profile information). In addition, finder methods should not be implicit, as it leads to the issues outlined above. Instead we use finder objects.

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 class ProfileFinder attr_accessor :current_user def find_all_by_state (state) conditions = [ " state = ? and (public = ? or user_id = ?) " , state, true , @current_user .id ] Profile .find( :all , :conditions => conditions) end def find_all_by_last_name (last_name) conditions = [ " last_name = ? and (public = ? or user_id = ?) " , last_name, true , @current_user .id ] Profile .find( :all , :conditions => conditions) end def find_all_for_admin Profile .find( :all ) end def find_by_id (id) conditions = [ " public = ? or user_id = ? " , true , @current_user .id] Profile .find(id, :conditions => conditions) end def find_friend_profiles_of (user) conditions = [ " public = ? or user_id = ? " , true , @current_user .id] user.friend_profiles.find( :all , :conditions => conditions) end end class ProfilesController before_filter :build_profile_finder def search_by_state @profiles = @profile_finder .find_all_by_state(params[ :state ]) end def search_by_last_name @profiles = @profile_finder .find_all_by_last_name(params[ :last_name ]) end def show @profile = @profile_finder .find_by_id(params[ :id ]) end private def build_profile_finder @profile_finder = ProfileFinder .new @profile_finder .current_user = current_user end end

Now when our new pair is asked to implement the ability to find profiles by last name, they know project standards dictate the use of the relevant finder. Their integration test fails since the method does not exist. When they create it, they see the other privacy logic in close proximity, and now have enough information to initiate a conversation with the customer about privacy. We’ve found that conscientious use of finder objects enables us to have very consistent finding logic, which reduces bugs and user confusion. It also simplifies tests, as the tests benefit from a clear scope definition, and are not cluttered by other model logic. It also makes finder rules discoverable as they are now in a centralized location.

We decided that finder objects are allowed to know model fields and conditions, sql, etc. They are also allowed to call with_scope on that model. This all fits with their responsibility of knowing how to find the appropriate model.

Usage Tips

For bonus points, combine this with dependency injection. We do, and we’re loving it. It allows context like the current user to be injected, and then the caller doesn’t even need to know that the method requires the current user, as that’s simply a privacy constraint.

We’ve also discovered that for purposes of authorization rules (are you allowed to see this record?), it helps for the caller to provide intent. If a record otherwise exists but the user does not have permission to follow through with the stated intent, nil is returned.

1 2 profile_finder.find_by_id(id, :for => :show ) profile_finder.find_by_id(id, :for => :update )

I may be able to see my friend’s profile, but I can’t update it. Finder objects already need some authorization information to restrict queries. It makes sense to block out access at the data layer. If I can’t fetch the record, I can’t update it. My code can be required to provide an intent with the :for attribute; you may rig your finder to raise an error if no intention is indicated.

Sometimes, even just the act of determining the permissions of the current user and the object at hand can be complicated, and given that you may need to apply action-oriented permissions for an entity type in more than one place in your code, it makes sense to implement a Permitter (in this case, a ProfilePermitter). The topic of permitters may warrant another blog post.