Fix ActiveRecord::Persistence#touch with locking

`ActiveRecord::Persistence#touch` does not work well when optimistic locking enabled and `locking_column`, without default value, is null in the database.
ragesoss · Jun 20, 2017 · 4d1264c · 4d1264c
1 parent 890de77
commit 4d1264c
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 3 deletions.
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   `ActiveRecord::Persistence#touch` does not work well when optimistic locking enabled and
+    `locking_column`, without default value, is null in the database.
+
+    *bogdanvlviv*
+
 *   Fix destroying existing object does not work well when optimistic locking enabled and
     `locking column` is null in the database.
 

diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
@@ -526,7 +526,7 @@ def touch(*names, time: nil)
 
         if locking_enabled?
           locking_column = self.class.locking_column
-          scope = scope.where(locking_column => _read_attribute(locking_column))
+          scope = scope.where(locking_column => read_attribute_before_type_cast(locking_column))
           changes[locking_column] = increment_lock
         end
 

diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb
@@ -244,10 +244,33 @@ def test_lock_without_default_sets_version_to_zero
     assert_equal 0, t1.lock_version_before_type_cast
   end
 
+  def test_touch_existing_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
+
+    assert_equal 0, t1.lock_version
+    assert_nil t1.lock_version_before_type_cast
+
+    t1.touch
+
+    assert_equal 1, t1.lock_version
+  end
+
+  def test_touch_stale_object_with_lock_without_default
+    t1 = LockWithoutDefault.create!(title: "title1")
+    stale_object = LockWithoutDefault.find(t1.id)
+
+    t1.update!(title: "title2")
+
+    assert_raises(ActiveRecord::StaleObjectError) do
+      stale_object.touch
+    end
+  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
+    t2 = LockWithoutDefault.find(t1.id)
 
     assert_equal 0, t1.lock_version
     assert_nil t1.lock_version_before_type_cast
@@ -304,7 +327,7 @@ def test_lock_with_custom_column_without_default_should_work_with_null_in_the_da
     ActiveRecord::Base.connection.execute("INSERT INTO lock_without_defaults_cust(title) VALUES('title1')")
 
     t1 = LockWithCustomColumnWithoutDefault.last
-    t2 = LockWithCustomColumnWithoutDefault.last
+    t2 = LockWithCustomColumnWithoutDefault.find(t1.id)
 
     assert_equal 0, t1.custom_lock_version
     assert_nil t1.custom_lock_version_before_type_cast

diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
@@ -453,11 +453,13 @@
   create_table :lock_without_defaults, force: true do |t|
     t.column :title, :string
     t.column :lock_version, :integer
+    t.timestamps null: true
   end
 
   create_table :lock_without_defaults_cust, force: true do |t|
     t.column :title, :string
     t.column :custom_lock_version, :integer
+    t.timestamps null: true
   end
 
   create_table :magazines, force: true do |t|