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

Cause ActiveRecord::Base::reload to also ignore the QueryCache. #19886

Merged
merged 1 commit into from
May 28, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion activerecord/lib/active_record/persistence.rb
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ def toggle!(attribute)
# # => #<Account id: 1, email: 'account@example.com'>
#
# Attributes are reloaded from the database, and caches busted, in
# particular the associations cache.
# particular the associations cache and the QueryCache.
#
# If the record no longer exists in the database <tt>ActiveRecord::RecordNotFound</tt>
# is raised. Otherwise, in addition to the in-place modification the method
Expand Down Expand Up @@ -416,6 +416,8 @@ def toggle!(attribute)
# end
#
def reload(options = nil)
self.class.connection.clear_query_cache
Copy link
Member

Choose a reason for hiding this comment

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

I think this should use uncached {} https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb#L47 block instead. So we dont clear the entire cache when calling reload.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think uncached is a good solution, as explained in #17140 (comment) and #17140 (comment).


fresh_object =
if options && options[:lock]
self.class.unscoped { self.class.lock(options[:lock]).find(id) }
Expand Down
27 changes: 27 additions & 0 deletions activerecord/test/cases/persistence_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,33 @@ def test_find_via_reload
assert_not post.new_record?
end

def test_reload_via_querycache
ActiveRecord::Base.connection.enable_query_cache!
ActiveRecord::Base.connection.clear_query_cache
assert ActiveRecord::Base.connection.query_cache_enabled, 'cache should be on'
parrot = Parrot.create(:name => 'Shane')

# populate the cache with the SELECT result
found_parrot = Parrot.find(parrot.id)
assert_equal parrot.id, found_parrot.id

# Manually update the 'name' attribute in the DB directly
assert_equal 1, ActiveRecord::Base.connection.query_cache.length
ActiveRecord::Base.uncached do
found_parrot.name = 'Mary'
found_parrot.save
end

# Now reload, and verify that it gets the DB version, and not the querycache version
found_parrot.reload
assert_equal 'Mary', found_parrot.name

found_parrot = Parrot.find(parrot.id)
assert_equal 'Mary', found_parrot.name
ensure
ActiveRecord::Base.connection.disable_query_cache!
end

class SaveTest < ActiveRecord::TestCase
self.use_transactional_tests = false

Expand Down