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

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

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

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
 

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
 

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
 

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
 

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

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

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

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?)

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)

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

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