From e117d9266e577675afe1462e37601c029e21091b Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Sun, 4 Mar 2018 15:32:46 +0900
Subject: `id_in_database` should be respected as primary key value for
 persisted records

Currently primary key value can not be updated if a record has a locking
column because of `_update_record` in `Locking::Optimistic` doesn't
respect `id_in_database` as primary key value unlike in `Persistence`.

And also, if a record has dirty primary key value, it may destroy any
other record by the lock version of dirty record itself.

When updating/destroying persisted records, it should identify
themselves by `id_in_database`, not by dirty primary key value.
---
 .../lib/active_record/locking/optimistic.rb        |  2 +-
 activerecord/lib/active_record/persistence.rb      |  2 +-
 activerecord/test/cases/locking_test.rb            | 39 ++++++++++++++++++++++
 3 files changed, 41 insertions(+), 2 deletions(-)

(limited to 'activerecord')

diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb
index bf17f27e68..052b5d23aa 100644
--- a/activerecord/lib/active_record/locking/optimistic.rb
+++ b/activerecord/lib/active_record/locking/optimistic.rb
@@ -94,7 +94,7 @@ module ActiveRecord
             relation = self.class.unscoped
 
             affected_rows = relation.where(
-              self.class.primary_key => id,
+              self.class.primary_key => id_in_database,
               lock_col => previous_lock_value
             ).update_all(
               attributes_for_update(attribute_names).map do |name|
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index c9f929680b..8f9cb85a90 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -691,7 +691,7 @@ module ActiveRecord
     end
 
     def _relation_for_itself
-      self.class.unscoped.where(self.class.primary_key => id)
+      self.class.unscoped.where(self.class.primary_key => id_in_database)
     end
 
     def create_or_update(*args, &block)
diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb
index 02be0aa9e3..9d04750712 100644
--- a/activerecord/test/cases/locking_test.rb
+++ b/activerecord/test/cases/locking_test.rb
@@ -194,6 +194,45 @@ class OptimisticLockingTest < ActiveRecord::TestCase
     end
   end
 
+  def test_update_with_dirty_primary_key
+    assert_raises(ActiveRecord::RecordNotUnique) do
+      person = Person.find(1)
+      person.id = 2
+      person.save!
+    end
+
+    person = Person.find(1)
+    person.id = 42
+    person.save!
+
+    assert Person.find(42)
+    assert_raises(ActiveRecord::RecordNotFound) do
+      Person.find(1)
+    end
+  end
+
+  def test_delete_with_dirty_primary_key
+    person = Person.find(1)
+    person.id = 2
+    person.delete
+
+    assert Person.find(2)
+    assert_raises(ActiveRecord::RecordNotFound) do
+      Person.find(1)
+    end
+  end
+
+  def test_destroy_with_dirty_primary_key
+    person = Person.find(1)
+    person.id = 2
+    person.destroy
+
+    assert Person.find(2)
+    assert_raises(ActiveRecord::RecordNotFound) do
+      Person.find(1)
+    end
+  end
+
   def test_explicit_update_lock_column_raise_error
     person = Person.find(1)
 
-- 
cgit v1.2.3