Merge pull request #15562 from sgrif/sg-double-type-cast-dirty

Do not type cast twice on attribute assignment

5 files changed, 27 insertions, 25 deletions

diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb
index 4e32b78e34..be438aba95 100644
--- a/activerecord/lib/active_record/attribute_methods/dirty.rb
+++ b/activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -55,7 +55,7 @@ module ActiveRecord
         # optimistic locking) won't get written unless they get marked as changed
         self.class.columns.each do |c|
           attr, orig_value = c.name, c.default
-          changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value, @raw_attributes[attr])
+          changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value)
         end
       end
 
@@ -63,19 +63,26 @@ module ActiveRecord
       def write_attribute(attr, value)
         attr = attr.to_s
 
-        save_changed_attribute(attr, value)
+        old_value = old_attribute_value(attr)
 
-        super(attr, value)
+        result = super(attr, value)
+        save_changed_attribute(attr, old_value)
+        result
       end
 
-      def save_changed_attribute(attr, value)
-        # The attribute already has an unsaved change.
+      def save_changed_attribute(attr, old_value)
         if attribute_changed?(attr)
-          old = changed_attributes[attr]
-          changed_attributes.delete(attr) unless _field_changed?(attr, old, value)
+          changed_attributes.delete(attr) unless _field_changed?(attr, old_value)
         else
-          old = clone_attribute_value(:read_attribute, attr)
-          changed_attributes[attr] = old if _field_changed?(attr, old, value)
+          changed_attributes[attr] = old_value if _field_changed?(attr, old_value)
+        end
+      end
+
+      def old_attribute_value(attr)
+        if attribute_changed?(attr)
+          changed_attributes[attr]
+        else
+          clone_attribute_value(:read_attribute, attr)
         end
       end
 
@@ -93,8 +100,10 @@ module ActiveRecord
         changed
       end
 
-      def _field_changed?(attr, old, value)
-        column_for_attribute(attr).changed?(old, value)
+      def _field_changed?(attr, old_value)
+        new_value = read_attribute(attr)
+        raw_value = read_attribute_before_type_cast(attr)
+        column_for_attribute(attr).changed?(old_value, new_value, raw_value)
       end
     end
   end
diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb
index c7ec093824..38c6dcf88d 100644
--- a/activerecord/lib/active_record/enum.rb
+++ b/activerecord/lib/active_record/enum.rb
@@ -140,17 +140,14 @@ module ActiveRecord
         @_enum_methods_module ||= begin
           mod = Module.new do
             private
-              def save_changed_attribute(attr_name, value)
+              def save_changed_attribute(attr_name, old)
                 if (mapping = self.class.defined_enums[attr_name.to_s])
+                  value = read_attribute(attr_name)
                   if attribute_changed?(attr_name)
-                    old = changed_attributes[attr_name]
-
                     if mapping[old] == value
                       changed_attributes.delete(attr_name)
                     end
                   else
-                    old = clone_attribute_value(:read_attribute, attr_name)
-
                     if old != value
                       changed_attributes[attr_name] = mapping.key old
                     end
diff --git a/activerecord/lib/active_record/type/numeric.rb b/activerecord/lib/active_record/type/numeric.rb
index d5cb13233c..137c9e4c99 100644
--- a/activerecord/lib/active_record/type/numeric.rb
+++ b/activerecord/lib/active_record/type/numeric.rb
@@ -15,11 +15,11 @@ module ActiveRecord
         super(value)
       end
 
-      def changed?(old_value, new_value) # :nodoc:
+      def changed?(old_value, _new_value, new_value_before_type_cast) # :nodoc:
         # 0 => 'wibble' should mark as changed so numericality validations run
-        if nil_or_zero?(old_value) && non_numeric_string?(new_value)
+        if nil_or_zero?(old_value) && non_numeric_string?(new_value_before_type_cast)
           # nil => '' should not mark as changed
-          old_value != new_value.presence
+          old_value != new_value_before_type_cast.presence
         else
           super
         end
diff --git a/activerecord/lib/active_record/type/serialized.rb b/activerecord/lib/active_record/type/serialized.rb
index 94d9d9a549..0866383de9 100644
--- a/activerecord/lib/active_record/type/serialized.rb
+++ b/activerecord/lib/active_record/type/serialized.rb
@@ -36,10 +36,6 @@ module ActiveRecord
 
       private
 
-      def changed?(old_value, new_value) # :nodoc:
-        old_value != new_value
-      end
-
       def is_default_value?(value)
         value == coder.load(nil)
       end
diff --git a/activerecord/lib/active_record/type/value.rb b/activerecord/lib/active_record/type/value.rb
index 4bc3086db3..1c41b28646 100644
--- a/activerecord/lib/active_record/type/value.rb
+++ b/activerecord/lib/active_record/type/value.rb
@@ -59,8 +59,8 @@ module ActiveRecord
       # or from assignment, so it could be anything. Types
       # which cannot typecast arbitrary values should override
       # this method.
-      def changed?(old_value, new_value) # :nodoc:
-        old_value != type_cast(new_value)
+      def changed?(old_value, new_value, _new_value_before_type_cast) # :nodoc:
+        old_value != new_value
       end
 
       private