Merge branch 'multiparameter-attributes-refactor'

Refactor multiparameter attributes assignment implementation.

2 files changed, 104 insertions, 76 deletions

diff --git a/activerecord/lib/active_record/attribute_assignment.rb b/activerecord/lib/active_record/attribute_assignment.rb
index a54c103476..d9989274c8 100644
--- a/activerecord/lib/active_record/attribute_assignment.rb
+++ b/activerecord/lib/active_record/attribute_assignment.rb
@@ -104,8 +104,7 @@ module ActiveRecord
         end
       end
 
-      # assign any deferred nested attributes after the base attributes have been set
-      nested_parameter_attributes.each { |k,v| _assign_attribute(k, v) }
+      assign_nested_parameter_attributes(nested_parameter_attributes) unless nested_parameter_attributes.empty?
       assign_multiparameter_attributes(multi_parameter_attributes) unless multi_parameter_attributes.empty?
     ensure
       @mass_assignment_options = previous_options
@@ -133,6 +132,11 @@ module ActiveRecord
       end
     end
 
+    # Assign any deferred nested attributes after the base attributes have been set.
+    def assign_nested_parameter_attributes(pairs)
+      pairs.each { |k, v| _assign_attribute(k, v) }
+    end
+
     # Instantiates objects for all attribute classes that needs more than one constructor parameter. This is done
     # by calling new on the column type or aggregation type (through composed_of) object with these parameters.
     # So having the pairs written_on(1) = "2004", written_on(2) = "6", written_on(3) = "24", will instantiate
@@ -146,19 +150,11 @@ module ActiveRecord
       )
     end
 
-    def instantiate_time_object(name, values)
-      if self.class.send(:create_time_zone_conversion_attribute?, name, column_for_attribute(name))
-        Time.zone.local(*values)
-      else
-        Time.time_with_datetime_fallback(self.class.default_timezone, *values)
-      end
-    end
-
     def execute_callstack_for_multiparameter_attributes(callstack)
       errors = []
       callstack.each do |name, values_with_empty_parameters|
         begin
-          send(name + "=", read_value_from_parameter(name, values_with_empty_parameters))
+          send("#{name}=", MultiparameterAttribute.new(self, name, values_with_empty_parameters).read_value)
         rescue => ex
           errors << AttributeAssignmentError.new("error on assignment #{values_with_empty_parameters.values.inspect} to #{name} (#{ex.message})", ex, name)
         end
@@ -169,74 +165,12 @@ module ActiveRecord
       end
     end
 
-    def read_value_from_parameter(name, values_hash_from_param)
-      klass = (self.class.reflect_on_aggregation(name.to_sym) || column_for_attribute(name)).klass
-      if values_hash_from_param.values.all?{|v|v.nil?}
-        nil
-      elsif klass == Time
-        read_time_parameter_value(name, values_hash_from_param)
-      elsif klass == Date
-        read_date_parameter_value(name, values_hash_from_param)
-      else
-        read_other_parameter_value(klass, name, values_hash_from_param)
-      end
-    end
-
-    def read_time_parameter_value(name, values_hash_from_param)
-      # If column is a :time (and not :date or :timestamp) there is no need to validate if
-      # there are year/month/day fields
-      if column_for_attribute(name).type == :time
-        # if the column is a time set the values to their defaults as January 1, 1970, but only if they're nil
-        {1 => 1970, 2 => 1, 3 => 1}.each do |key,value|
-          values_hash_from_param[key] ||= value
-        end
-      else
-        # else column is a timestamp, so if Date bits were not provided, error
-        if missing_parameter = [1,2,3].detect{ |position| !values_hash_from_param.has_key?(position) }
-          raise ArgumentError.new("Missing Parameter - #{name}(#{missing_parameter}i)")
-        end
-
-        # If Date bits were provided but blank, then return nil
-        return nil if (1..3).any? { |position| values_hash_from_param[position].blank? }
-      end
-
-      max_position = extract_max_param_for_multiparameter_attributes(values_hash_from_param, 6)
-      set_values = (1..max_position).collect{ |position| values_hash_from_param[position] }
-      # If Time bits are not there, then default to 0
-      (3..5).each { |i| set_values[i] = set_values[i].blank? ? 0 : set_values[i] }
-      instantiate_time_object(name, set_values)
-    end
-
-    def read_date_parameter_value(name, values_hash_from_param)
-      return nil if (1..3).any? {|position| values_hash_from_param[position].blank?}
-      set_values = [values_hash_from_param[1], values_hash_from_param[2], values_hash_from_param[3]]
-      begin
-        Date.new(*set_values)
-      rescue ArgumentError # if Date.new raises an exception on an invalid date
-        instantiate_time_object(name, set_values).to_date # we instantiate Time object and convert it back to a date thus using Time's logic in handling invalid dates
-      end
-    end
-
-    def read_other_parameter_value(klass, name, values_hash_from_param)
-      max_position = extract_max_param_for_multiparameter_attributes(values_hash_from_param)
-      values = (1..max_position).collect do |position|
-        raise "Missing Parameter" if !values_hash_from_param.has_key?(position)
-        values_hash_from_param[position]
-      end
-      klass.new(*values)
-    end
-
-    def extract_max_param_for_multiparameter_attributes(values_hash_from_param, upper_cap = 100)
-      [values_hash_from_param.keys.max,upper_cap].min
-    end
-
     def extract_callstack_for_multiparameter_attributes(pairs)
       attributes = { }
 
-      pairs.each do |pair|
-        multiparameter_name, value = pair
+      pairs.each do |(multiparameter_name, value)|
         attribute_name = multiparameter_name.split("(").first
-        attributes[attribute_name] = {} unless attributes.include?(attribute_name)
+        attributes[attribute_name] ||= {}
 
         parameter_value = value.empty? ? nil : type_cast_attribute_value(multiparameter_name, value)
         attributes[attribute_name][find_parameter_position(multiparameter_name)] ||= parameter_value
@@ -253,5 +187,100 @@ module ActiveRecord
       multiparameter_name.scan(/\(([0-9]*).*\)/).first.first.to_i
     end
 
+    class MultiparameterAttribute #:nodoc:
+      attr_reader :object, :name, :values, :column
+
+      def initialize(object, name, values)
+        @object = object
+        @name   = name
+        @values = values
+      end
+
+      def read_value
+        return if values.values.compact.empty?
+
+        @column = object.class.reflect_on_aggregation(name.to_sym) || object.column_for_attribute(name)
+        klass   = column.klass
+
+        if klass == Time
+          read_time
+        elsif klass == Date
+          read_date
+        else
+          read_other(klass)
+        end
+      end
+
+      private
+
+      def instantiate_time_object(set_values)
+        if object.class.send(:create_time_zone_conversion_attribute?, name, column)
+          Time.zone.local(*set_values)
+        else
+          Time.time_with_datetime_fallback(object.class.default_timezone, *set_values)
+        end
+      end
+
+      def read_time
+        # If column is a :time (and not :date or :timestamp) there is no need to validate if
+        # there are year/month/day fields
+        if column.type == :time
+          # if the column is a time set the values to their defaults as January 1, 1970, but only if they're nil
+          { 1 => 1970, 2 => 1, 3 => 1 }.each do |key,value|
+            values[key] ||= value
+          end
+        else
+          # else column is a timestamp, so if Date bits were not provided, error
+          validate_missing_parameters!([1,2,3])
+
+          # If Date bits were provided but blank, then return nil
+          return if blank_date_parameter?
+        end
+
+        max_position = extract_max_param(6)
+        set_values   = values.values_at(*(1..max_position))
+        # If Time bits are not there, then default to 0
+        (3..5).each { |i| set_values[i] = set_values[i].presence || 0 }
+        instantiate_time_object(set_values)
+      end
+
+      def read_date
+        return if blank_date_parameter?
+        set_values = values.values_at(1,2,3)
+        begin
+          Date.new(*set_values)
+        rescue ArgumentError # if Date.new raises an exception on an invalid date
+          instantiate_time_object(set_values).to_date # we instantiate Time object and convert it back to a date thus using Time's logic in handling invalid dates
+        end
+      end
+
+      def read_other(klass)
+        max_position = extract_max_param
+        positions    = (1..max_position)
+        validate_missing_parameters!(positions)
+
+        set_values = values.values_at(*positions)
+        klass.new(*set_values)
+      end
+
+      # Checks whether some blank date parameter exists. Note that this is different
+      # than the validate_missing_parameters! method, since it just checks for blank
+      # positions instead of missing ones, and does not raise in case one blank position
+      # exists. The caller is responsible to handle the case of this returning true.
+      def blank_date_parameter?
+        (1..3).any? { |position| values[position].blank? }
+      end
+
+      # If some position is not provided, it errors out a missing parameter exception.
+      def validate_missing_parameters!(positions)
+        if missing_parameter = positions.detect { |position| !values.key?(position) }
+          raise ArgumentError.new("Missing Parameter - #{name}(#{missing_parameter})")
+        end
+      end
+
+      def extract_max_param(upper_cap = 100)
+        [values.keys.max, upper_cap].min
+      end
+    end
   end
 end
diff --git a/activerecord/test/cases/attribute_methods_test.rb b/activerecord/test/cases/attribute_methods_test.rb
index 807971d678..4bc68acd13 100644
--- a/activerecord/test/cases/attribute_methods_test.rb
+++ b/activerecord/test/cases/attribute_methods_test.rb
@@ -34,7 +34,6 @@ class AttributeMethodsTest < ActiveRecord::TestCase
     assert t.attribute_present?("written_on")
     assert !t.attribute_present?("content")
     assert !t.attribute_present?("author_name")
-    
   end
 
   def test_attribute_present_with_booleans