From 8e633e505880755e7e366ccec2210bbe2b5436e7 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Thu, 24 Sep 2015 11:50:11 -0600 Subject: [PATCH] Clean up the implementation of AR::Dirty This moves a bit more of the logic required for dirty checking into the attribute objects. I had hoped to remove the `with_value_from_database` stuff, but unfortunately just calling `dup` on the attribute objects isn't enough, since the values might contain deeply nested data structures. I think this can be cleaned up further. This makes most dirty checking become lazy, and reduces the number of object allocations and amount of CPU time when assigning a value. This opens the door (but doesn't quite finish) to improving the performance of writes to a place comparable to 4.1 --- .../helpers/accepts_multiparameter_time.rb | 8 ++ activemodel/lib/active_model/type/value.rb | 3 + .../associations/has_many_association.rb | 2 +- activerecord/lib/active_record/attribute.rb | 1 + .../attribute/user_provided_default.rb | 2 +- .../active_record/attribute_methods/dirty.rb | 117 ++++++------------ .../lib/active_record/attribute_set.rb | 5 + .../lib/active_record/coders/yaml_column.rb | 17 +-- activerecord/lib/active_record/enum.rb | 8 +- activerecord/lib/active_record/persistence.rb | 1 + .../lib/active_record/type/serialized.rb | 6 + activerecord/test/cases/attribute_set_test.rb | 14 +++ activerecord/test/cases/attribute_test.rb | 19 +++ 13 files changed, 114 insertions(+), 89 deletions(-) diff --git a/activemodel/lib/active_model/type/helpers/accepts_multiparameter_time.rb b/activemodel/lib/active_model/type/helpers/accepts_multiparameter_time.rb index fa1ccd057e..facea12704 100644 --- a/activemodel/lib/active_model/type/helpers/accepts_multiparameter_time.rb +++ b/activemodel/lib/active_model/type/helpers/accepts_multiparameter_time.rb @@ -11,6 +11,14 @@ module ActiveModel end end + define_method(:assert_valid_value) do |value| + if value.is_a?(Hash) + value_from_multiparameter_assignment(value) + else + super(value) + end + end + define_method(:value_from_multiparameter_assignment) do |values_hash| defaults.each do |k, v| values_hash[k] ||= v diff --git a/activemodel/lib/active_model/type/value.rb b/activemodel/lib/active_model/type/value.rb index c7d1197d69..5fea0561a6 100644 --- a/activemodel/lib/active_model/type/value.rb +++ b/activemodel/lib/active_model/type/value.rb @@ -91,6 +91,9 @@ module ActiveModel limit == other.limit end + def assert_valid_value(*) + end + private # Convenience method for types which do not need separate type casting diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 38bda0d2a5..7da20d8eea 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 counter = reflection.counter_cache_column owner[counter] ||= 0 owner[counter] += difference - owner.send(:clear_attribute_changes, counter) # eww + owner.send(:clear_attribute_change, counter) # eww end end diff --git a/activerecord/lib/active_record/attribute.rb b/activerecord/lib/active_record/attribute.rb index 73dd3fa041..21fe032a9c 100644 --- a/activerecord/lib/active_record/attribute.rb +++ b/activerecord/lib/active_record/attribute.rb @@ -55,6 +55,7 @@ module ActiveRecord end def with_value_from_user(value) + type.assert_valid_value(value) self.class.from_user(name, value, type) end diff --git a/activerecord/lib/active_record/attribute/user_provided_default.rb b/activerecord/lib/active_record/attribute/user_provided_default.rb index e0bee8c17e..501590cf0e 100644 --- a/activerecord/lib/active_record/attribute/user_provided_default.rb +++ b/activerecord/lib/active_record/attribute/user_provided_default.rb @@ -16,7 +16,7 @@ module ActiveRecord end end - def changed_in_place_from?(old_value) + def changed_from?(old_value) super || changed_from?(database_default.value) end diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index 0171ef3bdf..fd0a3cd313 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -38,19 +38,37 @@ module ActiveRecord end end + def init_internals + super + @original_attributes = @attributes.dup + end + def initialize_dup(other) # :nodoc: super - calculate_changes_from_defaults + @original_attributes = self.class._default_attributes.dup end def changes_applied super - store_original_raw_attributes + store_original_attributes end def clear_changes_information super - original_raw_attributes.clear + store_original_attributes + end + + def raw_write_attribute(attr_name, *) + result = super + clear_attribute_change(attr_name) + result + end + + def clear_attribute_changes(attr_names) + super + attr_names.each do |attr_name| + clear_attribute_change(attr_name) + end end def changed_attributes @@ -59,7 +77,7 @@ module ActiveRecord if defined?(@cached_changed_attributes) @cached_changed_attributes else - super.reverse_merge(attributes_changed_in_place).freeze + calculate_changed_attributes.freeze end end @@ -70,58 +88,20 @@ module ActiveRecord end def attribute_changed_in_place?(attr_name) - old_value = original_raw_attribute(attr_name) - @attributes[attr_name].changed_in_place_from?(old_value) + original_database_value = @original_attributes[attr_name].value_before_type_cast + @attributes[attr_name].changed_in_place_from?(original_database_value) end private def changes_include?(attr_name) - super || attribute_changed_in_place?(attr_name) + attr_name = attr_name.to_s + super || attribute_modified?(attr_name) || attribute_changed_in_place?(attr_name) end - def calculate_changes_from_defaults - @changed_attributes = nil - self.class.column_defaults.each do |attr, orig_value| - set_attribute_was(attr, orig_value) if _field_changed?(attr, orig_value) - end - end - - # Wrap write_attribute to remember original attribute value. - def write_attribute(attr, value) - attr = attr.to_s - - old_value = old_attribute_value(attr) - - result = super - store_original_raw_attribute(attr) - save_changed_attribute(attr, old_value) - result - end - - def raw_write_attribute(attr, value) - attr = attr.to_s - - result = super - original_raw_attributes[attr] = value - result - end - - def save_changed_attribute(attr, old_value) - clear_changed_attributes_cache - if attribute_changed_by_setter?(attr) - clear_attribute_changes(attr) unless _field_changed?(attr, old_value) - else - set_attribute_was(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 + def clear_attribute_change(attr_name) + attr_name = attr_name.to_s + @original_attributes[attr_name] = @attributes[attr_name].dup end def _update_record(*) @@ -136,40 +116,21 @@ module ActiveRecord changed & self.class.column_names end - def _field_changed?(attr, old_value) - @attributes[attr].changed_from?(old_value) + def attribute_modified?(attr_name) + @attributes[attr_name].changed_from?(@original_attributes.fetch_value(attr_name)) 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 + def store_original_attributes + @original_attributes = @attributes.map do |attr| + attr.with_value_from_database(attr.value_for_database) end end - def changed_in_place - self.class.attribute_names.select do |attr_name| - attribute_changed_in_place?(attr_name) - end - end - - def original_raw_attribute(attr_name) - original_raw_attributes.fetch(attr_name) do - read_attribute_before_type_cast(attr_name) - end - end - - def original_raw_attributes - @original_raw_attributes ||= {} - end - - def store_original_raw_attribute(attr_name) - original_raw_attributes[attr_name] = @attributes[attr_name].value_for_database rescue nil - end - - def store_original_raw_attributes - attribute_names.each do |attr| - store_original_raw_attribute(attr) + def calculate_changed_attributes + attribute_names.each_with_object({}.with_indifferent_access) do |attr_name, result| + if changes_include?(attr_name) + result[attr_name] = @original_attributes.fetch_value(attr_name) + end end end diff --git a/activerecord/lib/active_record/attribute_set.rb b/activerecord/lib/active_record/attribute_set.rb index 013a7d0e01..ee278388a4 100644 --- a/activerecord/lib/active_record/attribute_set.rb +++ b/activerecord/lib/active_record/attribute_set.rb @@ -80,6 +80,11 @@ module ActiveRecord attributes.select { |_, attr| attr.has_been_read? }.keys end + def map(&block) + new_attributes = attributes.transform_values(&block) + AttributeSet.new(new_attributes) + end + protected attr_reader :attributes diff --git a/activerecord/lib/active_record/coders/yaml_column.rb b/activerecord/lib/active_record/coders/yaml_column.rb index 9ea22ed798..2456b8ad8c 100644 --- a/activerecord/lib/active_record/coders/yaml_column.rb +++ b/activerecord/lib/active_record/coders/yaml_column.rb @@ -14,10 +14,7 @@ module ActiveRecord def dump(obj) return if obj.nil? - unless obj.is_a?(object_class) - raise SerializationTypeMismatch, - "Attribute was supposed to be a #{object_class}, but was a #{obj.class}. -- #{obj.inspect}" - end + assert_valid_value(obj) YAML.dump obj end @@ -26,15 +23,19 @@ module ActiveRecord return yaml unless yaml.is_a?(String) && yaml =~ /^---/ obj = YAML.load(yaml) - unless obj.is_a?(object_class) || obj.nil? - raise SerializationTypeMismatch, - "Attribute was supposed to be a #{object_class}, but was a #{obj.class}" - end + assert_valid_value(obj) obj ||= object_class.new if object_class != Object obj end + def assert_valid_value(obj) + unless obj.nil? || obj.is_a?(object_class) + raise SerializationTypeMismatch, + "Attribute was supposed to be a #{object_class}, but was a #{obj.class}. -- #{obj.inspect}" + end + end + private def check_arity_of_constructor diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index a79bf0366b..10b5fcab24 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -118,7 +118,7 @@ module ActiveRecord elsif mapping.has_value?(value) mapping.key(value) else - raise ArgumentError, "'#{value}' is not a valid #{name}" + assert_valid_value(value) end end @@ -131,6 +131,12 @@ module ActiveRecord mapping.fetch(value, value) end + def assert_valid_value(value) + unless value.blank? || mapping.has_key?(value) || mapping.has_value?(value) + raise ArgumentError, "'#{value}' is not a valid #{name}" + end + end + protected attr_reader :name, :mapping diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 09c36d7b4d..1b7ee0bd38 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -211,6 +211,7 @@ module ActiveRecord def becomes(klass) became = klass.new became.instance_variable_set("@attributes", @attributes) + became.instance_variable_set("@original_attributes", @original_attributes) became.instance_variable_set("@changed_attributes", attributes_changed_by_setter) became.instance_variable_set("@new_record", new_record?) became.instance_variable_set("@destroyed", destroyed?) diff --git a/activerecord/lib/active_record/type/serialized.rb b/activerecord/lib/active_record/type/serialized.rb index 203a395415..4ff0740cfb 100644 --- a/activerecord/lib/active_record/type/serialized.rb +++ b/activerecord/lib/active_record/type/serialized.rb @@ -41,6 +41,12 @@ module ActiveRecord ActiveRecord::Store::IndifferentHashAccessor end + def assert_valid_value(value) + if coder.respond_to?(:assert_valid_value) + coder.assert_valid_value(value) + end + end + private def default_value?(value) diff --git a/activerecord/test/cases/attribute_set_test.rb b/activerecord/test/cases/attribute_set_test.rb index 9d927481ec..7524243270 100644 --- a/activerecord/test/cases/attribute_set_test.rb +++ b/activerecord/test/cases/attribute_set_test.rb @@ -160,6 +160,9 @@ module ActiveRecord return if value.nil? value + " from database" end + + def assert_valid_value(*) + end end test "write_from_database sets the attribute with database typecasting" do @@ -207,5 +210,16 @@ module ActiveRecord assert_equal [:foo], attributes.accessed end + + test "#map returns a new attribute set with the changes applied" do + builder = AttributeSet::Builder.new(foo: Type::Integer.new, bar: Type::Integer.new) + attributes = builder.build_from_database(foo: "1", bar: "2") + new_attributes = attributes.map do |attr| + attr.with_cast_value(attr.value + 1) + end + + assert_equal 2, new_attributes.fetch_value(:foo) + assert_equal 3, new_attributes.fetch_value(:bar) + end end end diff --git a/activerecord/test/cases/attribute_test.rb b/activerecord/test/cases/attribute_test.rb index c69782d93f..0f216b7a13 100644 --- a/activerecord/test/cases/attribute_test.rb +++ b/activerecord/test/cases/attribute_test.rb @@ -107,6 +107,9 @@ module ActiveRecord def deserialize(value) value + " from database" end + + def assert_valid_value(*) + end end test "with_value_from_user returns a new attribute with the value from the user" do @@ -186,5 +189,21 @@ module ActiveRecord assert_not attribute.changed_in_place_from?("bar") end + + test "with_value_from_user validates the value" do + type = Type::Value.new + type.define_singleton_method(:assert_valid_value) do |value| + if value == 1 + raise ArgumentError + end + end + + attribute = Attribute.from_database(:foo, 1, type) + assert_equal 1, attribute.value + assert_equal 2, attribute.with_value_from_user(2).value + assert_raises ArgumentError do + attribute.with_value_from_user(1) + end + end end end