From 368cca51d25fa15c0ddc8743b5904c4d5ce93e74 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sat, 7 Jun 2014 08:56:27 -0600 Subject: Do not type cast twice on attribute assignment The definition of `write_attribute` in dirty checking ultimately leads to the columns calling `type_cast` on the value to perform the comparison. However, this is a potentially expensive computation that we cache when it occurs in `read_attribute`. The only case that we need the non-type-cast form is for numeric, so we pass that through as well (something I'm looking to remove in the future). This also reduces the number of places that manually access various stages in an attribute's type casting lifecycle, which will aid in one of the larger refactorings that I'm working on. --- .../lib/active_record/attribute_methods/dirty.rb | 31 ++++++++++++++-------- activerecord/lib/active_record/enum.rb | 7 ++--- activerecord/lib/active_record/type/numeric.rb | 6 ++--- activerecord/lib/active_record/type/serialized.rb | 4 --- activerecord/lib/active_record/type/value.rb | 4 +-- 5 files changed, 27 insertions(+), 25 deletions(-) (limited to 'activerecord/lib') 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 -- cgit v1.2.3