This project is archived and is in readonly mode.

#3680 ✓ stale
Matthew O'Riordan

Scope problem with ActiveRecord in before_save or after_save when using create method as opposed to build

Reported by Matthew O'Riordan | January 10th, 2010 @ 05:07 PM

I have discovered an easily replicable bug in ActiveRecord when calling the create method on a scope. However, strangely when using the build method and then calling save immediately afterwards, it works perfectly, which leads me to believe there is definitely a bug in the way create is implemented.

The error I receive is "ActiveRecord::RecordNotFound: Couldn't find Thing with ID=446545869", yet a Thing does exist in the database with that ID. When using the build method, this error does not occur.

Here is how the problem is replicated:

db/migrate/20100101_create_things.rb Migration code to set up model

class CreateThings < ActiveRecord::Migration
  def self.up
    create_table :things do |t|
      t.string :name
      t.integer :parent_id
      t.timestamps
    end
  end

  def self.down
    drop_table :things
  end
end

test/fixutres/things.yml Fixtures code used to run tests which fail

us:
  name: United States

california:
  name: California
  parent_id: <%= Fixtures.identify(:us) %>
  
florida:
  name: Florida
  parent_id: <%= Fixtures.identify(:us) %>

sanfran:
  name: San Francisco
  parent_id: <%= Fixtures.identify(:california) %>
  
sandiego:
  name: San Diego
  parent_id: <%= Fixtures.identify(:california) %>
  
miami:
  name: Miami
  parent_id: <%= Fixtures.identify(:florida) %>
  
daytona:
  name: Daytona
  parent_id: <%= Fixtures.identify(:florida) %>

models/thing.rb Model code which has named scopes

class Thing < ActiveRecord::Base
  # some debugging output to demonstrate the error 
  def before_save
    puts "Before save called for #{name}, parent id: #{parent_id}, self: #{self.inspect}"
    puts "Parent #{parent.name} with id #{parent.id} found" if parent # this fails when create method called
  end
  
  def children
    self.class.scoped :conditions => { "parent_id" => id }
  end
  
  def parent 
    self.class.find(parent_id) if parent_id
  end
end

/test/unit/thing_test.rb Unit test code which fails

require 'test_helper'

class ThingTest < ActiveSupport::TestCase
  test "creation of children objects" do
    us = things(:us)
    assert_not_nil(Thing.create(:name => "United Kingdom"), "Created a root element")
    assert(us.children.first.children.first.parent.parent == us, "Iterate down tree and back up to US did not work")
    assert(things(:daytona).parent == things(:florida), "Parent not working")
    
    nevada = us.children.build(:name => "Nevada")
    assert(nevada.save, "Could not save Nevada with build statement")
    assert(nevada.parent == us, "Nevada's parent should be the US")
    
    arizona = us.children.create(:name => "Arizona") # a runtime error is raised when this method is called
    assert_not_nil(arizona, "Could not save Arizona with create statement")
    assert(arizona.parent == us, "Arizona's parent should be the US")
  end
end

As you will see when you run the unit test, the us.children.create method will fail.
Strangely when inspecting the actual Thing objet, it looks exactly the same when it fails versus when it does not i.e. it is "#"

Give me a shout if there is anything I can help with. I have attached the code I used to replicate this issue if that helps.

Matt
http://mattheworiordan.com

Comments and changes to this ticket

  • Tom Stuart

    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

  • Matthew O'Riordan

    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.

  • Tom Stuart

    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

  • Stefan Kroes

    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.

  • Matthew O'Riordan

    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

    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

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

    • Tag cleared.

    Automatic cleanup of spam.

  • Santiago Pastorino

    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

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

    • State changed from “open” to “stale”
  • fgfhnmn
  • mklo

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Attachments

Pages