Removes caching from ActiveRecord::Core::ClassMethods#relation #5718

Merged
merged 2 commits into from Apr 8, 2012

Conversation

Projects
None yet
2 participants
@benedikt
Contributor

benedikt commented Apr 3, 2012

The #relation method gets called in four places and the return value was instantly cloned in three of them. The only place that did not clone was ActiveRecord::Scoping::Default::ClassMethods#unscoped. This introduced a bug described in #5667 and should really clone the relation, too. This means all four places would clone the relation, so it doesn't make a lot of sense caching it in the first place.

The four places with calls to relations are:

activerecord/lib/active_record/scoping/default.rb:110:in `block in build_default_scope'"
activerecord/lib/active_record/scoping/default.rb:42:in `unscoped'"
activerecord/lib/active_record/scoping/named.rb:38:in `scoped'"
activerecord/lib/active_record/scoping/named.rb:52:in `scope_attributes'"
Removes caching from ActiveRecord::Core::ClassMethods#relation
The #relation method gets called in four places and the return value was instantly cloned in three of them. The only place that did not clone was ActiveRecord::Scoping::Default::ClassMethods#unscoped. This introduced a bug described in #5667 and should really clone the relation, too. This means all four places would clone the relation, so it doesn't make a lot of sense caching it in the first place.

The four places with calls to relations are:

activerecord/lib/active_record/scoping/default.rb:110:in `block in build_default_scope'"
activerecord/lib/active_record/scoping/default.rb:42:in `unscoped'"
activerecord/lib/active_record/scoping/named.rb:38:in `scoped'"
activerecord/lib/active_record/scoping/named.rb:52:in `scope_attributes'"
@@ -1099,6 +1099,10 @@ def test_unscoped_block_style
assert_equal 'honda', FastCar.unscoped { FastCar.order_using_old_style.limit(1).first.name}
end
+ def test_unscoped_relation_clones
+ assert_not_equal CoolCar.unscoped.object_id, CoolCar.unscoped.object_id

This comment has been minimized.

@jeremy

jeremy Apr 3, 2012

Member

This feels like testing an implementation detail. Got a test from #5667 that fails when the relation isn't cloned?

@jeremy

jeremy Apr 3, 2012

Member

This feels like testing an implementation detail. Got a test from #5667 that fails when the relation isn't cloned?

This comment has been minimized.

@benedikt

benedikt Apr 3, 2012

Contributor

You're right. I'll change it to a test that resembles the #5667 issue.

@benedikt

benedikt Apr 3, 2012

Contributor

You're right. I'll change it to a test that resembles the #5667 issue.

@benedikt

This comment has been minimized.

Show comment
Hide comment
@benedikt

benedikt Apr 8, 2012

Contributor

@jeremy I changed the test to resemble the remaining issue described in #5667. Is it okay or do you want me to change anything else?

/cc @jonleighton

Contributor

benedikt commented Apr 8, 2012

@jeremy I changed the test to resemble the remaining issue described in #5667. Is it okay or do you want me to change anything else?

/cc @jonleighton

jeremy added a commit that referenced this pull request Apr 8, 2012

Merge pull request #5718 from benedikt/master
Removes caching from ActiveRecord::Core::ClassMethods#relation

@jeremy jeremy merged commit 9f37f33 into rails:master Apr 8, 2012

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Apr 8, 2012

Member

Thanks @benedikt !

Member

jeremy commented Apr 8, 2012

Thanks @benedikt !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment