Fixed: Optimistic locking does not work well with null in the database

4 files changed, 69 insertions, 15 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 15b49e0a0b..db9c394a65 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   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..fa9e789b24 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
@@ -80,7 +81,8 @@ module ActiveRecord
           return 0 if attribute_names.empty?
 
           lock_col = self.class.locking_column
-          previous_lock_value = send(lock_col).to_i
+          previous_lock_value = read_attribute_before_type_cast(lock_col)
+
           increment_lock
 
           attribute_names += [lock_col]
@@ -91,7 +93,7 @@ module ActiveRecord
 
             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 +106,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/locking_test.rb b/activerecord/test/cases/locking_test.rb
index 13b6f6daaf..8b6a969c9e 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,73 @@ 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
 
-    t1.save
-    t1 = LockWithoutDefault.find(t1.id)
     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_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 [0, "0"].include?(t1.custom_lock_version_before_type_cast)
+    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_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_readonly_attributes
@@ -461,6 +504,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