This project is archived and is in readonly mode.
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.
Comments and changes to this ticket
-
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
andcreate
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 everyfind
/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 bychildren
, it executes with:conditions => { :parent_id => ... }
applied to every resulting call tofind
andcreate
, which includes not only the initialcreate
call but also thefind
you do in#parent
as part of your#before_save
callback. If you watch the SQL log you'll see that this query looks likeSELECT * 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 (fromchildren
) that you're executing within; of course this query is going to fail because noThing
is its own parent.(The reason
build
works is because you're calling thesave
yourself afterwards, outside of the scope, whereascreate
is taking care of thesave
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'swith_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 dothing.children.create(:name => ...)
, except this time it'll work properly because the association proxy provides the right additional arguments to thecreate
(i.e.parent_id
) but doesn't create a scope that would interfere with any unrelatedfind
s 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 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 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 callsave
later, but because you don't call thatsave
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 ofActiveRecord::NamedScope::Scope
, which is returned by yourscoped
call in#children
. Calling a method on a named scope will activate that scope for the duration of the method call, andcreate
callssave
as part of doing its job, so thesave
executes while the scope is still active. And because callingsave
in turn fires thebefore_save
callback, that callback executes while the scope's active too.Callbacks don't get any special treatment: any call to
find
orcreate
while the scope is active (i.e. between the time when thecreate
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 callsfind
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
andcreate
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 yourparent
example), which is whywith_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 callfind
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 tofind
orcreate
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 inparent
,parent=
androot
). If it did this, its simulated associations would work fine.Cheers,
-Tom -
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 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.
-
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”
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>