Rails Lighthouse ticket #4346

4346.diff

From 59043a3a278d71755223ed8eef09171e2a74b9df Mon Sep 17 00:00:00 2001
From: Aditya Sanghi <[email protected]>
Date: Wed, 13 Oct 2010 19:57:21 +0530
Subject: [PATCH] Multiparameter attribute fixes for POLA failures

---
 activerecord/lib/active_record/base.rb |   81 +++++++++++------
 activerecord/test/cases/base_test.rb   |  146 +++++++++++++++++++++++++++++--
 2 files changed, 188 insertions(+), 39 deletions(-)

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 78b3507..d87f7fc 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1733,32 +1733,9 @@ MSG
         errors = []
         callstack.each do |name, values_with_empty_parameters|
           begin
-            klass = (self.class.reflect_on_aggregation(name.to_sym) || column_for_attribute(name)).klass
-            # in order to allow a date to be set without a year, we must keep the empty values.
-            # Otherwise, we wouldn't be able to distinguish it from a date with an empty day.
-            values = values_with_empty_parameters.reject { |v| v.nil? }
-
-            if values.empty?
-              send(name + "=", nil)
-            else
-
-              value = if Time == klass
-                instantiate_time_object(name, values)
-              elsif Date == klass
-                begin
-                  values = values_with_empty_parameters.collect do |v| v.nil? ? 1 : v end
-                  Date.new(*values)
-                rescue ArgumentError => ex # if Date.new raises an exception on an invalid date
-                  instantiate_time_object(name, values).to_date # we instantiate Time object and convert it back to a date thus using Time's logic in handling invalid dates
-                end
-              else
-                klass.new(*values)
-              end
-
-              send(name + "=", value)
-            end
+            send(name + "=", read_value_from_parameter(name, values_with_empty_parameters))
           rescue => ex
-            errors << AttributeAssignmentError.new("error on assignment #{values.inspect} to #{name}", ex, name)
+            errors << AttributeAssignmentError.new("error on assignment #{values_with_empty_parameters.values.inspect} to #{name}", ex, name)
           end
         end
         unless errors.empty?
@@ -1766,19 +1743,65 @@ MSG
         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 Date bits were not provided, error
+        raise "Missing Parameter" if [1,2,3].any?{|position| !values_hash_from_param.has_key?(position)}
+        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 Date bits were provided but blank, then default to 1
+        # If Time bits are not there, then default to 0
+        [1,1,1,0,0,0].each_with_index{|v,i| set_values[i] = set_values[i].blank? ? v : set_values[i]}
+        instantiate_time_object(name, set_values)
+      end
+
+      def read_date_parameter_value(name, values_hash_from_param)
+        set_values = (1..3).collect{|position| values_hash_from_param[position].blank? ? 1 : values_hash_from_param[position]}
+        begin
+          Date.new(*set_values)
+        rescue ArgumentError => ex # 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 = { }
 
         for pair in pairs
           multiparameter_name, value = pair
           attribute_name = multiparameter_name.split("(").first
-          attributes[attribute_name] = [] unless attributes.include?(attribute_name)
+          attributes[attribute_name] = {} unless attributes.include?(attribute_name)
 
           parameter_value = value.empty? ? nil : type_cast_attribute_value(multiparameter_name, value)
-          attributes[attribute_name] << [ find_parameter_position(multiparameter_name), parameter_value ]
+          attributes[attribute_name][find_parameter_position(multiparameter_name)] ||= parameter_value
         end
 
-        attributes.each { |name, values| attributes[name] = values.sort_by{ |v| v.first }.collect { |v| v.last } }
+        attributes
       end
 
       def type_cast_attribute_value(multiparameter_name, value)
@@ -1786,7 +1809,7 @@ MSG
       end
 
       def find_parameter_position(multiparameter_name)
-        multiparameter_name.scan(/\(([0-9]*).*\)/).first.first
+        multiparameter_name.scan(/\(([0-9]*).*\)/).first.first.to_i
       end
 
       # Returns a comma-separated pair list, like "key1 = val1, key2 = val2".
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 16fd9a7..18609db 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -175,16 +175,6 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal("initialized from attributes", topic.title)
   end
 
-  def test_initialize_with_invalid_attribute
-    begin
-      topic = Topic.new({ "title" => "test",
-        "last_read(1i)" => "2005", "last_read(2i)" => "2", "last_read(3i)" => "31"})
-    rescue ActiveRecord::MultiparameterAssignmentErrors => ex
-      assert_equal(1, ex.errors.size)
-      assert_equal("last_read", ex.errors[0].attribute)
-    end
-  end
-
   def test_load
     topics = Topic.find(:all, :order => 'id')
     assert_equal(4, topics.size)
@@ -489,6 +479,29 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on
   end
 
+  def test_multiparameter_attributes_on_time_with_no_date
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(4i)" => "16", "written_on(5i)" => "24", "written_on(6i)" => "00"
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_attributes_on_time_with_invalid_time_params
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(1i)" => "2004", "written_on(2i)" => "6", "written_on(3i)" => "24",
+        "written_on(4i)" => "2004", "written_on(5i)" => "36", "written_on(6i)" => "64",
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
   def test_multiparameter_attributes_on_time_with_old_date
     attributes = {
       "written_on(1i)" => "1850", "written_on(2i)" => "6", "written_on(3i)" => "24",
@@ -598,6 +611,83 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on
   end
 
+  def test_multiparameter_attributes_on_time_will_raise_on_big_time_if_missing_date_parts
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(4i)" => "16", "written_on(5i)" => "24"
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_attributes_on_time_with_raise_on_small_time_if_missing_date_parts
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_hour_if_missing
+    attributes = {
+      "written_on(1i)" => "2004", "written_on(2i)" => "12", "written_on(3i)" => "12",
+      "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(2004, 12, 12, 0, 12, 2), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_hour_if_blank
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal 1, topic.written_on.year
+    assert_equal 1, topic.written_on.month
+    assert_equal 1, topic.written_on.day
+    assert_equal 0, topic.written_on.hour
+    assert_equal 12, topic.written_on.min
+    assert_equal 2, topic.written_on.sec
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_date_if_empty
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "16", "written_on(5i)" => "24"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal 1, topic.written_on.year
+    assert_equal 1, topic.written_on.month
+    assert_equal 1, topic.written_on.day
+    assert_equal 16, topic.written_on.hour
+    assert_equal 24, topic.written_on.min
+    assert_equal 0, topic.written_on.sec
+  end
+
+  def test_multiparameter_attributes_on_time_with_seconds_will_ignore_date_if_empty
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal 1, topic.written_on.year
+    assert_equal 1, topic.written_on.month
+    assert_equal 1, topic.written_on.day
+    assert_equal 16, topic.written_on.hour
+    assert_equal 12, topic.written_on.min
+    assert_equal 02, topic.written_on.sec
+  end
+
   def test_multiparameter_assignment_of_aggregation
     customer = Customer.new
     address = Address.new("The Street", "The City", "The Country")
@@ -606,6 +696,42 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal address, customer.address
   end
 
+  def test_multiparameter_assignment_of_aggregation_out_of_order
+    customer = Customer.new
+    address = Address.new("The Street", "The City", "The Country")
+    attributes = { "address(3)" => address.country, "address(2)" => address.city, "address(1)" => address.street }
+    customer.attributes = attributes
+    assert_equal address, customer.address
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_missing_values
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      customer = Customer.new
+      address = Address.new("The Street", "The City", "The Country")
+      attributes = { "address(2)" => address.city, "address(3)" => address.country }
+      customer.attributes = attributes
+    end
+    assert_equal("address", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_blank_values
+    customer = Customer.new
+    address = Address.new("The Street", "The City", "The Country")
+    attributes = { "address(1)" => "", "address(2)" => address.city, "address(3)" => address.country }
+    customer.attributes = attributes
+    assert_equal Address.new(nil, "The City", "The Country"), customer.address
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_large_index
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      customer = Customer.new
+      address = Address.new("The Street", "The City", "The Country")
+      attributes = { "address(1)" => "The Street", "address(2)" => address.city, "address(3000)" => address.country }
+      customer.attributes = attributes
+    end
+    assert_equal("address", ex.errors[0].attribute)
+  end
+
   def test_attributes_on_dummy_time
     # Oracle, and Sybase do not have a TIME datatype.
     return true if current_adapter?(:OracleAdapter, :SybaseAdapter)
-- 
1.7.1

4346_master_new.diff

From 13e5b287173e0aef7d77581366277d461330f046 Mon Sep 17 00:00:00 2001
From: Aditya Sanghi <[email protected]>
Date: Wed, 13 Oct 2010 19:57:21 +0530
Subject: [PATCH] Multiparameter attribute fixes for POLA failures

---
 activerecord/lib/active_record/base.rb |   81 +++++++++++------
 activerecord/test/cases/base_test.rb   |  146 +++++++++++++++++++++++++++++--
 2 files changed, 188 insertions(+), 39 deletions(-)

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index b35f59d..2d9a481 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1738,32 +1738,9 @@ MSG
         errors = []
         callstack.each do |name, values_with_empty_parameters|
           begin
-            klass = (self.class.reflect_on_aggregation(name.to_sym) || column_for_attribute(name)).klass
-            # in order to allow a date to be set without a year, we must keep the empty values.
-            # Otherwise, we wouldn't be able to distinguish it from a date with an empty day.
-            values = values_with_empty_parameters.reject { |v| v.nil? }
-
-            if values.empty?
-              send(name + "=", nil)
-            else
-
-              value = if Time == klass
-                instantiate_time_object(name, values)
-              elsif Date == klass
-                begin
-                  values = values_with_empty_parameters.collect do |v| v.nil? ? 1 : v end
-                  Date.new(*values)
-                rescue ArgumentError => ex # if Date.new raises an exception on an invalid date
-                  instantiate_time_object(name, values).to_date # we instantiate Time object and convert it back to a date thus using Time's logic in handling invalid dates
-                end
-              else
-                klass.new(*values)
-              end
-
-              send(name + "=", value)
-            end
+            send(name + "=", read_value_from_parameter(name, values_with_empty_parameters))
           rescue => ex
-            errors << AttributeAssignmentError.new("error on assignment #{values.inspect} to #{name}", ex, name)
+            errors << AttributeAssignmentError.new("error on assignment #{values_with_empty_parameters.values.inspect} to #{name}", ex, name)
           end
         end
         unless errors.empty?
@@ -1771,19 +1748,65 @@ MSG
         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 Date bits were not provided, error
+        raise "Missing Parameter" if [1,2,3].any?{|position| !values_hash_from_param.has_key?(position)}
+        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 Date bits were provided but blank, then default to 1
+        # If Time bits are not there, then default to 0
+        [1,1,1,0,0,0].each_with_index{|v,i| set_values[i] = set_values[i].blank? ? v : set_values[i]}
+        instantiate_time_object(name, set_values)
+      end
+
+      def read_date_parameter_value(name, values_hash_from_param)
+        set_values = (1..3).collect{|position| values_hash_from_param[position].blank? ? 1 : values_hash_from_param[position]}
+        begin
+          Date.new(*set_values)
+        rescue ArgumentError => ex # 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 = { }
 
         for pair in pairs
           multiparameter_name, value = pair
           attribute_name = multiparameter_name.split("(").first
-          attributes[attribute_name] = [] unless attributes.include?(attribute_name)
+          attributes[attribute_name] = {} unless attributes.include?(attribute_name)
 
           parameter_value = value.empty? ? nil : type_cast_attribute_value(multiparameter_name, value)
-          attributes[attribute_name] << [ find_parameter_position(multiparameter_name), parameter_value ]
+          attributes[attribute_name][find_parameter_position(multiparameter_name)] ||= parameter_value
         end
 
-        attributes.each { |name, values| attributes[name] = values.sort_by{ |v| v.first }.collect { |v| v.last } }
+        attributes
       end
 
       def type_cast_attribute_value(multiparameter_name, value)
@@ -1791,7 +1814,7 @@ MSG
       end
 
       def find_parameter_position(multiparameter_name)
-        multiparameter_name.scan(/\(([0-9]*).*\)/).first.first
+        multiparameter_name.scan(/\(([0-9]*).*\)/).first.first.to_i
       end
 
       # Returns a comma-separated pair list, like "key1 = val1, key2 = val2".
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 73c7660..2f83cde 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -180,16 +180,6 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal("initialized from attributes", topic.title)
   end
 
-  def test_initialize_with_invalid_attribute
-    begin
-      topic = Topic.new({ "title" => "test",
-        "last_read(1i)" => "2005", "last_read(2i)" => "2", "last_read(3i)" => "31"})
-    rescue ActiveRecord::MultiparameterAssignmentErrors => ex
-      assert_equal(1, ex.errors.size)
-      assert_equal("last_read", ex.errors[0].attribute)
-    end
-  end
-
   def test_load
     topics = Topic.find(:all, :order => 'id')
     assert_equal(4, topics.size)
@@ -503,6 +493,29 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on
   end
 
+  def test_multiparameter_attributes_on_time_with_no_date
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(4i)" => "16", "written_on(5i)" => "24", "written_on(6i)" => "00"
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_attributes_on_time_with_invalid_time_params
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(1i)" => "2004", "written_on(2i)" => "6", "written_on(3i)" => "24",
+        "written_on(4i)" => "2004", "written_on(5i)" => "36", "written_on(6i)" => "64",
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
   def test_multiparameter_attributes_on_time_with_old_date
     attributes = {
       "written_on(1i)" => "1850", "written_on(2i)" => "6", "written_on(3i)" => "24",
@@ -612,6 +625,83 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on
   end
 
+  def test_multiparameter_attributes_on_time_will_raise_on_big_time_if_missing_date_parts
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(4i)" => "16", "written_on(5i)" => "24"
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_attributes_on_time_with_raise_on_small_time_if_missing_date_parts
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_hour_if_missing
+    attributes = {
+      "written_on(1i)" => "2004", "written_on(2i)" => "12", "written_on(3i)" => "12",
+      "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(2004, 12, 12, 0, 12, 2), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_hour_if_blank
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal 1, topic.written_on.year
+    assert_equal 1, topic.written_on.month
+    assert_equal 1, topic.written_on.day
+    assert_equal 0, topic.written_on.hour
+    assert_equal 12, topic.written_on.min
+    assert_equal 2, topic.written_on.sec
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_date_if_empty
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "16", "written_on(5i)" => "24"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal 1, topic.written_on.year
+    assert_equal 1, topic.written_on.month
+    assert_equal 1, topic.written_on.day
+    assert_equal 16, topic.written_on.hour
+    assert_equal 24, topic.written_on.min
+    assert_equal 0, topic.written_on.sec
+  end
+
+  def test_multiparameter_attributes_on_time_with_seconds_will_ignore_date_if_empty
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal 1, topic.written_on.year
+    assert_equal 1, topic.written_on.month
+    assert_equal 1, topic.written_on.day
+    assert_equal 16, topic.written_on.hour
+    assert_equal 12, topic.written_on.min
+    assert_equal 02, topic.written_on.sec
+  end
+
   def test_multiparameter_assignment_of_aggregation
     customer = Customer.new
     address = Address.new("The Street", "The City", "The Country")
@@ -620,6 +710,42 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal address, customer.address
   end
 
+  def test_multiparameter_assignment_of_aggregation_out_of_order
+    customer = Customer.new
+    address = Address.new("The Street", "The City", "The Country")
+    attributes = { "address(3)" => address.country, "address(2)" => address.city, "address(1)" => address.street }
+    customer.attributes = attributes
+    assert_equal address, customer.address
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_missing_values
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      customer = Customer.new
+      address = Address.new("The Street", "The City", "The Country")
+      attributes = { "address(2)" => address.city, "address(3)" => address.country }
+      customer.attributes = attributes
+    end
+    assert_equal("address", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_blank_values
+    customer = Customer.new
+    address = Address.new("The Street", "The City", "The Country")
+    attributes = { "address(1)" => "", "address(2)" => address.city, "address(3)" => address.country }
+    customer.attributes = attributes
+    assert_equal Address.new(nil, "The City", "The Country"), customer.address
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_large_index
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      customer = Customer.new
+      address = Address.new("The Street", "The City", "The Country")
+      attributes = { "address(1)" => "The Street", "address(2)" => address.city, "address(3000)" => address.country }
+      customer.attributes = attributes
+    end
+    assert_equal("address", ex.errors[0].attribute)
+  end
+
   def test_attributes_on_dummy_time
     # Oracle, and Sybase do not have a TIME datatype.
     return true if current_adapter?(:OracleAdapter, :SybaseAdapter)
-- 
1.7.1

more_tests.diff

diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index a44999a..f3560de 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -620,6 +620,64 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on
   end
 
+  def test_multiparameter_attributes_on_time_will_raise_on_big_time_if_missing_date_parts
+    attributes = {
+      "written_on(4i)" => "16", "written_on(5i)" => "24"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(1, 1, 1, 16, 24, 0), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_with_raise_on_small_time_if_missing_date_parts
+    attributes = {
+      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(1, 1, 1, 16, 12, 2), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_with_ignore_hour_if_missing
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(1, 1, 1, 0, 12, 2), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_with_ignore_hour_if_empty
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(1, 1, 1, 0, 12, 2), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_date_if_empty
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "16", "written_on(5i)" => "24"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(1, 1, 1, 16, 24), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_with_seconds_will_ignore_date_if_empty
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(1, 1, 1, 16, 12, 02), topic.written_on
+  end
+
   def test_multiparameter_assignment_of_aggregation
     customer = Customer.new
     address = Address.new("The Street", "The City", "The Country")
@@ -628,6 +686,30 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal address, customer.address
   end
 
+  def test_multiparameter_assignment_of_aggregation_out_of_order
+    customer = Customer.new
+    address = Address.new("The Street", "The City", "The Country")
+    attributes = { "address(3)" => address.country, "address(2)" => address.city, "address(1)" => address.street }
+    customer.attributes = attributes
+    assert_equal address, customer.address
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_missing_values
+    customer = Customer.new
+    address = Address.new("The Street", "The City", "The Country")
+    attributes = { "address(2)" => address.city, "address(3)" => address.country }
+    customer.attributes = attributes
+    assert_equal Address.new(nil, "The City", "The Country"), customer.address
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_missing_values
+    customer = Customer.new
+    address = Address.new("The Street", "The City", "The Country")
+    attributes = { "address(1)" => "", "address(2)" => address.city, "address(3)" => address.country }
+    customer.attributes = attributes
+    assert_equal Address.new(nil, "The City", "The Country"), customer.address
+  end
+
   def test_attributes_on_dummy_time
     # Oracle, and Sybase do not have a TIME datatype.
     return true if current_adapter?(:OracleAdapter, :SybaseAdapter)

multi_param_pola_fixes.diff

From be310ecac28e98b5ddf6831c7e09ef6c65e2f240 Mon Sep 17 00:00:00 2001
From: Aditya Sanghi <[email protected]>
Date: Wed, 13 Oct 2010 19:57:21 +0530
Subject: [PATCH] Multiparameter attribute fixes for POLA failures

---
 activerecord/lib/active_record/base.rb |   28 ++++++++++++++++++++--------
 activerecord/test/cases/base_test.rb   |   22 ++++++++++++++++++++++
 2 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 78b3507..2c5084f 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1736,23 +1736,35 @@ MSG
             klass = (self.class.reflect_on_aggregation(name.to_sym) || column_for_attribute(name)).klass
             # in order to allow a date to be set without a year, we must keep the empty values.
             # Otherwise, we wouldn't be able to distinguish it from a date with an empty day.
-            values = values_with_empty_parameters.reject { |v| v.nil? }
+            # values_with_empty_parameters = {"1" => 2009, "2" => 12, "3" => 31}
+            values = values_with_empty_parameters.values.reject(&:nil?)
 
             if values.empty?
               send(name + "=", nil)
             else
 
+              # Ensure there are no missing bits which cause POLA failures
+              # set_values = [2009, 2, 31]
+              # In case values_with_empty_parameters = {"4" => 23, "5" => 11, "6" => 11}
+              # set_values = [nil, nil, nil, 23, 11, 11]
+              set_values = (1..values_with_empty_parameters.keys.max.to_i).to_a.collect do |position|
+                values_with_empty_parameters[position.to_s]
+              end
+
               value = if Time == klass
-                instantiate_time_object(name, values)
+                # Default the date component to if not given to 0001-01-01
+                [2001,1,1].each_with_index{|v,i| set_values[i] = set_values[i].blank? ? v : set_values[i]}
+                instantiate_time_object(name, set_values)
               elsif Date == klass
                 begin
-                  values = values_with_empty_parameters.collect do |v| v.nil? ? 1 : v end
-                  Date.new(*values)
+                  # Default the date component to if not given to 0001-01-01
+                  [1,1,1].each_with_index{|v,i| set_values[i] = set_values[i].blank? ? v : set_values[i]}
+                  Date.new(*set_values)
                 rescue ArgumentError => ex # if Date.new raises an exception on an invalid date
                   instantiate_time_object(name, values).to_date # we instantiate Time object and convert it back to a date thus using Time's logic in handling invalid dates
                 end
               else
-                klass.new(*values)
+                klass.new(*set_values)
               end
 
               send(name + "=", value)
@@ -1772,13 +1784,13 @@ MSG
         for pair in pairs
           multiparameter_name, value = pair
           attribute_name = multiparameter_name.split("(").first
-          attributes[attribute_name] = [] unless attributes.include?(attribute_name)
+          attributes[attribute_name] = {} unless attributes.include?(attribute_name)
 
           parameter_value = value.empty? ? nil : type_cast_attribute_value(multiparameter_name, value)
-          attributes[attribute_name] << [ find_parameter_position(multiparameter_name), parameter_value ]
+          attributes[attribute_name][find_parameter_position(multiparameter_name)] ||= parameter_value
         end
 
-        attributes.each { |name, values| attributes[name] = values.sort_by{ |v| v.first }.collect { |v| v.last } }
+        attributes
       end
 
       def type_cast_attribute_value(multiparameter_name, value)
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 16fd9a7..a44999a 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -489,6 +489,28 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on
   end
 
+  def test_multiparameter_attributes_on_time_with_no_date
+    attributes = {
+      "written_on(4i)" => "16", "written_on(5i)" => "24", "written_on(6i)" => "00"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(2001, 1, 1, 16, 24, 0), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_with_invalid_time_params
+    begin
+      attributes = {
+        "written_on(4i)" => "2004", "written_on(5i)" => "36", "written_on(6i)" => "64",
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    rescue ActiveRecord::MultiparameterAssignmentErrors => ex
+      assert_equal(1, ex.errors.size)
+      assert_equal("written_on", ex.errors[0].attribute)
+    end
+  end
+
   def test_multiparameter_attributes_on_time_with_old_date
     attributes = {
       "written_on(1i)" => "1850", "written_on(2i)" => "6", "written_on(3i)" => "24",
-- 
1.7.1

multi_param_pola_fixes_with_review_changes.diff

From 9966041a7ceb9cbaca52e5d2a471e8ce24edbfe6 Mon Sep 17 00:00:00 2001
From: Aditya Sanghi <[email protected]>
Date: Wed, 13 Oct 2010 19:57:21 +0530
Subject: [PATCH 1/2] Multiparameter attribute fixes for POLA failures

---
 activerecord/lib/active_record/base.rb |   28 ++++++++++++++++++++--------
 activerecord/test/cases/base_test.rb   |   22 ++++++++++++++++++++++
 2 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 78b3507..2c5084f 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1736,23 +1736,35 @@ MSG
             klass = (self.class.reflect_on_aggregation(name.to_sym) || column_for_attribute(name)).klass
             # in order to allow a date to be set without a year, we must keep the empty values.
             # Otherwise, we wouldn't be able to distinguish it from a date with an empty day.
-            values = values_with_empty_parameters.reject { |v| v.nil? }
+            # values_with_empty_parameters = {"1" => 2009, "2" => 12, "3" => 31}
+            values = values_with_empty_parameters.values.reject(&:nil?)
 
             if values.empty?
               send(name + "=", nil)
             else
 
+              # Ensure there are no missing bits which cause POLA failures
+              # set_values = [2009, 2, 31]
+              # In case values_with_empty_parameters = {"4" => 23, "5" => 11, "6" => 11}
+              # set_values = [nil, nil, nil, 23, 11, 11]
+              set_values = (1..values_with_empty_parameters.keys.max.to_i).to_a.collect do |position|
+                values_with_empty_parameters[position.to_s]
+              end
+
               value = if Time == klass
-                instantiate_time_object(name, values)
+                # Default the date component to if not given to 0001-01-01
+                [2001,1,1].each_with_index{|v,i| set_values[i] = set_values[i].blank? ? v : set_values[i]}
+                instantiate_time_object(name, set_values)
               elsif Date == klass
                 begin
-                  values = values_with_empty_parameters.collect do |v| v.nil? ? 1 : v end
-                  Date.new(*values)
+                  # Default the date component to if not given to 0001-01-01
+                  [1,1,1].each_with_index{|v,i| set_values[i] = set_values[i].blank? ? v : set_values[i]}
+                  Date.new(*set_values)
                 rescue ArgumentError => ex # if Date.new raises an exception on an invalid date
                   instantiate_time_object(name, values).to_date # we instantiate Time object and convert it back to a date thus using Time's logic in handling invalid dates
                 end
               else
-                klass.new(*values)
+                klass.new(*set_values)
               end
 
               send(name + "=", value)
@@ -1772,13 +1784,13 @@ MSG
         for pair in pairs
           multiparameter_name, value = pair
           attribute_name = multiparameter_name.split("(").first
-          attributes[attribute_name] = [] unless attributes.include?(attribute_name)
+          attributes[attribute_name] = {} unless attributes.include?(attribute_name)
 
           parameter_value = value.empty? ? nil : type_cast_attribute_value(multiparameter_name, value)
-          attributes[attribute_name] << [ find_parameter_position(multiparameter_name), parameter_value ]
+          attributes[attribute_name][find_parameter_position(multiparameter_name)] ||= parameter_value
         end
 
-        attributes.each { |name, values| attributes[name] = values.sort_by{ |v| v.first }.collect { |v| v.last } }
+        attributes
       end
 
       def type_cast_attribute_value(multiparameter_name, value)
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 16fd9a7..a44999a 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -489,6 +489,28 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on
   end
 
+  def test_multiparameter_attributes_on_time_with_no_date
+    attributes = {
+      "written_on(4i)" => "16", "written_on(5i)" => "24", "written_on(6i)" => "00"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(2001, 1, 1, 16, 24, 0), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_with_invalid_time_params
+    begin
+      attributes = {
+        "written_on(4i)" => "2004", "written_on(5i)" => "36", "written_on(6i)" => "64",
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    rescue ActiveRecord::MultiparameterAssignmentErrors => ex
+      assert_equal(1, ex.errors.size)
+      assert_equal("written_on", ex.errors[0].attribute)
+    end
+  end
+
   def test_multiparameter_attributes_on_time_with_old_date
     attributes = {
       "written_on(1i)" => "1850", "written_on(2i)" => "6", "written_on(3i)" => "24",
-- 
1.7.1


From 7260aed28a0d2c7e1134e2b3a0b3626534827c5e Mon Sep 17 00:00:00 2001
From: Aditya Sanghi <[email protected]>
Date: Fri, 15 Oct 2010 22:19:04 +0530
Subject: [PATCH 2/2] Review changes for multiparameter attributes from Andrea Campi; refactored; added tests

---
 activerecord/lib/active_record/base.rb |   87 +++++++++++---------
 activerecord/test/cases/base_test.rb   |  144 +++++++++++++++++++++++++++-----
 2 files changed, 173 insertions(+), 58 deletions(-)

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 2c5084f..d87f7fc 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1733,44 +1733,9 @@ MSG
         errors = []
         callstack.each do |name, values_with_empty_parameters|
           begin
-            klass = (self.class.reflect_on_aggregation(name.to_sym) || column_for_attribute(name)).klass
-            # in order to allow a date to be set without a year, we must keep the empty values.
-            # Otherwise, we wouldn't be able to distinguish it from a date with an empty day.
-            # values_with_empty_parameters = {"1" => 2009, "2" => 12, "3" => 31}
-            values = values_with_empty_parameters.values.reject(&:nil?)
-
-            if values.empty?
-              send(name + "=", nil)
-            else
-
-              # Ensure there are no missing bits which cause POLA failures
-              # set_values = [2009, 2, 31]
-              # In case values_with_empty_parameters = {"4" => 23, "5" => 11, "6" => 11}
-              # set_values = [nil, nil, nil, 23, 11, 11]
-              set_values = (1..values_with_empty_parameters.keys.max.to_i).to_a.collect do |position|
-                values_with_empty_parameters[position.to_s]
-              end
-
-              value = if Time == klass
-                # Default the date component to if not given to 0001-01-01
-                [2001,1,1].each_with_index{|v,i| set_values[i] = set_values[i].blank? ? v : set_values[i]}
-                instantiate_time_object(name, set_values)
-              elsif Date == klass
-                begin
-                  # Default the date component to if not given to 0001-01-01
-                  [1,1,1].each_with_index{|v,i| set_values[i] = set_values[i].blank? ? v : set_values[i]}
-                  Date.new(*set_values)
-                rescue ArgumentError => ex # if Date.new raises an exception on an invalid date
-                  instantiate_time_object(name, values).to_date # we instantiate Time object and convert it back to a date thus using Time's logic in handling invalid dates
-                end
-              else
-                klass.new(*set_values)
-              end
-
-              send(name + "=", value)
-            end
+            send(name + "=", read_value_from_parameter(name, values_with_empty_parameters))
           rescue => ex
-            errors << AttributeAssignmentError.new("error on assignment #{values.inspect} to #{name}", ex, name)
+            errors << AttributeAssignmentError.new("error on assignment #{values_with_empty_parameters.values.inspect} to #{name}", ex, name)
           end
         end
         unless errors.empty?
@@ -1778,6 +1743,52 @@ MSG
         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 Date bits were not provided, error
+        raise "Missing Parameter" if [1,2,3].any?{|position| !values_hash_from_param.has_key?(position)}
+        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 Date bits were provided but blank, then default to 1
+        # If Time bits are not there, then default to 0
+        [1,1,1,0,0,0].each_with_index{|v,i| set_values[i] = set_values[i].blank? ? v : set_values[i]}
+        instantiate_time_object(name, set_values)
+      end
+
+      def read_date_parameter_value(name, values_hash_from_param)
+        set_values = (1..3).collect{|position| values_hash_from_param[position].blank? ? 1 : values_hash_from_param[position]}
+        begin
+          Date.new(*set_values)
+        rescue ArgumentError => ex # 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 = { }
 
@@ -1798,7 +1809,7 @@ MSG
       end
 
       def find_parameter_position(multiparameter_name)
-        multiparameter_name.scan(/\(([0-9]*).*\)/).first.first
+        multiparameter_name.scan(/\(([0-9]*).*\)/).first.first.to_i
       end
 
       # Returns a comma-separated pair list, like "key1 = val1, key2 = val2".
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index a44999a..18609db 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -175,16 +175,6 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal("initialized from attributes", topic.title)
   end
 
-  def test_initialize_with_invalid_attribute
-    begin
-      topic = Topic.new({ "title" => "test",
-        "last_read(1i)" => "2005", "last_read(2i)" => "2", "last_read(3i)" => "31"})
-    rescue ActiveRecord::MultiparameterAssignmentErrors => ex
-      assert_equal(1, ex.errors.size)
-      assert_equal("last_read", ex.errors[0].attribute)
-    end
-  end
-
   def test_load
     topics = Topic.find(:all, :order => 'id')
     assert_equal(4, topics.size)
@@ -490,25 +480,26 @@ class BasicsTest < ActiveRecord::TestCase
   end
 
   def test_multiparameter_attributes_on_time_with_no_date
-    attributes = {
-      "written_on(4i)" => "16", "written_on(5i)" => "24", "written_on(6i)" => "00"
-    }
-    topic = Topic.find(1)
-    topic.attributes = attributes
-    assert_equal Time.local(2001, 1, 1, 16, 24, 0), topic.written_on
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(4i)" => "16", "written_on(5i)" => "24", "written_on(6i)" => "00"
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
   end
 
   def test_multiparameter_attributes_on_time_with_invalid_time_params
-    begin
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
       attributes = {
+        "written_on(1i)" => "2004", "written_on(2i)" => "6", "written_on(3i)" => "24",
         "written_on(4i)" => "2004", "written_on(5i)" => "36", "written_on(6i)" => "64",
       }
       topic = Topic.find(1)
       topic.attributes = attributes
-    rescue ActiveRecord::MultiparameterAssignmentErrors => ex
-      assert_equal(1, ex.errors.size)
-      assert_equal("written_on", ex.errors[0].attribute)
     end
+    assert_equal("written_on", ex.errors[0].attribute)
   end
 
   def test_multiparameter_attributes_on_time_with_old_date
@@ -620,6 +611,83 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on
   end
 
+  def test_multiparameter_attributes_on_time_will_raise_on_big_time_if_missing_date_parts
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(4i)" => "16", "written_on(5i)" => "24"
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_attributes_on_time_with_raise_on_small_time_if_missing_date_parts
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      attributes = {
+        "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
+      }
+      topic = Topic.find(1)
+      topic.attributes = attributes
+    end
+    assert_equal("written_on", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_hour_if_missing
+    attributes = {
+      "written_on(1i)" => "2004", "written_on(2i)" => "12", "written_on(3i)" => "12",
+      "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(2004, 12, 12, 0, 12, 2), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_hour_if_blank
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal 1, topic.written_on.year
+    assert_equal 1, topic.written_on.month
+    assert_equal 1, topic.written_on.day
+    assert_equal 0, topic.written_on.hour
+    assert_equal 12, topic.written_on.min
+    assert_equal 2, topic.written_on.sec
+  end
+
+  def test_multiparameter_attributes_on_time_will_ignore_date_if_empty
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "16", "written_on(5i)" => "24"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal 1, topic.written_on.year
+    assert_equal 1, topic.written_on.month
+    assert_equal 1, topic.written_on.day
+    assert_equal 16, topic.written_on.hour
+    assert_equal 24, topic.written_on.min
+    assert_equal 0, topic.written_on.sec
+  end
+
+  def test_multiparameter_attributes_on_time_with_seconds_will_ignore_date_if_empty
+    attributes = {
+      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
+      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal 1, topic.written_on.year
+    assert_equal 1, topic.written_on.month
+    assert_equal 1, topic.written_on.day
+    assert_equal 16, topic.written_on.hour
+    assert_equal 12, topic.written_on.min
+    assert_equal 02, topic.written_on.sec
+  end
+
   def test_multiparameter_assignment_of_aggregation
     customer = Customer.new
     address = Address.new("The Street", "The City", "The Country")
@@ -628,6 +696,42 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal address, customer.address
   end
 
+  def test_multiparameter_assignment_of_aggregation_out_of_order
+    customer = Customer.new
+    address = Address.new("The Street", "The City", "The Country")
+    attributes = { "address(3)" => address.country, "address(2)" => address.city, "address(1)" => address.street }
+    customer.attributes = attributes
+    assert_equal address, customer.address
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_missing_values
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      customer = Customer.new
+      address = Address.new("The Street", "The City", "The Country")
+      attributes = { "address(2)" => address.city, "address(3)" => address.country }
+      customer.attributes = attributes
+    end
+    assert_equal("address", ex.errors[0].attribute)
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_blank_values
+    customer = Customer.new
+    address = Address.new("The Street", "The City", "The Country")
+    attributes = { "address(1)" => "", "address(2)" => address.city, "address(3)" => address.country }
+    customer.attributes = attributes
+    assert_equal Address.new(nil, "The City", "The Country"), customer.address
+  end
+
+  def test_multiparameter_assignment_of_aggregation_with_large_index
+    ex = assert_raise(ActiveRecord::MultiparameterAssignmentErrors) do
+      customer = Customer.new
+      address = Address.new("The Street", "The City", "The Country")
+      attributes = { "address(1)" => "The Street", "address(2)" => address.city, "address(3000)" => address.country }
+      customer.attributes = attributes
+    end
+    assert_equal("address", ex.errors[0].attribute)
+  end
+
   def test_attributes_on_dummy_time
     # Oracle, and Sybase do not have a TIME datatype.
     return true if current_adapter?(:OracleAdapter, :SybaseAdapter)
-- 
1.7.1

time_select_comments.diff

From d904c7817f385a0a08c5643279effe6466ece507 Mon Sep 17 00:00:00 2001
From: Aditya Sanghi <[email protected]>
Date: Sat, 9 Oct 2010 21:23:11 +0530
Subject: [PATCH] remove redundant examples add note about use of :ignore_date option in time_select

---
 actionpack/lib/action_view/helpers/date_helper.rb |   17 ++---------------
 1 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/actionpack/lib/action_view/helpers/date_helper.rb b/actionpack/lib/action_view/helpers/date_helper.rb
index 3aee4fb..36c7514 100644
--- a/actionpack/lib/action_view/helpers/date_helper.rb
+++ b/actionpack/lib/action_view/helpers/date_helper.rb
@@ -207,7 +207,8 @@ module ActionView
       # +object+). You can include the seconds with <tt>:include_seconds</tt>.
       #
       # This method will also generate 3 input hidden tags, for the actual year, month and day unless the option
-      # <tt>:ignore_date</tt> is set to +true+.
+      # <tt>:ignore_date</tt> is set to +true+. If this option is used, it is imperative that a date_select
+      # on the same attribute is also included in the form.
       #
       # If anything is passed in the html_options hash it will be applied to every select tag in the set.
       #
@@ -215,21 +216,10 @@ module ActionView
       #   # Creates a time select tag that, when POSTed, will be stored in the post variable in the sunrise attribute
       #   time_select("post", "sunrise")
       #
-      #   # Creates a time select tag that, when POSTed, will be stored in the order variable in the submitted
-      #   # attribute
-      #   time_select("order", "submitted")
-      #
-      #   # Creates a time select tag that, when POSTed, will be stored in the mail variable in the sent_at attribute
-      #   time_select("mail", "sent_at")
-      #
       #   # Creates a time select tag with a seconds field that, when POSTed, will be stored in the post variables in
       #   # the sunrise attribute.
       #   time_select("post", "start_time", :include_seconds => true)
       #
-      #   # Creates a time select tag with a seconds field that, when POSTed, will be stored in the entry variables in
-      #   # the submission_time attribute.
-      #   time_select("entry", "submission_time", :include_seconds => true)
-      #
       #   # You can set the :minute_step to 15 which will give you: 00, 15, 30 and 45.
       #   time_select 'game', 'game_time', {:minute_step => 15}
       #
@@ -239,9 +229,6 @@ module ActionView
       #   time_select("post", "written_on", :prompt => true) # generic prompts for all
       #
       # The selects are prepared for multi-parameter assignment to an Active Record object.
-      #
-      # Note: If the day is not included as an option but the month is, the day will be set to the 1st to ensure that
-      # all month choices are valid.
       def time_select(object_name, method, options = {}, html_options = {})
         InstanceTag.new(object_name, method, self, options.delete(:object)).to_time_select_tag(options, html_options)
       end
-- 
1.7.1

time_select_comments2.diff

From d904c7817f385a0a08c5643279effe6466ece507 Mon Sep 17 00:00:00 2001
From: Aditya Sanghi <[email protected]>
Date: Sat, 9 Oct 2010 21:23:11 +0530
Subject: [PATCH 1/2] remove redundant examples add note about use of :ignore_date option in time_select

---
 actionpack/lib/action_view/helpers/date_helper.rb |   17 ++---------------
 1 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/actionpack/lib/action_view/helpers/date_helper.rb b/actionpack/lib/action_view/helpers/date_helper.rb
index 3aee4fb..36c7514 100644
--- a/actionpack/lib/action_view/helpers/date_helper.rb
+++ b/actionpack/lib/action_view/helpers/date_helper.rb
@@ -207,7 +207,8 @@ module ActionView
       # +object+). You can include the seconds with <tt>:include_seconds</tt>.
       #
       # This method will also generate 3 input hidden tags, for the actual year, month and day unless the option
-      # <tt>:ignore_date</tt> is set to +true+.
+      # <tt>:ignore_date</tt> is set to +true+. If this option is used, it is imperative that a date_select
+      # on the same attribute is also included in the form.
       #
       # If anything is passed in the html_options hash it will be applied to every select tag in the set.
       #
@@ -215,21 +216,10 @@ module ActionView
       #   # Creates a time select tag that, when POSTed, will be stored in the post variable in the sunrise attribute
       #   time_select("post", "sunrise")
       #
-      #   # Creates a time select tag that, when POSTed, will be stored in the order variable in the submitted
-      #   # attribute
-      #   time_select("order", "submitted")
-      #
-      #   # Creates a time select tag that, when POSTed, will be stored in the mail variable in the sent_at attribute
-      #   time_select("mail", "sent_at")
-      #
       #   # Creates a time select tag with a seconds field that, when POSTed, will be stored in the post variables in
       #   # the sunrise attribute.
       #   time_select("post", "start_time", :include_seconds => true)
       #
-      #   # Creates a time select tag with a seconds field that, when POSTed, will be stored in the entry variables in
-      #   # the submission_time attribute.
-      #   time_select("entry", "submission_time", :include_seconds => true)
-      #
       #   # You can set the :minute_step to 15 which will give you: 00, 15, 30 and 45.
       #   time_select 'game', 'game_time', {:minute_step => 15}
       #
@@ -239,9 +229,6 @@ module ActionView
       #   time_select("post", "written_on", :prompt => true) # generic prompts for all
       #
       # The selects are prepared for multi-parameter assignment to an Active Record object.
-      #
-      # Note: If the day is not included as an option but the month is, the day will be set to the 1st to ensure that
-      # all month choices are valid.
       def time_select(object_name, method, options = {}, html_options = {})
         InstanceTag.new(object_name, method, self, options.delete(:object)).to_time_select_tag(options, html_options)
       end
-- 
1.7.1


From 3b45e59f8a24b85f9680a1ac064d8ec203681cf0 Mon Sep 17 00:00:00 2001
From: Aditya Sanghi <[email protected]>
Date: Sun, 10 Oct 2010 17:52:36 +0530
Subject: [PATCH 2/2] Increasing documentation coverage around :ignore_date on time_select

---
 actionpack/lib/action_view/helpers/date_helper.rb |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/actionpack/lib/action_view/helpers/date_helper.rb b/actionpack/lib/action_view/helpers/date_helper.rb
index 36c7514..3d03d47 100644
--- a/actionpack/lib/action_view/helpers/date_helper.rb
+++ b/actionpack/lib/action_view/helpers/date_helper.rb
@@ -206,9 +206,12 @@ module ActionView
       # specified time-based attribute (identified by +method+) on an object assigned to the template (identified by
       # +object+). You can include the seconds with <tt>:include_seconds</tt>.
       #
-      # This method will also generate 3 input hidden tags, for the actual year, month and day unless the option
-      # <tt>:ignore_date</tt> is set to +true+. If this option is used, it is imperative that a date_select
-      # on the same attribute is also included in the form.
+      # This method will also generate 3 input hidden tags, for the actual year, month and day to be used for the date component of the time.
+      #
+      # If you want to split the date and time input in two different input fields and keep them associated with the same model
+      # attribute, provide the <tt>:ignore_date => true</tt> option to prevent the generation of the hidden date component fields.
+      # Warning: If you miss creating a date_select on the same attribute while using <tt>:ignore_date => true</tt> on time_select,
+      # the time input parameters would be parsed as date, potentially causing errors.
       #
       # If anything is passed in the html_options hash it will be applied to every select tag in the set.
       #
@@ -228,6 +231,10 @@ module ActionView
       #   time_select("post", "written_on", :prompt => {:hour => true}) # generic prompt for hours
       #   time_select("post", "written_on", :prompt => true) # generic prompts for all
       #
+      #   # Creates separate date and time inputs on the same written_on attribute.
+      #   date_select("post", "written_on")
+      #   time_select("post", "written_on", :ignore_date => true)
+      #
       # The selects are prepared for multi-parameter assignment to an Active Record object.
       def time_select(object_name, method, options = {}, html_options = {})
         InstanceTag.new(object_name, method, self, options.delete(:object)).to_time_select_tag(options, html_options)
-- 
1.7.1

time_select_failing_test.diff

From e4d05e1066b4a9944758578ce8a9c4391d7ddcdb Mon Sep 17 00:00:00 2001
From: Aditya Sanghi <[email protected]>
Date: Sat, 9 Oct 2010 01:19:31 +0530
Subject: [PATCH] add failing tests for time_select

---
 activerecord/test/cases/base_test.rb |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 16fd9a7..c1a1d2e 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -598,6 +598,24 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on
   end
 
+  def test_multiparameter_attributes_on_time_with_ignore_date_big_time
+    attributes = {
+      "written_on(4i)" => "16", "written_on(5i)" => "24"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(Date.today.year, Date.today.month, Date.today.day, 16, 24), topic.written_on
+  end
+
+  def test_multiparameter_attributes_on_time_with_ignore_date_small_time
+    attributes = {
+      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
+    }
+    topic = Topic.find(1)
+    topic.attributes = attributes
+    assert_equal Time.local(Date.today.year, Date.today.month, Date.today.day, 16, 12, 02), topic.written_on
+  end
+
   def test_multiparameter_assignment_of_aggregation
     customer = Customer.new
     address = Address.new("The Street", "The City", "The Country")
-- 
1.7.1