Merge pull request #29724 from eugeneius/sync_primary_key

Sync transaction state when accessing primary key

4 files changed, 57 insertions, 5 deletions

diff --git a/activerecord/lib/active_record/attribute_methods/read.rb b/activerecord/lib/active_record/attribute_methods/read.rb
index 6adaeb0121..615b2fa701 100644
--- a/activerecord/lib/active_record/attribute_methods/read.rb
+++ b/activerecord/lib/active_record/attribute_methods/read.rb
@@ -31,9 +31,11 @@ module ActiveRecord
             temp_method = "__temp__#{safe_name}"
 
             ActiveRecord::AttributeMethods::AttrNames.set_name_cache safe_name, name
+            sync_with_transaction_state = "sync_with_transaction_state" if name == primary_key
 
             generated_attribute_methods.module_eval <<-STR, __FILE__, __LINE__ + 1
               def #{temp_method}
+                #{sync_with_transaction_state}
                 name = ::ActiveRecord::AttributeMethods::AttrNames::ATTR_#{safe_name}
                 _read_attribute(name) { |n| missing_attribute(n, caller) }
               end
@@ -57,6 +59,7 @@ module ActiveRecord
         end
 
         name = self.class.primary_key if name == "id".freeze && self.class.primary_key
+        sync_with_transaction_state if name == self.class.primary_key
         _read_attribute(name, &block)
       end
 
diff --git a/activerecord/lib/active_record/attribute_methods/write.rb b/activerecord/lib/active_record/attribute_methods/write.rb
index fa01f832ac..62c5ce059b 100644
--- a/activerecord/lib/active_record/attribute_methods/write.rb
+++ b/activerecord/lib/active_record/attribute_methods/write.rb
@@ -15,10 +15,12 @@ module ActiveRecord
           def define_method_attribute=(name)
             safe_name = name.unpack("h*".freeze).first
             ActiveRecord::AttributeMethods::AttrNames.set_name_cache safe_name, name
+            sync_with_transaction_state = "sync_with_transaction_state" if name == primary_key
 
             generated_attribute_methods.module_eval <<-STR, __FILE__, __LINE__ + 1
               def __temp__#{safe_name}=(value)
                 name = ::ActiveRecord::AttributeMethods::AttrNames::ATTR_#{safe_name}
+                #{sync_with_transaction_state}
                 _write_attribute(name, value)
               end
               alias_method #{(name + '=').inspect}, :__temp__#{safe_name}=
@@ -38,6 +40,7 @@ module ActiveRecord
         end
 
         name = self.class.primary_key if name == "id".freeze && self.class.primary_key
+        sync_with_transaction_state if name == self.class.primary_key
         _write_attribute(name, value)
       end
 
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 3e8a0789df..f91f0cdf12 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -432,8 +432,8 @@ module ActiveRecord
             @new_record = restore_state[:new_record]
             @destroyed  = restore_state[:destroyed]
             pk = self.class.primary_key
-            if pk && read_attribute(pk) != restore_state[:id]
-              write_attribute(pk, restore_state[:id])
+            if pk && _read_attribute(pk) != restore_state[:id]
+              _write_attribute(pk, restore_state[:id])
             end
             freeze if restore_state[:frozen?]
           end
diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb
index b771a1d12c..7fd125ab74 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -686,7 +686,7 @@ class TransactionTest < ActiveRecord::TestCase
       raise ActiveRecord::Rollback
     end
 
-    assert_nil movie.id
+    assert_nil movie.movieid
   end
 
   def test_assign_id_after_rollback
@@ -709,8 +709,54 @@ class TransactionTest < ActiveRecord::TestCase
       raise ActiveRecord::Rollback
     end
 
-    movie.id = nil
-    assert_nil movie.id
+    movie.movieid = nil
+    assert_nil movie.movieid
+  end
+
+  def test_read_attribute_after_rollback
+    topic = Topic.new
+
+    Topic.transaction do
+      topic.save!
+      raise ActiveRecord::Rollback
+    end
+
+    assert_nil topic.read_attribute(:id)
+  end
+
+  def test_read_attribute_with_custom_primary_key_after_rollback
+    movie = Movie.new(name: "foo")
+
+    Movie.transaction do
+      movie.save!
+      raise ActiveRecord::Rollback
+    end
+
+    assert_nil movie.read_attribute(:movieid)
+  end
+
+  def test_write_attribute_after_rollback
+    topic = Topic.create!
+
+    Topic.transaction do
+      topic.save!
+      raise ActiveRecord::Rollback
+    end
+
+    topic.write_attribute(:id, nil)
+    assert_nil topic.id
+  end
+
+  def test_write_attribute_with_custom_primary_key_after_rollback
+    movie = Movie.create!(name: "foo")
+
+    Movie.transaction do
+      movie.save!
+      raise ActiveRecord::Rollback
+    end
+
+    movie.write_attribute(:movieid, nil)
+    assert_nil movie.movieid
   end
 
   def test_rollback_of_frozen_records