Fix touch option to behave consistently with Persistence#touch method

`touch` option was added to `increment!` (#27660) and `update_counters`
(#26995). But that option behaves inconsistently with
`Persistence#touch` method.

If `touch` option is passed attribute names, it won't update
update_at/on attributes unlike `Persistence#touch` method.

Due to changed from `Persistence#touch` to `increment!` with `touch`
option, #31405 has a regression that `counter_cache` with `touch` option
which is passed attribute names won't update update_at/on attributes.

I think that the inconsistency is not intended. To get back consistency,
ensure that `touch` option updates update_at/on attributes.

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Fix `touch` option to behave consistently with `Persistence#touch` method.
+
+    *Ryuta Kamizono*
+
 *   Migrations raise when duplicate column definition.
 
     Fixes #33024.

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/relation.rb

@@ -393,10 +393,7 @@ def update_all(updates)
     #   Person.where(name: 'David').touch_all
     #   # => "UPDATE \"people\" SET \"updated_at\" = '2018-01-04 22:55:23.132670' WHERE \"people\".\"name\" = 'David'"
     def touch_all(*names, time: nil)
-      attributes = Array(names) + klass.timestamp_attributes_for_update_in_model
-      time ||= klass.current_time_from_proper_timezone
-      updates = {}
-      attributes.each { |column| updates[column] = time }
+      updates = touch_attributes_with_time(*names, time: time)
 
       if klass.locking_enabled?
         quoted_locking_column = connection.quote_column_name(klass.locking_column)

activerecord/lib/active_record/timestamp.rb

@@ -53,12 +53,10 @@ def initialize_dup(other) # :nodoc:
     end
 
     module ClassMethods # :nodoc:
-      def timestamp_attributes_for_update_in_model
-        timestamp_attributes_for_update.select { |c| column_names.include?(c) }
-      end
-
-      def current_time_from_proper_timezone
-        default_timezone == :utc ? Time.now.utc : Time.now
+      def touch_attributes_with_time(*names, time: nil)
+        attribute_names = timestamp_attributes_for_update_in_model
+        attribute_names |= names.map(&:to_s)
+        attribute_names.index_with(time ||= current_time_from_proper_timezone)
       end
 
       private
@@ -66,6 +64,10 @@ def timestamp_attributes_for_create_in_model
           timestamp_attributes_for_create.select { |c| column_names.include?(c) }
         end
 
+        def timestamp_attributes_for_update_in_model
+          timestamp_attributes_for_update.select { |c| column_names.include?(c) }
+        end
+
         def all_timestamp_attributes_in_model
           timestamp_attributes_for_create_in_model + timestamp_attributes_for_update_in_model
         end
@@ -77,6 +79,10 @@ def timestamp_attributes_for_create
         def timestamp_attributes_for_update
           ["updated_at", "updated_on"]
         end
+
+        def current_time_from_proper_timezone
+          default_timezone == :utc ? Time.now.utc : Time.now
+        end
     end
 
   private
@@ -116,7 +122,7 @@ def timestamp_attributes_for_create_in_model
     end
 
     def timestamp_attributes_for_update_in_model
-      self.class.timestamp_attributes_for_update_in_model
+      self.class.send(:timestamp_attributes_for_update_in_model)
     end
 
     def all_timestamp_attributes_in_model
@@ -124,7 +130,7 @@ def all_timestamp_attributes_in_model
     end
 
     def current_time_from_proper_timezone
-      self.class.current_time_from_proper_timezone
+      self.class.send(:current_time_from_proper_timezone)
     end
 
     def max_updated_column_timestamp(timestamp_names = timestamp_attributes_for_update_in_model)

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,32 +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
-    travel(1.day) do
+    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

@@ -1,5 +1,5 @@
 # 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

@@ -36,6 +36,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|
@@ -126,7 +127,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