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:
  d753645d40/activerecord/test/cases/timestamp_test.rb (L17)

* Expand changelog entry.

  Elaborate and show examples.

Closes #26995.

[ Jarred Trost & Kasper Timm Hansen ]

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.
 

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
 

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
-
-    Topic.decrement_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.decrement_counter(:replies_count, @topic.id, touch: %i( updated_at written_on ))
+    end
   end
+
+  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
+
+      yield
+
+      touch_times.each do |attr, previous_touch_time|
+        assert_operator previous_touch_time, :<, record.reload.public_send(attr)
+      end
+    end
 end