From 4db95d84322a384fb7c6e3d148aa09e29bc0961a Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Tue, 12 Jan 2021 09:13:46 +0900
Subject: [PATCH] Restore the ability that update/destroy optimistic locking
 object without default

The ability has lost due to reverted #39321 in #41049.

We should allow updating with dirty locking value to work the documented
usage, but if casted value has no difference (i.e. regarded as no dirty),
identify the object by the original (uninitialized default) value.
---
 activemodel/lib/active_model/attribute.rb     | 20 +++++------
 .../lib/active_record/locking/optimistic.rb   | 18 +++++++---
 activerecord/test/cases/locking_test.rb       | 33 +++++++++++++++++++
 3 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/activemodel/lib/active_model/attribute.rb b/activemodel/lib/active_model/attribute.rb
index 7facc6cc75656..5bb5e747efdb2 100644
--- a/activemodel/lib/active_model/attribute.rb
+++ b/activemodel/lib/active_model/attribute.rb
@@ -133,14 +133,13 @@ def encode_with(coder)
       coder["value"] = value if defined?(@value)
     end
 
-    protected
-      def original_value_for_database
-        if assigned?
-          original_attribute.original_value_for_database
-        else
-          _original_value_for_database
-        end
+    def original_value_for_database
+      if assigned?
+        original_attribute.original_value_for_database
+      else
+        _original_value_for_database
       end
+    end
 
     private
       attr_reader :original_attribute
@@ -165,9 +164,10 @@ def type_cast(value)
           type.deserialize(value)
         end
 
-        def _original_value_for_database
-          value_before_type_cast
-        end
+        private
+          def _original_value_for_database
+            value_before_type_cast
+          end
       end
 
       class FromUser < Attribute # :nodoc:
diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb
index 015562d55c8df..2611b8d72347e 100644
--- a/activerecord/lib/active_record/locking/optimistic.rb
+++ b/activerecord/lib/active_record/locking/optimistic.rb
@@ -89,7 +89,9 @@ def _update_row(attribute_names, attempted_action = "update")
 
           begin
             locking_column = self.class.locking_column
-            previous_lock_value = attribute_before_type_cast(locking_column)
+            lock_attribute_was = @attributes[locking_column]
+            lock_value_for_database = _lock_value_for_database(locking_column)
+
             attribute_names = attribute_names.dup if attribute_names.frozen?
             attribute_names << locking_column
 
@@ -98,7 +100,7 @@ def _update_row(attribute_names, attempted_action = "update")
             affected_rows = self.class._update_record(
               attributes_with_values(attribute_names),
               @primary_key => id_in_database,
-              locking_column => previous_lock_value
+              locking_column => lock_value_for_database
             )
 
             if affected_rows != 1
@@ -109,7 +111,7 @@ def _update_row(attribute_names, attempted_action = "update")
 
           # If something went wrong, revert the locking_column value.
           rescue Exception
-            self[locking_column] = previous_lock_value.to_i
+            @attributes[locking_column] = lock_attribute_was
             raise
           end
         end
@@ -121,7 +123,7 @@ def destroy_row
 
           affected_rows = self.class._delete_record(
             @primary_key => id_in_database,
-            locking_column => attribute_before_type_cast(locking_column)
+            locking_column => _lock_value_for_database(locking_column)
           )
 
           if affected_rows != 1
@@ -131,6 +133,14 @@ def destroy_row
           affected_rows
         end
 
+        def _lock_value_for_database(locking_column)
+          if will_save_change_to_attribute?(locking_column)
+            @attributes[locking_column].value_for_database
+          else
+            @attributes[locking_column].original_value_for_database
+          end
+        end
+
         module ClassMethods
           DEFAULT_LOCKING_COLUMN = "lock_version"
 
diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb
index 164dfb5a740f1..5de4eb3b952cf 100644
--- a/activerecord/test/cases/locking_test.rb
+++ b/activerecord/test/cases/locking_test.rb
@@ -340,6 +340,39 @@ def test_lock_without_default_should_work_with_null_in_the_database
     assert_equal "new title2", t2.title
   end
 
+  def test_update_with_lock_version_without_default_should_work_on_dirty_value_before_type_cast
+    ActiveRecord::Base.connection.execute("INSERT INTO lock_without_defaults(title) VALUES('title1')")
+    t1 = LockWithoutDefault.last
+
+    assert_equal 0, t1.lock_version
+    assert_nil t1.lock_version_before_type_cast
+
+    t1.lock_version = t1.lock_version
+
+    assert_equal 0, t1.lock_version
+    assert_equal 0, t1.lock_version_before_type_cast
+
+    assert_nothing_raised { t1.update!(title: "new title1") }
+    assert_equal 1, t1.lock_version
+    assert_equal "new title1", t1.title
+  end
+
+  def test_destroy_with_lock_version_without_default_should_work_on_dirty_value_before_type_cast
+    ActiveRecord::Base.connection.execute("INSERT INTO lock_without_defaults(title) VALUES('title1')")
+    t1 = LockWithoutDefault.last
+
+    assert_equal 0, t1.lock_version
+    assert_nil t1.lock_version_before_type_cast
+
+    t1.lock_version = t1.lock_version
+
+    assert_equal 0, t1.lock_version
+    assert_equal 0, t1.lock_version_before_type_cast
+
+    assert_nothing_raised { t1.destroy! }
+    assert_predicate t1, :destroyed?
+  end
+
   def test_lock_without_default_queries_count
     t1 = LockWithoutDefault.create(title: "title1")