Article summary
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.
has_finder /named_scope is far more elegant to declare findders. See:
http://didcoe.id.au/archives/edge-rails-has_finder-merged-to-edge
Depending on the level of complexity, I think this could be more elegany than has_finder/named_scope. It could also be used in conjunction with has_finder/named_scope. I like the idea of injecting the current user into the finder. I think that you might have equally complicated logic in terms of updates, so it might make sense to refer to the Finder more generically as a Service, so it can handle CRUD.
@Paul, I can’t see how finder objects are more elegant. named_scope is significantly easier to read and probably more compact and reusable. Just in terms of efficiency finder objects don’t make sense. If you try to chain any of those finders together you get multiple queries. named_scope uses with_scope for one query.
The idea with finders is to make it so any chaining required is done inside the finder, to keep it to a single query. Named scopes are a decent solution. However, they still spread the logic a little too far afield.
My current project recently felt this pain. We deal heavily with events. 18 one-week iterations into the project, the customer asked for privacy support for events. We have many controllers that find one or more events. Each one needed to be updated to support privacy. Fixing this was very painful, and it was difficult to prove that every piece of the application respects privacy.
We chose to move to finders, because when all of the application uses the EventFinder, it’s trivial to prove that it respects privacy. In addition, the details are hidden from the caller. Privacy becomes an implicit rule that automatically applies throughout the application, instead of an explicit call that can be neglected.
With that said, for a different project without stringent data access rules, finders may be overkill. Unfortunately, you may not always know for sure at the beginning of the project.
@Ryan, that makes sense. I’ve used scoping extensively and have run into situations where that can backfire. On a recent project we scoped all finders a certain way only to find later on in the project an edge case that needs to be unscoped. Less code can sometimes be rigid. That said, I still think scoping is more efficient querying the DB and a little more DRY in comparison especially if you’re explicitly defining finders.
Finder objects separate your data model (in this instance an activerecord object) from the business logic of your application. A finder uses the data model to find objects given a specific rule or set of rules. I think it is wrong to have a data model use itself to find objects given a specific rule or set of rules.
I know rails allows you to do it, in fact, it encourages you to do so. However, I think we need to step back and think before we drink that rails software design “koolaid”.