From 877ea784e4cd0d539bdfbd15839ae3d28169b156 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sat, 12 Jul 2014 18:30:49 -0600 Subject: [PATCH] Implement `_was` and `changes` for in-place mutations of AR attributes --- activemodel/lib/active_model/dirty.rb | 19 +++++++++++--- .../associations/has_many_association.rb | 2 +- activerecord/lib/active_record/attribute.rb | 8 ++++-- .../active_record/attribute_methods/dirty.rb | 25 +++++++++++-------- activerecord/lib/active_record/enum.rb | 4 +-- activerecord/lib/active_record/persistence.rb | 2 +- activerecord/lib/active_record/timestamp.rb | 2 +- activerecord/test/cases/dirty_test.rb | 21 ++++++++++++++++ 8 files changed, 62 insertions(+), 21 deletions(-) diff --git a/activemodel/lib/active_model/dirty.rb b/activemodel/lib/active_model/dirty.rb index d11243c4c0..ed00d8cf12 100644 --- a/activemodel/lib/active_model/dirty.rb +++ b/activemodel/lib/active_model/dirty.rb @@ -114,7 +114,7 @@ module ActiveModel include ActiveModel::AttributeMethods included do - attribute_method_suffix '_changed?', '_change', '_will_change!', '_was' + attribute_method_suffix '_changed?', '_change', '_will_change!', '_was', '_was=' attribute_method_affix prefix: 'reset_', suffix: '!' attribute_method_affix prefix: 'restore_', suffix: '!' end @@ -180,13 +180,26 @@ module ActiveModel attribute_changed?(attr) ? changed_attributes[attr] : __send__(attr) end + # Handle *_was= for +method_missing+ + def attribute_was=(attr, old_value) + attributes_changed_by_setter[attr] = old_value + end + alias_method :set_attribute_was, :attribute_was= + # Restore all previous data of the provided attributes. def restore_attributes(attributes = changed) attributes.each { |attr| restore_attribute! attr } end + # Remove changes information for the provided attributes. + def clear_attribute_changes(attributes) + attributes_changed_by_setter.except!(*attributes) + end + private + alias_method :attributes_changed_by_setter, :changed_attributes # :nodoc: + # Removes current changes and makes them accessible through +previous_changes+. def changes_applied # :doc: @previously_changed = changes @@ -219,7 +232,7 @@ module ActiveModel rescue TypeError, NoMethodError end - changed_attributes[attr] = value + set_attribute_was(attr, value) end # Handle reset_*! for +method_missing+. @@ -233,7 +246,7 @@ module ActiveModel def restore_attribute!(attr) if attribute_changed?(attr) __send__("#{attr}=", changed_attributes[attr]) - changed_attributes.delete(attr) + clear_attribute_changes([attr]) end end end 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?