Association Proxies

Posted by Koz Monday, March 26, 2007 17:56:00 GMT

We’ve had several submissions in the past which failed to fully utilise rails’ collection proxies. For future reference, I figured I’d write up the most commonly missed opportunities. To demonstrate this we’ll assume a Todo list application where each user can manage multiple Todo lists, with a simple model like this:

1
2
3
4
5
6
7

class User < ActiveRecord::Base
  has_many :todo_lists
end

class TodoList < ActiveRecord::Base
  belongs_to :user
end

Case One – Restricting Access to User Data

If you’re following the new restful conventions the url to access a Todo List is going to look something like http://todo.example.com/todo_lists/1

To keep your application secure, you’ll want to ensure that users can’t just change the ID in the url and manage someone else’s tasks. This is one of the most commonly asked questions on mailing lists and IRC channels, and we’ve seen quite a few anti-patterns to solve it. Some of the most common anti-patterns we’ve seen are these:

Anti-Pattern #1: Manually specifying the IDs when you construct the queries;

1
2
3
4
5

def show
  unless @todo_list = TodoList.find_by_id_and_user_id(params[:id], current_user.id)
  redirect_to '/'
  end
end

Anti-Pattern #2: Querying globally, then checking ownership after the fact;

1
2
3
4

def show
  @todo_list = TodoList.find(params[:id])
  redirect_to '/' unless @todo_list.user_id = current_user.id
end

Anti-Pattern #3: Abusing with_scope for a this simple case either directly, or in an around_filter.

1
2
3
4
5

def show
  with_scope(:find=>{:user_id=>current_user.id}) do
    @todo_list = TodoList.find(params[:id])
  end
end

Best Practice: The most effective way to do this is to call find on the todo_lists association.

1
2
3

def show
  @todo_list = current_user.todo_lists.find(params[:id])
end

In the event that the user plays with the URL to point to something they don’t own, an ActiveRecord::RecordNotFound exception will be raised. In general I simply ignore these exceptions as it’s unlikely they’re caused by legitimate use, but if you want to handle them you can simply rescue the exception.

1
2
3
4
5
6

def show
  @todo_list = current_user.todo_lists.find(params[:id])
rescue ActiveRecord::RecordNotFound => e
  flash[:warning] = "Stop playing around with your urls"
  redirect_to '/'
end

Additionally, the find method on todo_lists is just like the regular find method; you’re not restricted to merely passing it an ID. So to build a secure search action you can do something like:

1
2
3
4

def search
  @todo_lists = current_user.todo_lists.find(:all, :conditions=>["name like ?", params[:q]],
       :include=>[:items])
end

That dispatching isn’t limited to find, you can call any method defined on the TodoList class. For example, say you wanted to encapsulate the search behaviour inside the TodoList class, you can define a class method:

1
2
3
4
5
6

class TodoList < ActiveRecord::Base
  # ...
  def self.search(query)
    find(:all, :conditions=>["name like ?", query], :include=>[:items])
  end
end

Then you can call that by using class methods on the has_many association:

1
2
3

def search
  @todo_lists = current_user.todo_lists.search(params[:q])
end

Case Two – Assigning Ownership

Just as you need to find TodoLists for a user, your Todo list application will also need to create new lists for a user. Frequently in submissions we’ll see something like this:

1
2
3
4
5
6

def create
  @todo_list = TodoList.new(params[:todo_list])
  @todo_list.user_id = current_user.id
  @todo_list.save!
  redirect_to todo_list_url(@todo_list)
end

Just like find, the has_many association creates a number of methods to help you create new members of the todo_lists collection. If there’s no validation on the todo list model, then the simplest option is just to use create!:

1
2
3
4

def create
  @todo_list = current_user.todo_lists.create! params[:todo_list]
  redirect_to todo_list_url(@todo_list)
end

This will raise an exception in the event the model fails validation, which prevents your application from silently failing. However, it doesn’t provide a particularly good user experience (in fact, it sucks). In situations where validation failures are likely, you can still use the association proxies:

1
2
3
4
5
6
7
8

def create
  @todo_list = current_user.todo_lists.build params[:todo_list]
  if @todo_list.save
    redirect_to todo_list_url(@todo_list)
  else
    render :action=>'new'
  end
end

Case Three – Special Queries

The final case I wanted to cover is where association declarations can be used to save you time is where you have a special query you’re frequently using. In our case, let’s say you occasionally want to get the user’s completed lists, and their active ones. The simplest way to get these is:

1
2
3
4

def show
  @completed_lists = current_user.todo_lists.find_all_by_complete(false)
  @active_lists    = current_user.todo_lists.find_all_by_complete(true)
end

However if you’ve been following our posts to date, you probably know that you should make your model match your domain, and that means your statements should read reasonably close to plain English. So if you want to find the user’s active_lists, you should probably do that with something like current_user.active_lists. A first pass at that may be.

1
2
3
4
5
6
7
8
9

class User < ActiveRecord::Base
  def completed_lists
    todo_lists.find_all_by_completed(true)
  end

  def active_lists
    todo_lists.find_all_by_completed(false)
  end
end

However a big downside to this approach is that repeated calls to active_lists will issue the query again, then construct the records from the result set, wasting time and memory. The obvious solution is to make sure the calls are cached, and the easiest way to do that is to use a has_many association.

1
2
3
4

class User < ActiveRecord::Base
  has_many :completed_lists, :class_name=>"TodoList", :conditions=>{:completed=>true }
  has_many :active_lists,    :class_name=>"TodoList", :conditions=>{:completed=>false}
end
Comments

Leave a response

  1. Jared – March 26, 2007 @ 07:06 PM

    For your first example, what about cases where you don’t have the current user object already?

    Say you save the user id in the session, wouldn’t it make sense to do this: TodoList.find_by_id_and_user_id(params[:id], session[:current_user_id])

    Rather than lookup the User just do a scoped find?

  2. Michael Koziarski – March 26, 2007 @ 07:23 PM

    @Jared:

    I would recommend you always fetch the user record, and there are a few reasons for that:

    1) If the user is deleted, they’ll be prevented from using the application. 2) You almost certainly want the user object for other parts of the application, perhaps rendering a partial which contains the user’s name. 3) Unless you have extensive benchmarks showing that your ‘find a user’ query is the problem, what you’re doing is premature optimisation with a big hit to readibility.

  3. Chris – March 26, 2007 @ 07:24 PM

    @Jared:

    This pattern is used throughout most of the authorization plug-ins. You’d simply have a current_user method that returned the current user object from session.

    If the current user object wasn’t in the session, you’d probably just redirect to the login page.

    def current_user @current_user if logged_in? end

    That sort of deal.

  4. Jon Leighton – March 26, 2007 @ 07:29 PM

    Good stuff. I think this all basically boils down to, as Chad Fowler put it, “don’t use SQL in your controller”. Or more specifically, don’t use SQL or anything that builds SQL in your controller. I think it’s a good best practise.

    In the last situation, you could also do something along the lines of current_user.todo_lists.complete and current_user.todo_lists.active using memoization. I prefer the syntax and it’s only a tiny bit more code. (Example).

  5. Michael Koziarski – March 26, 2007 @ 07:32 PM

    @Jon:

    But your solution doesn’t let you call find on the ‘active’ collection, making things like pagination, includes etc. You could implement that with with_scope in a class method, but then you’ve just reinvented has_many.

  6. Jon Leighton – March 26, 2007 @ 07:51 PM

    Fair comment. I guess it depends how you need to use it. You could always add a “page” parameter to the method, for example, which would change the query (though that would also affect the memoized value)—that would help with factoring all SQL into the model.

    Compare:

    @completed = current_user.completed_lists.find(:all, :pagination_conds)

    vs.

    @completed = current_user.todo_lists.complete(params[:page])

    Still, it depends how much flexibility is needed as to which is a better option.

  7. Tom – March 26, 2007 @ 07:56 PM

    Kind of surprised you didn’t mention calling through the association proxy to class methods, as you demonstrated here. That’s a nicer solution all round, surely?

  8. jared – March 26, 2007 @ 08:17 PM

    @Michael

    I’m not suggesting your method is bad or trying to prematurely optimize. Most of the time it does make sense to use the association just as you described. Rather, I was just suggesting that in cases where you don’t already have the User object, it seems silly to me to do an extra query to first fetch the User just for the sake of doing a scoped find.

    Your reasons for suggesting that one would always want to pull back a User object are reasonable. I just wasn’t making the same set of assumptions.

    Anyway, it’s just my two cents.

  9. Michael Koziarski – March 26, 2007 @ 08:33 PM

    @Jon: Absolutely, everything comes down to trade-offs. Both options work fine and it basically comes down to your own personal preference.

    @jared: Sure, I didn’t mean to suggest you’re prematurely optimizing just that most people who use that approach would be much better served by fetching the record.

    @Tom: Good point, I’ll work a section in here doing just that.

  10. Michael Koziarski – March 26, 2007 @ 08:41 PM

    @Tom: Done, thanks for the tip!!

  11. Jon Leighton – March 26, 2007 @ 08:56 PM

    Would Tom’s way no have exactly the same issues as the way I suggested? Or have I missed something?

  12. Michael Koziarski – March 26, 2007 @ 09:55 PM

    @Jon: Search takes a parameter so you couldn’t replace it with an association. It’s included for completeness when referencing this article in the future.

  13. Wes Ratcliff – March 26, 2007 @ 11:36 PM

    Great post as always – I’m guilty of a few of these myself… off to refactor. Thanks guys!

  14. Nicolás Sanguinetti – March 27, 2007 @ 01:39 AM
    However a big downside to this approach is that repeated calls to active_lists will issue the query again
    Correct me if I’m wrong, but isn’t that fixed in edge with the object caching? Of course it’s better) to use has_many associations, but for the readability / code beauty / maintainability, instead of the performance issues :)
  15. Dan Manges – March 27, 2007 @ 03:27 AM

    Regarding that last example, I really like the scope_out plugin for creating the finders.

    class TodoList < ActiveRecord::Base
  scope_out :active, :conditions => {:completed => false}
  scope_out :completed
end
    class User < ActiveRecord::Base
  has_many :todo_lists, :extend => TodoList::AssociationMethods
end

    Now all of the following work:

    TodoList.find_active(:all) TodoList.find_completed(:first) current_user.todo_lists.find_active(:all, :order => ‘whatever’)
    1. with caching as Jamis recommended on his blog current_user.todo_lists.active
  16. Michael Koziarski – March 27, 2007 @ 05:36 AM

    @Nicolas: While it does cache the query result, it saves the result set not the instance, and will still incur quite a bit of overhead while building up the objects from the hashes.

    @Dan: Interesting plugin, but I think I still prefer the seperate associations rather than the scoped class methods. Caching is only the first part of the job. Cache invalidation is harder, and the association methods give you that with current_user.active_lists(:reload).each…

  17. Bragi Ragnarson – March 27, 2007 @ 08:22 AM

    I really like your solution to second and third problem.

    However I do not entirely agree with first suggestion. Using association find is in fact a cleanest way of fetching user’s resources. Knowing that I still separately fetch items by id and explicitly check ownership.

    When there is no item with given ID I simply send out 404 response. But when user is unauthorized to view existing resource I log such occurrence together with user’s information and send out 403 (Unauthorized).

    I think such approach is more REST oriented and in addition allows to find potential troublemakers early.

  18. Joel Kociolek – March 27, 2007 @ 08:24 AM

    This is a great Rails pattern that I’ve used many times.

    However I’m still looking for a way to cleanly handle the case where some of the users (e.g. admin users) should have acces to all the records, and some should have access only to their own.

  19. Doug – March 27, 2007 @ 02:47 PM

    Thanks Koz, this is great. I love the anti-pattern rundown, too. I’ve hit most of those before settling on the recommended approach. This could have saved me some time :-)

  20. Michael Koziarski – March 27, 2007 @ 04:40 PM

    @Bragi: If you have reasons for handling the error states seperately, then yeah, the ‘query then check’ style is probably best for you.

    As for showing a 404 vs a 403, for my applications there’s not really any practical benefits to that. If someone’s tweaking the URLs, I’m not going to go out of my way to show them a more ‘friendly’ error message. Your requirements may well be different though.

  21. Pat Maddox – March 28, 2007 @ 01:41 AM

    @Joel: As I was reading this post, I thought to myself that that’s where this approach (sort of) breaks down. Of course the vast majority of the time it’s perfect though.

    I’ve used two solutions in the past, and like everything I guess it boils down to personal preference. The first is to make another has_many with different conditions. The other is sticking some query on the user model.

    1
2
3
4
5

    class User < ActiveRecord::Base
  def access?(list)
    admin? || list.user_id == id
  end
end

    I definitely prefer the latter, but like everything else it comes down to examining the tradeoffs and personal preference.

  22. Pat Maddox – March 28, 2007 @ 01:44 AM

    @myself: Obviously with my solution you wouldn’t use the proxy in the controller anymore…it’d be like

    1
2
3
4

    def authorized?
  @list = TodoList.find params[:id]
  current_user.access? @list
end
  23. Dan Manges – March 28, 2007 @ 03:46 AM

    @Koz: The scope out plugin supports cache invalidation. In your example instead of current_user.active_lists(:reload).each… it would be current_user.todo_lists.active(:reload).each… it may not be ideal for all applications, but I find the plugin very useful for this.

    What bothers me with your last example is (1) you just defined what makes a TodoList active or complete in your User model and (2) For every model that has_many :todo_lists, you need to add the same associations to have the scoped criteria.

  24. Ben Reubenstein – March 28, 2007 @ 07:39 PM
    def create @todo_list = current_user.todo_lists.build params[:todo_list] if @todo_list.save redirect_to todo_list_url(@todo_list) else render :action=>'new' end end

    Could be:

    def create @todo_list = current_user.todo_lists.build params[:todo_list] @todo_list.save ? redirect_to todo_list_url(@todo_list) : render :action=>'new' end
  25. Jeff Casimir – March 28, 2007 @ 09:09 PM

    Koz,

    This is pushing the subject of the article, but do you know of any way to do multi-step proxying? For instance, lets say the following…

    A has_many C :through => B C has_many D

    I’d like to be able to say a.ds and get a list of all the matching models. It that dumb? Right now I end up with hacks like a.cs.collect{|x| x.ds} but you lose the ability to do nicely scoped queries. I thought maybe I could pull some trickery like this:

    A has_many D :through => [B, C]

    But that doesn’t seem to exist, best I can tell. If it helps I can describe the problem in a real scenario I’m facing. Thanks!

  26. Random8r – March 29, 2007 @ 12:33 AM

    In the example where you rescue the exception, you forgot rescue’s relative end statement.

    - Random8r.

  27. Jonathan – March 29, 2007 @ 08:23 PM

    I like this approach a lot. How would you go about testing this though? Formerly I would have written (using rspec)

    TodoList.should_receive(:find_all_by_user_id).with(current_user.id)
  28. Jacob Burkhart – March 30, 2007 @ 05:33 PM

    def search @todo_lists = current_user.todo_lists.search(params[:q]) end

    wait, what? how does that work? Didn’t we define “search” as a method on the TodoList class? How does it end up as a method on the association proxy? shouldn’t we call the method like this:

    TodoList.search(params[:q])

  29. Rabbit – March 30, 2007 @ 10:00 PM

    Wow! I dig the use of has_many there; I didn’t know you could do that. Thanks for sharing, koz. :)

  30. Adam Roth – April 18, 2007 @ 06:03 PM

    Best. Tips. Ever.

  31. Paul Hoehne – June 01, 2007 @ 04:43 PM

    In case 2, why wouldn’t you do?:

    def create @todo_list = TodoList.new(params[:todo_list]) current_user.todo_lists << @todo_list current_user.save redirect_to todo_list_url(@todo_list) end

  32. Jamis Buck – June 01, 2007 @ 05:17 PM

    Paul, that’s pretty much what case 2 demonstrated, with the following line:

    

    @todo_list = current_user.todo_lists.create! params[:todo_list]

    Although the solution you presented works, I’m finding that SomeModel.new(params[:foo]), followed by something to hook it up to the appropriate association, is generally error prone and (IMHO) ugly. :) I much prefer letting rails do the work of hooking up the associations for me.

  33. Hyun – June 02, 2007 @ 09:23 AM

    Wow, The three tips hit my head, Bang! Bang!! Bang!!!

  34. Ben Poweski – June 07, 2007 @ 01:54 PM

    I really enjoyed the “Case One – Restricting Access to User Data” part as I’m a bit of a security buff. It is a really interesting application of the pattern, you have just inspired me greatly!

  35. Jason Kunesh – June 14, 2007 @ 08:42 PM

    Jamis + Koz: The Rails Way is the most useful Rails resource out there. Thanks a ton for dedicating the time to it.

    How would one apply the Case 2 pattern when using a has_many :through relationship? If the user has todo_lists through projects, for example, is one relegated to:

    @todo_list = TodoList.new(params[:list]) current_user.todo_lists << @todolist

    all over again? Is there a way to avoid HasManyThroughCantAssociateNewRecords errors without doing this?

Comment