From 59043a3a278d71755223ed8eef09171e2a74b9df Mon Sep 17 00:00:00 2001 From: Aditya Sanghi 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