Merge pull request #33148 from kamipo/backport_33107

5-2-stable: Fix `touch` option to behave consistently with `Persistence#touch` method

activerecord/CHANGELOG.md

+*   Fix `touch` option to behave consistently with `Persistence#touch` method.
+
+    *Ryuta Kamizono*
+
 *   Fix `save` in `after_create_commit` won't invoke extra `after_create_commit`.
 
     Fixes #32831.

activerecord/lib/active_record/counter_cache.rb

@@ -47,8 +47,12 @@ def reset_counters(id, *counters, touch: nil)
           reflection   = child_class._reflections.values.find { |e| e.belongs_to? && e.foreign_key.to_s == foreign_key && e.options[:counter_cache].present? }
           counter_name = reflection.counter_cache_column
 
-          updates = { counter_name.to_sym => object.send(counter_association).count(:all) }
-          updates.merge!(touch_updates(touch)) if touch
+          updates = { counter_name => object.send(counter_association).count(:all) }
+
+          if touch
+            names = touch if touch != true
+            updates.merge!(touch_attributes_with_time(*names))
+          end
 
           unscoped.where(primary_key => object.id).update_all(updates)
         end
@@ -68,8 +72,8 @@ def reset_counters(id, *counters, touch: nil)
       # * +counters+ - A Hash containing the names of the fields
       #   to update as keys and the amount to update the field by as values.
       # * <tt>:touch</tt> option - Touch timestamp columns when updating.
-      #   Pass +true+ to touch +updated_at+ and/or +updated_on+. Pass a symbol to
-      #   touch that column or an array of symbols to touch just those ones.
+      #   If attribute names are passed, they are updated along with updated_at/on
+      #   attributes.
       #
       # ==== Examples
       #
@@ -107,7 +111,8 @@ def update_counters(id, counters)
         end
 
         if touch
-          touch_updates = touch_updates(touch)
+          names = touch if touch != true
+          touch_updates = touch_attributes_with_time(*names)
           updates << sanitize_sql_for_assignment(touch_updates) unless touch_updates.empty?
         end
 
@@ -171,13 +176,6 @@ def increment_counter(counter_name, id, touch: nil)
       def decrement_counter(counter_name, id, touch: nil)
         update_counters(id, counter_name => -1, touch: touch)
       end
-
-      private
-        def touch_updates(touch)
-          touch = timestamp_attributes_for_update_in_model if touch == true
-          touch_time = current_time_from_proper_timezone
-          Array(touch).map { |column| [ column, touch_time ] }.to_h
-        end
     end
 
     private

activerecord/lib/active_record/timestamp.rb

@@ -53,6 +53,13 @@ def initialize_dup(other) # :nodoc:
     end
 
     module ClassMethods # :nodoc:
+      def touch_attributes_with_time(*names, time: nil)
+        attribute_names = timestamp_attributes_for_update_in_model
+        attribute_names |= names.map(&:to_s)
+        time ||= current_time_from_proper_timezone
+        attribute_names.each_with_object({}) { |attr_name, result| result[attr_name] = time }
+      end
+
       private
         def timestamp_attributes_for_create_in_model
           timestamp_attributes_for_create.select { |c| column_names.include?(c) }

activerecord/test/cases/counter_cache_test.rb

@@ -280,38 +280,38 @@ class ::SpecialReply < ::Reply
   end
 
   test "update counters with touch: :written_on" do
-    assert_touching @topic, :written_on do
+    assert_touching @topic, :updated_at, :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
-    assert_touching @topic, :written_on do
+    assert_touching @topic, :updated_at, :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
-    assert_touching @topic, :written_on do
+    assert_touching @topic, :updated_at, :written_on do
       Topic.reset_counters(@topic.id, :replies, touch: :written_on)
     end
   end
 
   test "reset multiple counters with touch: :written_on" do
-    assert_touching @topic, :written_on do
+    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: :written_on)
     end
   end
 
   test "increment counters with touch: :written_on" do
-    assert_touching @topic, :written_on do
+    assert_touching @topic, :updated_at, :written_on do
       Topic.increment_counter(:replies_count, @topic.id, touch: :written_on)
     end
   end
 
   test "decrement counters with touch: :written_on" do
-    assert_touching @topic, :written_on do
+    assert_touching @topic, :updated_at, :written_on do
       Topic.decrement_counter(:replies_count, @topic.id, touch: :written_on)
     end
   end

activerecord/test/cases/locking_test.rb

@@ -445,30 +445,38 @@ def test_counter_cache_with_touch_and_lock_version
     assert_equal 0, car.wheels_count
     assert_equal 0, car.lock_version
 
-    previously_car_updated_at = car.updated_at
-    travel(2.second) do
+    previously_updated_at = car.updated_at
+    previously_wheels_owned_at = car.wheels_owned_at
+    travel(1.second) do
       Wheel.create!(wheelable: car)
     end
 
     assert_equal 1, car.reload.wheels_count
-    assert_not_equal previously_car_updated_at, car.updated_at
     assert_equal 1, car.lock_version
+    assert_operator previously_updated_at, :<, car.updated_at
+    assert_operator previously_wheels_owned_at, :<, car.wheels_owned_at
 
-    previously_car_updated_at = car.updated_at
-    car.wheels.first.update(size: 42)
+    previously_updated_at = car.updated_at
+    previously_wheels_owned_at = car.wheels_owned_at
+    travel(2.second) do
+      car.wheels.first.update(size: 42)
+    end
 
     assert_equal 1, car.reload.wheels_count
-    assert_not_equal previously_car_updated_at, car.updated_at
     assert_equal 2, car.lock_version
+    assert_operator previously_updated_at, :<, car.updated_at
+    assert_operator previously_wheels_owned_at, :<, car.wheels_owned_at
 
-    previously_car_updated_at = car.updated_at
-    travel(2.second) do
+    previously_updated_at = car.updated_at
+    previously_wheels_owned_at = car.wheels_owned_at
+    travel(3.second) do
       car.wheels.first.destroy!
     end
 
     assert_equal 0, car.reload.wheels_count
-    assert_not_equal previously_car_updated_at, car.updated_at
     assert_equal 3, car.lock_version
+    assert_operator previously_updated_at, :<, car.updated_at
+    assert_operator previously_wheels_owned_at, :<, car.wheels_owned_at
   end
 
   def test_polymorphic_destroy_with_dependencies_and_lock_version

activerecord/test/cases/persistence_test.rb

@@ -206,12 +206,28 @@ def test_increment_updates_counter_in_db_using_offset
     assert_equal initial_credit + 2, a1.reload.credit_limit
   end
 
-  def test_increment_updates_timestamps
+  def test_increment_with_touch_updates_timestamps
     topic = topics(:first)
-    topic.update_columns(updated_at: 5.minutes.ago)
-    previous_updated_at = topic.updated_at
-    topic.increment!(:replies_count, touch: true)
-    assert_operator previous_updated_at, :<, topic.reload.updated_at
+    assert_equal 1, topic.replies_count
+    previously_updated_at = topic.updated_at
+    travel(1.second) do
+      topic.increment!(:replies_count, touch: true)
+    end
+    assert_equal 2, topic.reload.replies_count
+    assert_operator previously_updated_at, :<, topic.updated_at
+  end
+
+  def test_increment_with_touch_an_attribute_updates_timestamps
+    topic = topics(:first)
+    assert_equal 1, topic.replies_count
+    previously_updated_at = topic.updated_at
+    previously_written_on = topic.written_on
+    travel(1.second) do
+      topic.increment!(:replies_count, touch: :written_on)
+    end
+    assert_equal 2, topic.reload.replies_count
+    assert_operator previously_updated_at, :<, topic.updated_at
+    assert_operator previously_written_on, :<, topic.written_on
   end
 
   def test_destroy_all
@@ -333,12 +349,28 @@ def test_decrement_attribute_by
     assert_equal 41, accounts(:signals37, :reload).credit_limit
   end
 
-  def test_decrement_updates_timestamps
+  def test_decrement_with_touch_updates_timestamps
     topic = topics(:first)
-    topic.update_columns(updated_at: 5.minutes.ago)
-    previous_updated_at = topic.updated_at
-    topic.decrement!(:replies_count, touch: true)
-    assert_operator previous_updated_at, :<, topic.reload.updated_at
+    assert_equal 1, topic.replies_count
+    previously_updated_at = topic.updated_at
+    travel(1.second) do
+      topic.decrement!(:replies_count, touch: true)
+    end
+    assert_equal 0, topic.reload.replies_count
+    assert_operator previously_updated_at, :<, topic.updated_at
+  end
+
+  def test_decrement_with_touch_an_attribute_updates_timestamps
+    topic = topics(:first)
+    assert_equal 1, topic.replies_count
+    previously_updated_at = topic.updated_at
+    previously_written_on = topic.written_on
+    travel(1.second) do
+      topic.decrement!(:replies_count, touch: :written_on)
+    end
+    assert_equal 0, topic.reload.replies_count
+    assert_operator previously_updated_at, :<, topic.updated_at
+    assert_operator previously_written_on, :<, topic.written_on
   end
 
   def test_create

activerecord/test/models/car.rb

@@ -20,6 +20,8 @@ class Car < ActiveRecord::Base
   scope :incl_engines, -> { includes(:engines) }
 
   scope :order_using_new_style,  -> { order("name asc") }
+
+  attribute :wheels_owned_at, :datetime, default: -> { Time.now }
 end
 
 class CoolCar < Car

activerecord/test/models/wheel.rb

 # frozen_string_literal: true
 
 class Wheel < ActiveRecord::Base
-  belongs_to :wheelable, polymorphic: true, counter_cache: true, touch: true
+  belongs_to :wheelable, polymorphic: true, counter_cache: true, touch: :wheels_owned_at
 end

activerecord/test/schema/schema.rb

@@ -33,6 +33,7 @@
   create_table :aircraft, force: true do |t|
     t.string :name
     t.integer :wheels_count, default: 0, null: false
+    t.datetime :wheels_owned_at
   end
 
   create_table :articles, force: true do |t|
@@ -123,7 +124,8 @@
   create_table :cars, force: true do |t|
     t.string  :name
     t.integer :engines_count
-    t.integer :wheels_count, default: 0
+    t.integer :wheels_count, default: 0, null: false
+    t.datetime :wheels_owned_at
     t.column :lock_version, :integer, null: false, default: 0
     t.timestamps null: false
   end