Implement `_was` and `changes` for in-place mutations of AR attributes

7 files changed, 46 insertions, 18 deletions

diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 79c3d2b0f5..cf59699228 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -103,7 +103,7 @@ module ActiveRecord
           if has_cached_counter?(reflection)
             counter = cached_counter_attribute_name(reflection)
             owner[counter] += difference
-            owner.changed_attributes.delete(counter) # eww
+            owner.clear_attribute_changes([counter]) # eww
           end
         end
 
diff --git a/activerecord/lib/active_record/attribute.rb b/activerecord/lib/active_record/attribute.rb
index 15cbbcff68..8cc1904575 100644
--- a/activerecord/lib/active_record/attribute.rb
+++ b/activerecord/lib/active_record/attribute.rb
@@ -30,10 +30,14 @@ module ActiveRecord
 
     def value
       # `defined?` is cheaper than `||=` when we get back falsy values
-      @value = type_cast(value_before_type_cast) unless defined?(@value)
+      @value = original_value unless defined?(@value)
       @value
     end
 
+    def original_value
+      type_cast(value_before_type_cast)
+    end
+
     def value_for_database
       type.type_cast_for_database(value)
     end
@@ -54,7 +58,7 @@ module ActiveRecord
       self.class.from_database(name, value, type)
     end
 
-    def type_cast
+    def type_cast(*)
       raise NotImplementedError
     end
 
diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb
index b58295a106..d3f4e51c33 100644
--- a/activerecord/lib/active_record/attribute_methods/dirty.rb
+++ b/activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -51,14 +51,6 @@ module ActiveRecord
         super | changed_in_place
       end
 
-      def attribute_changed?(attr_name, options = {})
-        result = super
-        # We can't change "from" something in place. Only setters can define
-        # "from" and "to"
-        result ||= changed_in_place?(attr_name) unless options.key?(:from)
-        result
-      end
-
       def changes_applied
         super
         store_original_raw_attributes
@@ -69,12 +61,16 @@ module ActiveRecord
         original_raw_attributes.clear
       end
 
+      def changed_attributes
+        super.reverse_merge(attributes_changed_in_place).freeze
+      end
+
       private
 
       def calculate_changes_from_defaults
         @changed_attributes = nil
         self.class.column_defaults.each do |attr, orig_value|
-          changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value)
+          set_attribute_was(attr, orig_value) if _field_changed?(attr, orig_value)
         end
       end
 
@@ -100,9 +96,9 @@ module ActiveRecord
 
       def save_changed_attribute(attr, old_value)
         if attribute_changed?(attr)
-          changed_attributes.delete(attr) unless _field_changed?(attr, old_value)
+          clear_attribute_changes(attr) unless _field_changed?(attr, old_value)
         else
-          changed_attributes[attr] = old_value if _field_changed?(attr, old_value)
+          set_attribute_was(attr, old_value) if _field_changed?(attr, old_value)
         end
       end
 
@@ -132,6 +128,13 @@ module ActiveRecord
         @attributes[attr].changed_from?(old_value)
       end
 
+      def attributes_changed_in_place
+        changed_in_place.each_with_object({}) do |attr_name, h|
+          orig = @attributes[attr_name].original_value
+          h[attr_name] = orig
+        end
+      end
+
       def changed_in_place
         self.class.attribute_names.select do |attr_name|
           changed_in_place?(attr_name)
diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb
index f0ee433d0b..5958373e88 100644
--- a/activerecord/lib/active_record/enum.rb
+++ b/activerecord/lib/active_record/enum.rb
@@ -145,11 +145,11 @@ module ActiveRecord
                   value = read_attribute(attr_name)
                   if attribute_changed?(attr_name)
                     if mapping[old] == value
-                      changed_attributes.delete(attr_name)
+                      clear_attribute_changes([attr_name])
                     end
                   else
                     if old != value
-                      changed_attributes[attr_name] = mapping.key old
+                      set_attribute_was(attr_name, mapping.key(old))
                     end
                   end
                 else
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index 51b1931ed5..755ff2b2f1 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -466,7 +466,7 @@ module ActiveRecord
 
         changes[self.class.locking_column] = increment_lock if locking_enabled?
 
-        changed_attributes.except!(*changes.keys)
+        clear_attribute_changes(changes.keys)
         primary_key = self.class.primary_key
         self.class.unscoped.where(primary_key => self[primary_key]).update_all(changes) == 1
       else
diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb
index 417cd61f0c..936a18d99a 100644
--- a/activerecord/lib/active_record/timestamp.rb
+++ b/activerecord/lib/active_record/timestamp.rb
@@ -114,7 +114,7 @@ module ActiveRecord
     def clear_timestamp_attributes
       all_timestamp_attributes_in_model.each do |attribute_name|
         self[attribute_name] = nil
-        changed_attributes.delete(attribute_name)
+        clear_attribute_changes([attribute_name])
       end
     end
   end
diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb
index 69a7f25213..0c77eedb52 100644
--- a/activerecord/test/cases/dirty_test.rb
+++ b/activerecord/test/cases/dirty_test.rb
@@ -661,6 +661,27 @@ class DirtyTest < ActiveRecord::TestCase
     assert_not model.foo_changed?
   end
 
+  test "in place mutation detection" do
+    pirate = Pirate.create!(catchphrase: "arrrr")
+    pirate.catchphrase << " matey!"
+
+    assert pirate.catchphrase_changed?
+    expected_changes = {
+      "catchphrase" => ["arrrr", "arrrr matey!"]
+    }
+    assert_equal(expected_changes, pirate.changes)
+    assert_equal("arrrr", pirate.catchphrase_was)
+    assert pirate.catchphrase_changed?(from: "arrrr")
+    assert_not pirate.catchphrase_changed?(from: "anything else")
+    assert pirate.changed_attributes.include?(:catchphrase)
+
+    pirate.save!
+    pirate.reload
+
+    assert_equal "arrrr matey!", pirate.catchphrase
+    assert_not pirate.changed?
+  end
+
   private
     def with_partial_writes(klass, on = true)
       old = klass.partial_writes?