Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Apr 8, 2012
Merged

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

merged 2 commits into from
Apr 8, 2012

Conversation

benedikt
Copy link
Contributor

@benedikt 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'"

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@benedikt
Copy link
Contributor Author

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
Removes caching from ActiveRecord::Core::ClassMethods#relation
@jeremy jeremy merged commit 9f37f33 into rails:master Apr 8, 2012
@jeremy
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants