From 19deeb08df6279a9cbf0c65548641776c0451471 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Sat, 31 Dec 2016 18:45:52 +0100 Subject: Fix tests with counter cache touching and more. * Refactor to use `touch_updates` Ensures we only call `current_time_from_proper_timezone` from one place. * Clarify touch default in tests. Not interested in what happens when passed false but that nothing passed means no touching. * Backdate touched columns in tests. We can't be sure a test progresses through time, so our touching code may be working correctly but the test itself is brittle. Fix by backdating that's further in the past akin to what the timestamps tests do: https://github.com/rails/rails/blob/d753645d40e925973724e4c3a8617b654da90e41/activerecord/test/cases/timestamp_test.rb#L17 * Expand changelog entry. Elaborate and show examples. Closes #26995. [ Jarred Trost & Kasper Timm Hansen ] --- activerecord/CHANGELOG.md | 15 ++- activerecord/lib/active_record/counter_cache.rb | 28 ++--- activerecord/test/cases/counter_cache_test.rb | 159 +++++++++++------------- 3 files changed, 98 insertions(+), 104 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5fa70e4aad..b59d24f88d 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,16 @@ -* Add option to update specified timestamp columns when incrementing, - decrementing, resetting, or updating counter caches. +* Add `touch` option to counter cache modifying methods. + + Works when updating, resetting, incrementing and decrementing counters: + + # Touches `updated_at`/`updated_on`. + Topic.increment_counter(:messages_count, 1, touch: true) + Topic.decrement_counter(:messages_count, 1, touch: true) + + # Touches `last_discussed_at`. + Topic.reset_counters(18, :messages, touch: :last_discussed_at) + + # Touches `updated_at` and `last_discussed_at`. + Topic.update_counters(18, messages_count: 5, touch: %i( updated_at last_discussed_at )) Fixes #26724. diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb index af29b4c5bb..9355a2f7a7 100644 --- a/activerecord/lib/active_record/counter_cache.rb +++ b/activerecord/lib/active_record/counter_cache.rb @@ -47,12 +47,9 @@ module ActiveRecord updates = { counter_name.to_sym => object.send(counter_association).count(:all) } - touch_time = object.send(:current_time_from_proper_timezone) - resolve_timestamp_columns(object, touch).each do |column| - updates[column] = touch_time - end - - unscoped.where(primary_key => object.id).update_all(updates) + unscoped.where(primary_key => object.id).update_all( + updates.merge(touch_updates(object, touch)) + ) end return true end @@ -109,10 +106,7 @@ module ActiveRecord if touch object = find(id) - touch_time = object.send(:current_time_from_proper_timezone) - timestamps = resolve_timestamp_columns(object, touch) - - timestamps.map do |column| + touch_updates(object, touch).map do |column, touch_time| updates << "#{connection.quote_column_name(column.to_s)} = #{connection.quote(touch_time)}" end end @@ -144,7 +138,7 @@ module ActiveRecord # # and update the updated_at value. # DiscussionBoard.increment_counter(:posts_count, 5, touch: true) def increment_counter(counter_name, id, touch: nil) - update_counters(id, { counter_name => 1 }.merge(touch: touch)) + update_counters(id, counter_name => 1, touch: touch) end # Decrement a numeric field by one, via a direct SQL update. @@ -169,16 +163,14 @@ module ActiveRecord # # and update the updated_at value. # DiscussionBoard.decrement_counter(:posts_count, 5, touch: true) def decrement_counter(counter_name, id, touch: nil) - update_counters(id, { counter_name => -1 }.merge(touch: touch)) + update_counters(id, counter_name => -1, touch: touch) end private - def resolve_timestamp_columns(object, touch) - if touch == true - object.send(:timestamp_attributes_for_update_in_model) - else - Array(touch) - end + def touch_updates(object, touch) + touch = object.send(:timestamp_attributes_for_update_in_model) if touch == true + touch_time = object.send(:current_time_from_proper_timezone) + Array(touch).map { |column| [ column, touch_time ] }.to_h end end diff --git a/activerecord/test/cases/counter_cache_test.rb b/activerecord/test/cases/counter_cache_test.rb index 478b44008d..c735e13715 100644 --- a/activerecord/test/cases/counter_cache_test.rb +++ b/activerecord/test/cases/counter_cache_test.rb @@ -212,144 +212,135 @@ class CounterCacheTest < ActiveRecord::TestCase end end - test "does not update counters with touch: false" do - previous_updated_at = @topic.updated_at - Topic.update_counters(@topic.id, replies_count: -1, touch: false) - assert_equal previous_updated_at, @topic.reload.updated_at + test "update counters doesn't touch timestamps by default" do + @topic.update_column :updated_at, 5.minutes.ago + previously_updated_at = @topic.updated_at + + Topic.update_counters(@topic.id, replies_count: -1) + + assert_equal previously_updated_at, @topic.updated_at end test "update counters with touch: true" do - previous_updated_at = @topic.updated_at - Topic.update_counters(@topic.id, replies_count: -1, touch: true) - assert_not_equal previous_updated_at, @topic.reload.updated_at + assert_touching @topic, :updated_at do + Topic.update_counters(@topic.id, replies_count: -1, touch: true) + end end test "update multiple counters with touch: true" do - previous_updated_at = @topic.updated_at - Topic.update_counters(@topic.id, replies_count: 2, unique_replies_count: 2, touch: true) - assert_not_equal previous_updated_at, @topic.reload.updated_at + assert_touching @topic, :updated_at do + Topic.update_counters(@topic.id, replies_count: 2, unique_replies_count: 2, touch: true) + end end test "reset counters with touch: true" do - previous_updated_at = @topic.updated_at - Topic.reset_counters(@topic.id, :replies, touch: true) - assert_not_equal previous_updated_at, @topic.reload.updated_at + assert_touching @topic, :updated_at do + Topic.reset_counters(@topic.id, :replies, touch: true) + end end test "reset multiple counters with touch: true" do - previous_updated_at = @topic.updated_at - Topic.update_counters(@topic.id, replies_count: 1, unique_replies_count: 1) - Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: true) - assert_not_equal previous_updated_at, @topic.reload.updated_at + assert_touching @topic, :updated_at do + Topic.update_counters(@topic.id, replies_count: 1, unique_replies_count: 1) + Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: true) + end end test "increment counters with touch: true" do - previous_updated_at = @topic.updated_at - Topic.increment_counter(:replies_count, @topic.id, touch: true) - assert_not_equal previous_updated_at, @topic.reload.updated_at + assert_touching @topic, :updated_at do + Topic.increment_counter(:replies_count, @topic.id, touch: true) + end end test "decrement counters with touch: true" do - previous_updated_at = @topic.updated_at - Topic.decrement_counter(:replies_count, @topic.id, touch: true) - assert_not_equal previous_updated_at, @topic.reload.updated_at + assert_touching @topic, :updated_at do + Topic.decrement_counter(:replies_count, @topic.id, touch: true) + end end test "update counters with touch: :written_on" do - previous_written_on = @topic.written_on - Topic.update_counters(@topic.id, replies_count: -1, touch: :written_on) - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :written_on do + Topic.update_counters(@topic.id, replies_count: -1, touch: :written_on) + end end test "update multiple counters with touch: :written_on" do - previous_written_on = @topic.written_on - Topic.update_counters(@topic.id, replies_count: 2, unique_replies_count: 2, touch: :written_on) - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :written_on do + Topic.update_counters(@topic.id, replies_count: 2, unique_replies_count: 2, touch: :written_on) + end end test "reset counters with touch: :written_on" do - previous_written_on = @topic.written_on - Topic.reset_counters(@topic.id, :replies, touch: :written_on) - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :written_on do + Topic.reset_counters(@topic.id, :replies, touch: :written_on) + end end test "reset multiple counters with touch: :written_on" do - previous_written_on = @topic.written_on - Topic.update_counters(@topic.id, replies_count: 1, unique_replies_count: 1) - Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: :written_on) - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :written_on do + Topic.update_counters(@topic.id, replies_count: 1, unique_replies_count: 1) + Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: :written_on) + end end test "increment counters with touch: :written_on" do - previous_written_on = @topic.written_on - Topic.increment_counter(:replies_count, @topic.id, touch: :written_on) - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :written_on do + Topic.increment_counter(:replies_count, @topic.id, touch: :written_on) + end end test "decrement counters with touch: :written_on" do - previous_written_on = @topic.written_on - Topic.decrement_counter(:replies_count, @topic.id, touch: :written_on) - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :written_on do + Topic.decrement_counter(:replies_count, @topic.id, touch: :written_on) + end end test "update counters with touch: %i( updated_at written_on )" do - previous_updated_at = @topic.updated_at - previous_written_on = @topic.written_on - - Topic.update_counters(@topic.id, replies_count: -1, touch: %i( updated_at written_on )) - - assert_not_equal previous_updated_at, @topic.reload.updated_at - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :updated_at, :written_on do + Topic.update_counters(@topic.id, replies_count: -1, touch: %i( updated_at written_on )) + end end test "update multiple counters with touch: %i( updated_at written_on )" do - previous_updated_at = @topic.updated_at - previous_written_on = @topic.written_on - - Topic.update_counters(@topic.id, replies_count: 2, unique_replies_count: 2, touch: %i( updated_at written_on )) - - assert_not_equal previous_updated_at, @topic.reload.updated_at - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :updated_at, :written_on do + Topic.update_counters(@topic.id, replies_count: 2, unique_replies_count: 2, touch: %i( updated_at written_on )) + end end test "reset counters with touch: %i( updated_at written_on )" do - previous_updated_at = @topic.updated_at - previous_written_on = @topic.written_on - - Topic.reset_counters(@topic.id, :replies, touch: %i( updated_at written_on )) - - assert_not_equal previous_updated_at, @topic.reload.updated_at - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :updated_at, :written_on do + Topic.reset_counters(@topic.id, :replies, touch: %i( updated_at written_on )) + end end test "reset multiple counters with touch: %i( updated_at written_on )" do - previous_updated_at = @topic.updated_at - previous_written_on = @topic.written_on - - Topic.update_counters(@topic.id, replies_count: 1, unique_replies_count: 1) - Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: %i( updated_at written_on )) - - assert_not_equal previous_updated_at, @topic.reload.updated_at - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :updated_at, :written_on do + Topic.update_counters(@topic.id, replies_count: 1, unique_replies_count: 1) + Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: %i( updated_at written_on )) + end end test "increment counters with touch: %i( updated_at written_on )" do - previous_updated_at = @topic.updated_at - previous_written_on = @topic.written_on - - Topic.increment_counter(:replies_count, @topic.id, touch: %i( updated_at written_on )) - - assert_not_equal previous_updated_at, @topic.reload.updated_at - assert_not_equal previous_written_on, @topic.reload.written_on + assert_touching @topic, :updated_at, :written_on do + Topic.increment_counter(:replies_count, @topic.id, touch: %i( updated_at written_on )) + end end test "decrement counters with touch: %i( updated_at written_on )" do - previous_updated_at = @topic.updated_at - previous_written_on = @topic.written_on + assert_touching @topic, :updated_at, :written_on do + Topic.decrement_counter(:replies_count, @topic.id, touch: %i( updated_at written_on )) + end + end - Topic.decrement_counter(:replies_count, @topic.id, touch: %i( updated_at written_on )) + private + def assert_touching(record, *attributes) + record.update_columns attributes.map { |attr| [ attr, 5.minutes.ago ] }.to_h + touch_times = attributes.map { |attr| [ attr, record.public_send(attr) ] }.to_h - assert_not_equal previous_updated_at, @topic.reload.updated_at - assert_not_equal previous_written_on, @topic.reload.written_on - end + yield + + touch_times.each do |attr, previous_touch_time| + assert_operator previous_touch_time, :<, record.reload.public_send(attr) + end + end end -- cgit v1.2.3