Merge pull request #35352 from kamipo/update_all_doesnt_care_optimistic_locking

Ensure `update_all` series doesn't care optimistic locking

4 files changed, 83 insertions, 19 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 99a76b5b94..57f831bac3 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Ensure `update_all` series cares about optimistic locking.
+
+    *Ryuta Kamizono*
+
 *   Don't allow `where` with non numeric string matches to 0 values.
 
     *Ryuta Kamizono*
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index f6b21eaa3a..d612ff53c1 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -372,6 +372,12 @@ module ActiveRecord
       stmt.wheres = arel.constraints
 
       if updates.is_a?(Hash)
+        if klass.locking_enabled? &&
+            !updates.key?(klass.locking_column) &&
+            !updates.key?(klass.locking_column.to_sym)
+          attr = arel_attribute(klass.locking_column)
+          updates[attr.name] = _increment_attribute(attr)
+        end
         stmt.set _substitute_values(updates)
       else
         stmt.set Arel.sql(klass.sanitize_sql_for_assignment(updates, table.name))
@@ -394,10 +400,7 @@ module ActiveRecord
       updates = {}
       counters.each do |counter_name, value|
         attr = arel_attribute(counter_name)
-        bind = predicate_builder.build_bind_attribute(attr.name, value.abs)
-        expr = table.coalesce(Arel::Nodes::UnqualifiedColumn.new(attr), 0)
-        expr = value < 0 ? expr - bind : expr + bind
-        updates[counter_name] = expr.expr
+        updates[attr.name] = _increment_attribute(attr, value)
       end
 
       if touch
@@ -433,12 +436,7 @@ module ActiveRecord
     #   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)
-      if klass.locking_enabled?
-        names << { time: time }
-        update_counters(klass.locking_column => 1, touch: names)
-      else
-        update_all klass.touch_attributes_with_time(*names, time: time)
-      end
+      update_all klass.touch_attributes_with_time(*names, time: time)
     end
 
     # Destroys the records by instantiating each
@@ -709,6 +707,13 @@ module ActiveRecord
         end
       end
 
+      def _increment_attribute(attribute, value = 1)
+        bind = predicate_builder.build_bind_attribute(attribute.name, value.abs)
+        expr = table.coalesce(Arel::Nodes::UnqualifiedColumn.new(attribute), 0)
+        expr = value < 0 ? expr - bind : expr + bind
+        expr.expr
+      end
+
       def exec_queries(&block)
         skip_query_cache_if_necessary do
           @records =
diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb
index dfd74bfcb4..a2a501a794 100644
--- a/activerecord/test/cases/dirty_test.rb
+++ b/activerecord/test/cases/dirty_test.rb
@@ -352,7 +352,7 @@ class DirtyTest < ActiveRecord::TestCase
       Person.where(id: person.id).update_all(first_name: "baz")
     end
 
-    old_lock_version = person.lock_version
+    old_lock_version = person.lock_version + 1
 
     with_partial_writes Person, true do
       assert_no_queries { 2.times { person.save! } }
diff --git a/activerecord/test/cases/relation/update_all_test.rb b/activerecord/test/cases/relation/update_all_test.rb
index bb6912148c..0500574f28 100644
--- a/activerecord/test/cases/relation/update_all_test.rb
+++ b/activerecord/test/cases/relation/update_all_test.rb
@@ -138,14 +138,6 @@ class UpdateAllTest < ActiveRecord::TestCase
     assert_equal new_time, developer.updated_at
   end
 
-  def test_touch_all_updates_locking_column
-    person = people(:david)
-
-    assert_difference -> { person.reload.lock_version }, +1 do
-      Person.where(first_name: "David").touch_all
-    end
-  end
-
   def test_update_on_relation
     topic1 = TopicWithCallbacks.create! title: "arel", author_name: nil
     topic2 = TopicWithCallbacks.create! title: "activerecord", author_name: nil
@@ -186,6 +178,69 @@ class UpdateAllTest < ActiveRecord::TestCase
     end
   end
 
+  def test_update_all_cares_about_optimistic_locking
+    david = people(:david)
+
+    travel 5.seconds do
+      now = Time.now.utc
+      assert_not_equal now, david.updated_at
+
+      people = Person.where(id: people(:michael, :david, :susan))
+      expected = people.pluck(:lock_version)
+      expected.map! { |version| version + 1 }
+      people.update_all(updated_at: now)
+
+      assert_equal [now] * 3, people.pluck(:updated_at)
+      assert_equal expected, people.pluck(:lock_version)
+
+      assert_raises(ActiveRecord::StaleObjectError) do
+        david.touch(time: now)
+      end
+    end
+  end
+
+  def test_update_counters_cares_about_optimistic_locking
+    david = people(:david)
+
+    travel 5.seconds do
+      now = Time.now.utc
+      assert_not_equal now, david.updated_at
+
+      people = Person.where(id: people(:michael, :david, :susan))
+      expected = people.pluck(:lock_version)
+      expected.map! { |version| version + 1 }
+      people.update_counters(touch: [time: now])
+
+      assert_equal [now] * 3, people.pluck(:updated_at)
+      assert_equal expected, people.pluck(:lock_version)
+
+      assert_raises(ActiveRecord::StaleObjectError) do
+        david.touch(time: now)
+      end
+    end
+  end
+
+  def test_touch_all_cares_about_optimistic_locking
+    david = people(:david)
+
+    travel 5.seconds do
+      now = Time.now.utc
+      assert_not_equal now, david.updated_at
+
+      people = Person.where(id: people(:michael, :david, :susan))
+      expected = people.pluck(:lock_version)
+      expected.map! { |version| version + 1 }
+      people.touch_all(time: now)
+
+      assert_equal [now] * 3, people.pluck(:updated_at)
+      assert_equal expected, people.pluck(:lock_version)
+
+      assert_raises(ActiveRecord::StaleObjectError) do
+        david.touch(time: now)
+      end
+    end
+  end
+
   # Oracle UPDATE does not support ORDER BY
   unless current_adapter?(:OracleAdapter)
     def test_update_all_ignores_order_without_limit_from_association