Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Tom Stuart January 14th, 2010 @ 09:37 AM

  Hi Matt,

  This is (arguably) a subtle problem with your code rather than a bug in ActiveRecord.

  The issue is that find and create methods called within a scope execute with that scope applied, i.e. for the duration of the method's execution you inherit the scope's arguments on every find/create that you call, which of course is the whole point of a scope.

  This is causing problems here because, when you call create on the scoped class returned by children, it executes with :conditions => { :parent_id => ... } applied to every resulting call to find and create, which includes not only the initial create call but also the find you do in #parent as part of your #before_save callback. If you watch the SQL log you'll see that this query looks like

  SELECT * FROM "things" WHERE ("things"."id" = 446545869) AND ("things"."parent_id" = 446545869)
  

  where the first condition is being supplied by your call to find(parent_id) and the second is being supplied by the overall scope (from children) that you're executing within; of course this query is going to fail because no Thing is its own parent.

  (The reason build works is because you're calling the save yourself afterwards, outside of the scope, whereas create is taking care of the save while the scope is still in effect.)

  But that's not what you meant: you were expecting the call to self.class.find(parent_id) to ignore any scope that might currently be in effect and just have the single condition on the primary key. In Rails you have to say that explicitly by using ActiveRecord's with_exclusive_scope class method, so the naïve solution is to refactor #parent like this:

  def parent 
    self.class.find_parent(parent_id) if parent_id
  end
  
  def self.find_parent(parent_id)
    with_exclusive_scope do
      find(parent_id)
    end
  end
  

  This makes your tests pass, but it's not very handsome. Why should you have to fiddle around with obscure methods like with_exclusive_scope? Why should you need to drop out to a class method every time you want to ignore the current scope? Why don't Rails programmers run into subtle problems like this all of the time?

  Because associations take care of these details for you, so the underlying problem here is that you're using scopes to simulate associations without dealing with the necessary subtleties. A much better refactoring is to throw away your #children and #parent methods entirely and replace them with:

  belongs_to :parent, :class_name => 'Thing'
  has_many :children, :class_name => 'Thing', :foreign_key => :parent_id
  

  This will give you effectively the same behaviour that you were going for, but without any of the mucking about with scopes. Calling us.children will then return an association proxy (not an actual array of children), so this still lets you do thing.children.create(:name => ...), except this time it'll work properly because the association proxy provides the right additional arguments to the create (i.e. parent_id) but doesn't create a scope that would interfere with any unrelated finds that you do in your callbacks.

  This is much cleaner and more conventional. I'm not sure why you were trying to use scopes in the first place (I'm sure you know about associations!) but it's worth noting that association proxies have the same sort of power. You can call finders on them, for example, and they're scoped in the right way:

  >> us.children.find_by_name('Florida')
  => #<Thing id: 743108299, name: "Florida", parent_id: 446545869, created_at: "2010-01-14 09:21:48", updated_at: "2010-01-14 09:21:48">
  >> us.children.find_by_name('San Francisco')
  => nil
  

  You can also define additional methods on association proxies (see the Association extensions section) or even call through to existing class methods, by which I mean that if you define a class method like

  class Thing < ActiveRecord::Base
    # ...
  
    def self.names
      all.map(&:name).join(', ')
    end
  end
  

  then you can call it on either the class itself or any proxy for an association of objects of that class, and it'll do the right thing:

  >> Thing.names
  => "California, San Diego, Miami, United States, Florida, Daytona, San Francisco, Nevada"
  >> us = Thing.find_by_name('United States')
  => #<Thing id: 446545869, name: "United States", parent_id: nil, created_at: "2010-01-14 09:21:48", updated_at: "2010-01-14 09:21:48">
  >> us.children.names
  => "California, Florida, Nevada"
  

  All this means that associations are usually powerful enough for the job, so you can generally avoid scopes unless you're doing something that isn't association-like.

  And finally, you could make the Thing class even simpler again by using the official acts_as_tree plugin, which defines the associations for you and throws a couple more handy methods (#siblings, #root etc) into the bargain.

  Cheers,
  -Tom

• You flagged this item as spam.
  

  Matthew O'Riordan January 15th, 2010 @ 11:33 AM

  Hi Tom

  Thanks for that reply, and can certainly understand the view that this is not necessarily a bug, but perhaps a subtlety of the way my code was implemented. However, I am not sure I agree entirely as I do not understand why the scope in a callback event should be different when calling the save method over the build method? Whilst there are numerous other ways in which the child/parent relationships can be achieved to avoid this issue (which I have had to do), surely ActiveRecord should be consistent in the way it behaves? According to the documentation within ActiveRecord, there is no indication that the scope should be different - it simply states the callback events will be executed.

  Surely if ActiveRecord save & create methods have different scopes, then this should be either documented as a feature or side effect of the implementation, or it should behave consistently? If neither of those conditions are met, then I believe this constitues a bug as other people will be affected by this.

  BTW. In regards to your suggestion about using acts_as_tree to avoid these sorts of issues, I have been using a another plugin called ancestry as it provides a richer set of functionality to acts_as_tree. Please see this issue in Github where I explained the problem to Stefan, and he told me that he felt that this was an ActiveRecord issue. I then created a simplified version of the problem experienced in ancestry, and posted it here.

• You flagged this item as spam.
  

  Tom Stuart January 15th, 2010 @ 08:35 PM

  I do not understand why the scope in a callback event should be different when calling the save method over the build method

  The callback doesn't fire when you call build, it fires when you call save later, but because you don't call that save while a scope is active it doesn't cause a problem.

  In your example you're calling create on a named scope -- a special proxy object, specifically an instance of ActiveRecord::NamedScope::Scope, which is returned by your scoped call in #children. Calling a method on a named scope will activate that scope for the duration of the method call, and create calls save as part of doing its job, so the save executes while the scope is still active. And because calling save in turn fires the before_save callback, that callback executes while the scope's active too.

  Callbacks don't get any special treatment: any call to find or create while the scope is active (i.e. between the time when the create method starts executing and the time when it returns) will respect that scope, so the fact you're seeing the problem in a callback is just because it's a method which calls find on the scoped class at a time when the scope is active, not specifically because it's a callback.

  This is just how scopes work; none of it would have been a problem for you if Ancestry didn't have a bug. The underlying mechanism (of a per-class thread-local stack of currently active scopes) is not perfect but it does the job of making scopes work transparently by exposing them to every find and create that happens while they're active, regardless of where they're being called and by whom, which more often than not is exactly the right behaviour. There are certain occasions where you explicitly want to disregard any active scope (e.g. when trying to find a record by its primary key as in your parent example), which is why with_exclusive_scope exists, but those occasions are the exception rather than the rule.

  The bug in Ancestry is that it's simulating associations with named scopes but not accounting for the effect of those scopes. When you call scoped on a class, you get a proxy object which activates a scope for the duration of any methods called on it; when you call find on a class, any scope currently active will be respected. Things are therefore going to go wrong if some of your instance methods return named scopes and some of your other instance methods make calls to find or create which will go wrong when a scope is active.

  So when Ancestry says

  def parent
    if parent_id.blank?
      nil
    else
      self.base_class.find(parent_id)
    end
  end
  

  it actually means

  def parent
    if parent_id.blank?
      nil
    else
      self.base_class.send(:with_exclusive_scope) do
        self.base_class.find(parent_id)
      end
    end
  end
  

  (or, perhaps more tidily, it should elsewhere define a class method like find_with_exclusive_scope and call that in parent, parent= and root). If it did this, its simulated associations would work fine.

  Cheers,
  -Tom

• You flagged this item as spam.
  

  Stefan Kroes January 18th, 2010 @ 10:48 AM

  I think you can argue all you want about the intricacies of this bug. The bottom line is that it is definitely unexpected behavior. And following the principle of least surprise it should be eliminated.

• You flagged this item as spam.
  

  Matthew O'Riordan January 18th, 2010 @ 12:28 PM

  Hi Tom

  Thanks for explaining that to me. The per class thread local stack certainly makes sense, and I do understand now why this is happening. I do think this could be confusing for others though as a call such as:
  us.children.build(:name => "New York").save would appear to be the same as
  us.children.create(:name => "Arizona") but is clearly not when you consider how the scope within proxy_options is applied to build & create, but not to save as the proxy object is no longer once the model instance is created.

  Makes sense to me.

  Matt

• 

  Ryan Bigg June 12th, 2010 @ 03:22 AM

  • State changed from “new” to “open”

  Please supply a patch which duplicates this issue in Rails or if it's now fixed (please try 2-3-stable and 3.0), a comment saying that this ticket can be closed would be good too.

• 

  Ryan Bigg October 9th, 2010 @ 09:54 PM

  • Tag cleared.

  Automatic cleanup of spam.

• 

  Santiago Pastorino February 2nd, 2011 @ 04:30 PM

  This issue has been automatically marked as stale because it has not been commented on for at least three months.

  The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

  Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

• 

  Santiago Pastorino February 2nd, 2011 @ 04:30 PM

  • State changed from “open” to “stale”