Merge pull request #27543 from kamipo/fix_update_counters_of_multiple_records_with_touch

Fix update counters of multiple records with touch: true

4 files changed, 50 insertions, 41 deletions

diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb
index c654d4703a..93b9371206 100644
--- a/activerecord/lib/active_record/counter_cache.rb
+++ b/activerecord/lib/active_record/counter_cache.rb
@@ -46,7 +46,7 @@ module ActiveRecord
           counter_name = reflection.counter_cache_column
 
           updates = { counter_name.to_sym => object.send(counter_association).count(:all) }
-          updates.merge!(touch_updates(object, touch)) if touch
+          updates.merge!(touch_updates(touch)) if touch
 
           unscoped.where(primary_key => object.id).update_all(updates)
         end
@@ -105,8 +105,7 @@ module ActiveRecord
         end
 
         if touch
-          object = find(id)
-          updates << object.class.send(:sanitize_sql_for_assignment, touch_updates(object, touch))
+          updates << sanitize_sql_for_assignment(touch_updates(touch))
         end
 
         unscoped.where(primary_key => id).update_all updates.join(", ")
@@ -165,9 +164,9 @@ module ActiveRecord
       end
 
       private
-        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)
+        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
diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb
index 030fc35e2f..09d8d1cdd4 100644
--- a/activerecord/lib/active_record/timestamp.rb
+++ b/activerecord/lib/active_record/timestamp.rb
@@ -52,14 +52,41 @@ module ActiveRecord
       clear_timestamp_attributes
     end
 
+    class_methods do
+      private
+        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
+
+        def timestamp_attributes_for_create
+          ["created_at", "created_on"]
+        end
+
+        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
 
     def _create_record
       if record_timestamps
         current_time = current_time_from_proper_timezone
 
-        all_timestamp_attributes.each do |column|
-          if has_attribute?(column) && !attribute_present?(column)
+        all_timestamp_attributes_in_model.each do |column|
+          if !attribute_present?(column)
             write_attribute(column, current_time)
           end
         end
@@ -85,30 +112,22 @@ module ActiveRecord
     end
 
     def timestamp_attributes_for_create_in_model
-      timestamp_attributes_for_create.select { |c| self.class.column_names.include?(c) }
+      self.class.send(:timestamp_attributes_for_create_in_model)
     end
 
     def timestamp_attributes_for_update_in_model
-      timestamp_attributes_for_update.select { |c| self.class.column_names.include?(c) }
+      self.class.send(:timestamp_attributes_for_update_in_model)
     end
 
     def all_timestamp_attributes_in_model
-      timestamp_attributes_for_create_in_model + timestamp_attributes_for_update_in_model
-    end
-
-    def timestamp_attributes_for_update
-      ["updated_at", "updated_on"]
-    end
-
-    def timestamp_attributes_for_create
-      ["created_at", "created_on"]
+      self.class.send(:all_timestamp_attributes_in_model)
     end
 
-    def all_timestamp_attributes
-      timestamp_attributes_for_create + timestamp_attributes_for_update
+    def 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)
+    def max_updated_column_timestamp(timestamp_names = self.class.send(:timestamp_attributes_for_update))
       timestamp_names
         .map { |attr| self[attr] }
         .compact
@@ -116,10 +135,6 @@ module ActiveRecord
         .max
     end
 
-    def current_time_from_proper_timezone
-      self.class.default_timezone == :utc ? Time.now.utc : Time.now
-    end
-
     # Clear attributes and changed_attributes
     def clear_timestamp_attributes
       all_timestamp_attributes_in_model.each do |attribute_name|
diff --git a/activerecord/test/cases/counter_cache_test.rb b/activerecord/test/cases/counter_cache_test.rb
index c735e13715..c7d0ba32b4 100644
--- a/activerecord/test/cases/counter_cache_test.rb
+++ b/activerecord/test/cases/counter_cache_test.rb
@@ -227,6 +227,16 @@ class CounterCacheTest < ActiveRecord::TestCase
     end
   end
 
+  test "update counters of multiple records with touch: true" do
+    t1, t2 = topics(:first, :second)
+
+    assert_touching t1, :updated_at do
+      assert_difference ["t1.reload.replies_count", "t2.reload.replies_count"], 2 do
+        Topic.update_counters([t1.id, t2.id], replies_count: 2, touch: true)
+      end
+    end
+  end
+
   test "update multiple counters with touch: true" do
     assert_touching @topic, :updated_at do
       Topic.update_counters(@topic.id, replies_count: 2, unique_replies_count: 2, touch: true)
diff --git a/activerecord/test/cases/timestamp_test.rb b/activerecord/test/cases/timestamp_test.rb
index 7327926706..7766a74612 100644
--- a/activerecord/test/cases/timestamp_test.rb
+++ b/activerecord/test/cases/timestamp_test.rb
@@ -430,21 +430,6 @@ class TimestampTest < ActiveRecord::TestCase
     assert_not_equal person.born_at, nil
   end
 
-  def test_timestamp_attributes_for_create
-    toy = Toy.first
-    assert_equal ["created_at", "created_on"], toy.send(:timestamp_attributes_for_create)
-  end
-
-  def test_timestamp_attributes_for_update
-    toy = Toy.first
-    assert_equal ["updated_at", "updated_on"], toy.send(:timestamp_attributes_for_update)
-  end
-
-  def test_all_timestamp_attributes
-    toy = Toy.first
-    assert_equal ["created_at", "created_on", "updated_at", "updated_on"], toy.send(:all_timestamp_attributes)
-  end
-
   def test_timestamp_attributes_for_create_in_model
     toy = Toy.first
     assert_equal ["created_at"], toy.send(:timestamp_attributes_for_create_in_model)