Sync transaction state when accessing primary key

If a record is modified inside a transaction, it must check the outcome
of that transaction before accessing any state which would no longer be
valid if it was rolled back.

For example, consider a new record that was saved inside a transaction
which was later rolled back: it should be restored to its previous state
so that saving it again inserts a new row into the database instead of
trying to update a row that no longer exists.

The `id` and `id=` methods defined on the PrimaryKey module implement
this correctly, but when a model uses a custom primary key, the reader
and writer methods for that attribute must check the transaction state
too. The `read_attribute` and `write_attribute` methods also need to
check the transaction state when accessing the 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 fdc4bf6621..f7f3b7ec96 100644
--- a/activerecord/lib/active_record/attribute_methods/read.rb
+++ b/activerecord/lib/active_record/attribute_methods/read.rb
@@ -29,9 +29,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
@@ -55,6 +57,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 54b673c72e..9c43567a11 100644
--- a/activerecord/lib/active_record/attribute_methods/write.rb
+++ b/activerecord/lib/active_record/attribute_methods/write.rb
@@ -13,10 +13,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}=
@@ -36,6 +38,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 463bb1f314..4d883e66df 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -430,8 +430,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 76a997ba8b..a5348e2825 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -684,7 +684,7 @@ class TransactionTest < ActiveRecord::TestCase
       raise ActiveRecord::Rollback
     end
 
-    assert_nil movie.id
+    assert_nil movie.movieid
   end
 
   def test_assign_id_after_rollback
@@ -707,8 +707,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