Merge pull request #35987 from kamipo/fix_dirty_tracking_after_rollback

Fix dirty tracking after rollback.

9 files changed, 92 insertions, 25 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 59669ca26e..485547f036 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Fix dirty tracking after rollback.
+
+    Fixes #15018, #30167, #33868.
+
+    *Ryuta Kamizono*
+
 *   Add `ActiveRecord::Relation#cache_version` to support recyclable cache keys via
     the versioned entries in `ActiveSupport::Cache`. This also means that
     `ActiveRecord::Relation#cache_key` will now return a stable key that does not
diff --git a/activerecord/lib/active_record/attribute_methods/before_type_cast.rb b/activerecord/lib/active_record/attribute_methods/before_type_cast.rb
index dc239ff9ea..affcf2a4db 100644
--- a/activerecord/lib/active_record/attribute_methods/before_type_cast.rb
+++ b/activerecord/lib/active_record/attribute_methods/before_type_cast.rb
@@ -46,6 +46,7 @@ module ActiveRecord
       #   task.read_attribute_before_type_cast('completed_on') # => "2012-10-21"
       #   task.read_attribute_before_type_cast(:completed_on)  # => "2012-10-21"
       def read_attribute_before_type_cast(attr_name)
+        sync_with_transaction_state
         @attributes[attr_name.to_s].value_before_type_cast
       end
 
@@ -60,6 +61,7 @@ module ActiveRecord
       #   task.attributes_before_type_cast
       #   # => {"id"=>nil, "title"=>nil, "is_done"=>true, "completed_on"=>"2012-10-21", "created_at"=>nil, "updated_at"=>nil}
       def attributes_before_type_cast
+        sync_with_transaction_state
         @attributes.values_before_type_cast
       end
 
@@ -71,6 +73,7 @@ module ActiveRecord
         end
 
         def attribute_came_from_user?(attribute_name)
+          sync_with_transaction_state
           @attributes[attribute_name].came_from_user?
         end
     end
diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb
index 68ac8475b0..942fe48635 100644
--- a/activerecord/lib/active_record/attribute_methods/dirty.rb
+++ b/activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -156,6 +156,16 @@ module ActiveRecord
       end
 
       private
+        def mutations_from_database
+          sync_with_transaction_state
+          super
+        end
+
+        def mutations_before_last_save
+          sync_with_transaction_state
+          super
+        end
+
         def write_attribute_without_type_cast(attr_name, value)
           name = attr_name.to_s
           if self.class.attribute_alias?(name)
diff --git a/activerecord/lib/active_record/attribute_methods/primary_key.rb b/activerecord/lib/active_record/attribute_methods/primary_key.rb
index 6af5346fa7..feaef72a30 100644
--- a/activerecord/lib/active_record/attribute_methods/primary_key.rb
+++ b/activerecord/lib/active_record/attribute_methods/primary_key.rb
@@ -16,39 +16,33 @@ module ActiveRecord
 
       # Returns the primary key column's value.
       def id
-        sync_with_transaction_state
         primary_key = self.class.primary_key
         _read_attribute(primary_key) if primary_key
       end
 
       # Sets the primary key column's value.
       def id=(value)
-        sync_with_transaction_state
         primary_key = self.class.primary_key
         _write_attribute(primary_key, value) if primary_key
       end
 
       # Queries the primary key column's value.
       def id?
-        sync_with_transaction_state
         query_attribute(self.class.primary_key)
       end
 
       # Returns the primary key column's value before type cast.
       def id_before_type_cast
-        sync_with_transaction_state
         read_attribute_before_type_cast(self.class.primary_key)
       end
 
       # Returns the primary key column's previous value.
       def id_was
-        sync_with_transaction_state
         attribute_was(self.class.primary_key)
       end
 
       # Returns the primary key column's value from the database.
       def id_in_database
-        sync_with_transaction_state
         attribute_in_database(self.class.primary_key)
       end
 
diff --git a/activerecord/lib/active_record/attribute_methods/read.rb b/activerecord/lib/active_record/attribute_methods/read.rb
index ffac5313ad..84b1ec2fea 100644
--- a/activerecord/lib/active_record/attribute_methods/read.rb
+++ b/activerecord/lib/active_record/attribute_methods/read.rb
@@ -9,14 +9,11 @@ module ActiveRecord
         private
 
           def define_method_attribute(name)
-            sync_with_transaction_state = "sync_with_transaction_state" if name == primary_key
-
             ActiveModel::AttributeMethods::AttrNames.define_attribute_accessor_method(
               generated_attribute_methods, name
             ) do |temp_method_name, attr_name_expr|
               generated_attribute_methods.module_eval <<-RUBY, __FILE__, __LINE__ + 1
                 def #{temp_method_name}
-                  #{sync_with_transaction_state}
                   name = #{attr_name_expr}
                   _read_attribute(name) { |n| missing_attribute(n, caller) }
                 end
@@ -36,13 +33,13 @@ module ActiveRecord
 
         primary_key = self.class.primary_key
         name = primary_key if name == "id" && primary_key
-        sync_with_transaction_state if name == primary_key
         _read_attribute(name, &block)
       end
 
       # This method exists to avoid the expensive primary_key check internally, without
       # breaking compatibility with the read_attribute API
       def _read_attribute(attr_name, &block) # :nodoc
+        sync_with_transaction_state
         @attributes.fetch_value(attr_name.to_s, &block)
       end
 
diff --git a/activerecord/lib/active_record/attribute_methods/write.rb b/activerecord/lib/active_record/attribute_methods/write.rb
index d5ba2f42cb..d1cfe43bb2 100644
--- a/activerecord/lib/active_record/attribute_methods/write.rb
+++ b/activerecord/lib/active_record/attribute_methods/write.rb
@@ -13,15 +13,12 @@ module ActiveRecord
         private
 
           def define_method_attribute=(name)
-            sync_with_transaction_state = "sync_with_transaction_state" if name == primary_key
-
             ActiveModel::AttributeMethods::AttrNames.define_attribute_accessor_method(
               generated_attribute_methods, name, writer: true,
             ) do |temp_method_name, attr_name_expr|
               generated_attribute_methods.module_eval <<-RUBY, __FILE__, __LINE__ + 1
                 def #{temp_method_name}(value)
                   name = #{attr_name_expr}
-                  #{sync_with_transaction_state}
                   _write_attribute(name, value)
                 end
               RUBY
@@ -40,21 +37,21 @@ module ActiveRecord
 
         primary_key = self.class.primary_key
         name = primary_key if name == "id" && primary_key
-        sync_with_transaction_state if name == primary_key
         _write_attribute(name, value)
       end
 
       # This method exists to avoid the expensive primary_key check internally, without
       # breaking compatibility with the write_attribute API
       def _write_attribute(attr_name, value) # :nodoc:
+        sync_with_transaction_state
         @attributes.write_from_user(attr_name.to_s, value)
         value
       end
 
       private
         def write_attribute_without_type_cast(attr_name, value)
-          name = attr_name.to_s
-          @attributes.write_cast_value(name, value)
+          sync_with_transaction_state
+          @attributes.write_cast_value(attr_name.to_s, value)
           value
         end
 
diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index 6fed3e5c19..04b21b4d00 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -583,12 +583,6 @@ module ActiveRecord
       def initialize_internals_callback
       end
 
-      def thaw
-        if @attributes.frozen?
-          @attributes = @attributes.dup
-        end
-      end
-
       def custom_inspect_method_defined?
         self.class.instance_method(:inspect).owner != ActiveRecord::Base.instance_method(:inspect).owner
       end
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 03a373f0af..ea288456b9 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -390,6 +390,7 @@ module ActiveRecord
           id: id,
           new_record: @new_record,
           destroyed: @destroyed,
+          attributes: @attributes,
           frozen?: frozen?,
         )
         @_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) + 1
@@ -422,12 +423,18 @@ module ActiveRecord
           transaction_level = (@_start_transaction_state[:level] || 0) - 1
           if transaction_level < 1 || force_restore_state
             restore_state = @_start_transaction_state
-            thaw
             @new_record = restore_state[:new_record]
             @destroyed  = restore_state[:destroyed]
+            @attributes = restore_state[:attributes].map do |attr|
+              value = @attributes.fetch_value(attr.name)
+              attr = attr.with_value_from_user(value) if attr.value != value
+              attr
+            end
+            @mutations_from_database = nil
+            @mutations_before_last_save = nil
             pk = self.class.primary_key
-            if pk && _read_attribute(pk) != restore_state[:id]
-              _write_attribute(pk, restore_state[:id])
+            if pk && @attributes.fetch_value(pk) != restore_state[:id]
+              @attributes.write_from_user(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 7bad3de343..6795996cca 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -18,6 +18,65 @@ class TransactionTest < ActiveRecord::TestCase
     @first, @second = Topic.find(1, 2).sort_by(&:id)
   end
 
+  def test_rollback_dirty_changes
+    topic = topics(:fifth)
+
+    ActiveRecord::Base.transaction do
+      topic.update(title: "Ruby on Rails")
+      raise ActiveRecord::Rollback
+    end
+
+    title_change = ["The Fifth Topic of the day", "Ruby on Rails"]
+    assert_equal title_change, topic.changes["title"]
+  end
+
+  def test_rollback_dirty_changes_multiple_saves
+    topic = topics(:fifth)
+
+    ActiveRecord::Base.transaction do
+      topic.update(title: "Ruby on Rails")
+      topic.update(title: "Another Title")
+      raise ActiveRecord::Rollback
+    end
+
+    title_change = ["The Fifth Topic of the day", "Another Title"]
+    assert_equal title_change, topic.changes["title"]
+  end
+
+  def test_rollback_dirty_changes_then_retry_save
+    topic = topics(:fifth)
+
+    ActiveRecord::Base.transaction do
+      topic.update(title: "Ruby on Rails")
+      raise ActiveRecord::Rollback
+    end
+
+    title_change = ["The Fifth Topic of the day", "Ruby on Rails"]
+    assert_equal title_change, topic.changes["title"]
+
+    assert topic.save
+
+    assert_equal title_change, topic.saved_changes["title"]
+    assert_equal topic.title, topic.reload.title
+  end
+
+  def test_rollback_dirty_changes_then_retry_save_on_new_record
+    topic = Topic.new(title: "Ruby on Rails")
+
+    ActiveRecord::Base.transaction do
+      topic.save
+      raise ActiveRecord::Rollback
+    end
+
+    title_change = [nil, "Ruby on Rails"]
+    assert_equal title_change, topic.changes["title"]
+
+    assert topic.save
+
+    assert_equal title_change, topic.saved_changes["title"]
+    assert_equal topic.title, topic.reload.title
+  end
+
   def test_persisted_in_a_model_with_custom_primary_key_after_failed_save
     movie = Movie.create
     assert_not_predicate movie, :persisted?