Merge pull request #26050 from bogdanvlviv/optimistic_locking

Fixed: Optimistic locking does not work correctly

5 files changed, 157 insertions, 20 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 15b49e0a0b..f847c71420 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,14 @@
+*   Optimistic locking: Added ability update locking_column value.
+    Ignore optimistic locking if update with new locking_column value.
+
+    *bogdanvlviv*
+
+*   Fixed: Optimistic locking does not work well with null in the database.
+
+    Fixes #26024
+
+    *bogdanvlviv*
+
 *   Fixed support for case insensitive comparisons of `text` columns in
     PostgreSQL.
 
diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb
index 8e8a97990a..d39b7181f4 100644
--- a/activerecord/lib/active_record/locking/optimistic.rb
+++ b/activerecord/lib/active_record/locking/optimistic.rb
@@ -60,6 +60,7 @@ module ActiveRecord
       end
 
       private
+
         def increment_lock
           lock_col = self.class.locking_column
           previous_lock_value = send(lock_col).to_i
@@ -77,21 +78,24 @@ module ActiveRecord
 
         def _update_record(attribute_names = self.attribute_names) #:nodoc:
           return super unless locking_enabled?
-          return 0 if attribute_names.empty?
 
           lock_col = self.class.locking_column
-          previous_lock_value = send(lock_col).to_i
-          increment_lock
 
-          attribute_names += [lock_col]
-          attribute_names.uniq!
+          return super if attribute_names.include?(lock_col)
+          return 0 if attribute_names.empty?
 
           begin
+            previous_lock_value = read_attribute_before_type_cast(lock_col)
+
+            increment_lock
+
+            attribute_names.push(lock_col)
+
             relation = self.class.unscoped
 
             affected_rows = relation.where(
               self.class.primary_key => id,
-              lock_col => previous_lock_value,
+              lock_col => previous_lock_value
             ).update_all(
               attributes_for_update(attribute_names).map do |name|
                 [name, _read_attribute(name)]
@@ -104,9 +108,9 @@ module ActiveRecord
 
             affected_rows
 
-          # If something went wrong, revert the version.
+          # If something went wrong, revert the locking_column value.
           rescue Exception
-            send(lock_col + "=", previous_lock_value)
+            send(lock_col + "=", previous_lock_value.to_i)
             raise
           end
         end
diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb
index 09bd00291d..3bd8475bb0 100644
--- a/activerecord/test/cases/dirty_test.rb
+++ b/activerecord/test/cases/dirty_test.rb
@@ -341,7 +341,7 @@ class DirtyTest < ActiveRecord::TestCase
 
   def test_partial_update_with_optimistic_locking
     person = Person.new(first_name: "foo")
-    old_lock_version = 1
+    old_lock_version = person.lock_version
 
     with_partial_writes Person, false do
       assert_queries(2) { 2.times { person.save! } }
diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb
index 13b6f6daaf..0579df0a07 100644
--- a/activerecord/test/cases/locking_test.rb
+++ b/activerecord/test/cases/locking_test.rb
@@ -161,14 +161,6 @@ class OptimisticLockingTest < ActiveRecord::TestCase
     assert_equal(error.record.object_id, p2.object_id)
   end
 
-  def test_lock_new_with_nil
-    p1 = Person.new(first_name: "anika")
-    p1.save!
-    p1.lock_version = nil # simulate bad fixture or column with no default
-    p1.save!
-    assert_equal 1, p1.lock_version
-  end
-
   def test_lock_new_when_explicitly_passing_nil
     p1 = Person.new(first_name: "anika", lock_version: nil)
     p1.save!
@@ -222,22 +214,149 @@ class OptimisticLockingTest < ActiveRecord::TestCase
 
   def test_lock_without_default_sets_version_to_zero
     t1 = LockWithoutDefault.new
+
+    assert_equal 0, t1.lock_version
+    assert_nil t1.lock_version_before_type_cast
+
+    t1.save!
+    t1.reload
+
     assert_equal 0, t1.lock_version
+    assert_equal 0, t1.lock_version_before_type_cast
+  end
+
+  def test_lock_without_default_should_work_with_null_in_the_database
+    ActiveRecord::Base.connection.execute("INSERT INTO lock_without_defaults(title) VALUES('title1')")
+    t1 = LockWithoutDefault.last
+    t2 = LockWithoutDefault.last
+
+    assert_equal 0, t1.lock_version
+    assert_nil t1.lock_version_before_type_cast
+    assert_equal 0, t2.lock_version
+    assert_nil t2.lock_version_before_type_cast
+
+    t1.title = "new title1"
+    t2.title = "new title2"
+
+    assert_nothing_raised { t1.save! }
+    assert_equal 1, t1.lock_version
+    assert_equal "new title1", t1.title
+
+    assert_raise(ActiveRecord::StaleObjectError) { t2.save! }
+    assert_equal 0, t2.lock_version
+    assert_equal "new title2", t2.title
+  end
+
+  def test_lock_without_default_should_update_with_lock_col
+    t1 = LockWithoutDefault.create(title: "title1", lock_version: 6)
+
+    assert_equal 6, t1.lock_version
+
+    t1.update(lock_version: 0)
+    t1.reload
 
-    t1.save
-    t1 = LockWithoutDefault.find(t1.id)
     assert_equal 0, t1.lock_version
   end
 
+  def test_lock_without_default_queries_count
+    t1 = LockWithoutDefault.create(title: "title1")
+
+    assert_equal "title1", t1.title
+    assert_equal 0, t1.lock_version
+
+    assert_queries(1) { t1.update(title: "title2") }
+
+    t1.reload
+    assert_equal "title2", t1.title
+    assert_equal 1, t1.lock_version
+
+    assert_queries(1) { t1.update(title: "title3", lock_version: 6) }
+
+    t1.reload
+    assert_equal "title3", t1.title
+    assert_equal 6, t1.lock_version
+
+    t2 = LockWithoutDefault.new(title: "title1")
+
+    assert_queries(1) { t2.save! }
+
+    t2.reload
+    assert_equal "title1", t2.title
+    assert_equal 0, t2.lock_version
+  end
+
   def test_lock_with_custom_column_without_default_sets_version_to_zero
     t1 = LockWithCustomColumnWithoutDefault.new
+
     assert_equal 0, t1.custom_lock_version
     assert_nil t1.custom_lock_version_before_type_cast
 
     t1.save!
     t1.reload
+
+    assert_equal 0, t1.custom_lock_version
+    assert_equal 0, t1.custom_lock_version_before_type_cast
+  end
+
+  def test_lock_with_custom_column_without_default_should_work_with_null_in_the_database
+    ActiveRecord::Base.connection.execute("INSERT INTO lock_without_defaults_cust(title) VALUES('title1')")
+
+    t1 = LockWithCustomColumnWithoutDefault.last
+    t2 = LockWithCustomColumnWithoutDefault.last
+
     assert_equal 0, t1.custom_lock_version
-    assert [0, "0"].include?(t1.custom_lock_version_before_type_cast)
+    assert_nil t1.custom_lock_version_before_type_cast
+    assert_equal 0, t2.custom_lock_version
+    assert_nil t2.custom_lock_version_before_type_cast
+
+    t1.title = "new title1"
+    t2.title = "new title2"
+
+    assert_nothing_raised { t1.save! }
+    assert_equal 1, t1.custom_lock_version
+    assert_equal "new title1", t1.title
+
+    assert_raise(ActiveRecord::StaleObjectError) { t2.save! }
+    assert_equal 0, t2.custom_lock_version
+    assert_equal "new title2", t2.title
+  end
+
+  def test_lock_with_custom_column_without_default_should_update_with_lock_col
+    t1 = LockWithCustomColumnWithoutDefault.create(title: "title1", custom_lock_version: 6)
+
+    assert_equal 6, t1.custom_lock_version
+
+    t1.update(custom_lock_version: 0)
+    t1.reload
+
+    assert_equal 0, t1.custom_lock_version
+  end
+
+  def test_lock_with_custom_column_without_default_queries_count
+    t1 = LockWithCustomColumnWithoutDefault.create(title: "title1")
+
+    assert_equal "title1", t1.title
+    assert_equal 0, t1.custom_lock_version
+
+    assert_queries(1) { t1.update(title: "title2") }
+
+    t1.reload
+    assert_equal "title2", t1.title
+    assert_equal 1, t1.custom_lock_version
+
+    assert_queries(1) { t1.update(title: "title3", custom_lock_version: 6) }
+
+    t1.reload
+    assert_equal "title3", t1.title
+    assert_equal 6, t1.custom_lock_version
+
+    t2 = LockWithCustomColumnWithoutDefault.new(title: "title1")
+
+    assert_queries(1) { t2.save! }
+
+    t2.reload
+    assert_equal "title1", t2.title
+    assert_equal 0, t2.custom_lock_version
   end
 
   def test_readonly_attributes
@@ -461,6 +580,7 @@ unless in_memory_db?
       end
 
       protected
+
       def duel(zzz = 5)
         t0, t1, t2, t3 = nil, nil, nil, nil
 
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index d2fb090118..983ac076a9 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -436,10 +436,12 @@ ActiveRecord::Schema.define do
   end
 
   create_table :lock_without_defaults, force: true do |t|
+    t.column :title, :string
     t.column :lock_version, :integer
   end
 
   create_table :lock_without_defaults_cust, force: true do |t|
+    t.column :title, :string
     t.column :custom_lock_version, :integer
   end