Merge pull request #6143 from senny/composed_of_converter_returns_nil

allow the :converter Proc form composed_of to return nil

3 files changed, 31 insertions, 7 deletions

diff --git a/activerecord/lib/active_record/aggregations.rb b/activerecord/lib/active_record/aggregations.rb
index c7a329d74d..5de10a8dd6 100644
--- a/activerecord/lib/active_record/aggregations.rb
+++ b/activerecord/lib/active_record/aggregations.rb
@@ -193,7 +193,8 @@ module ActiveRecord
       # * <tt>:converter</tt> - A symbol specifying the name of a class method of <tt>:class_name</tt>
       #   or a Proc that is called when a new value is assigned to the value object. The converter is
       #   passed the single value that is used in the assignment and is only called if the new value is
-      #   not an instance of <tt>:class_name</tt>.
+      #   not an instance of <tt>:class_name</tt>. If <tt>:allow_nil</tt> is set to true, the converter
+      #   can return nil to skip the assignment.
       #
       # Option examples:
       #   composed_of :temperature, :mapping => %w(reading celsius)
@@ -241,16 +242,16 @@ module ActiveRecord
 
         def writer_method(name, class_name, mapping, allow_nil, converter)
           define_method("#{name}=") do |part|
+            unless part.is_a?(class_name.constantize) || converter.nil? || part.nil?
+              part = converter.respond_to?(:call) ?
+                converter.call(part) :
+                class_name.constantize.send(converter, part)
+            end
+
             if part.nil? && allow_nil
               mapping.each { |pair| self[pair.first] = nil }
               @aggregation_cache[name] = nil
             else
-              unless part.is_a?(class_name.constantize) || converter.nil?
-                part = converter.respond_to?(:call) ?
-                  converter.call(part) :
-                  class_name.constantize.send(converter, part)
-              end
-
               mapping.each { |pair| self[pair.first] = part.send(pair.last) }
               @aggregation_cache[name] = part.freeze
             end
diff --git a/activerecord/test/cases/aggregations_test.rb b/activerecord/test/cases/aggregations_test.rb
index 3e0e6dce2c..5bd8f76ba2 100644
--- a/activerecord/test/cases/aggregations_test.rb
+++ b/activerecord/test/cases/aggregations_test.rb
@@ -109,6 +109,24 @@ class AggregationsTest < ActiveRecord::TestCase
     assert_nil customers(:david).gps_location
   end
 
+  def test_nil_return_from_converter_is_respected_when_allow_nil_is_true
+    customers(:david).non_blank_gps_location = ""
+    customers(:david).save
+    customers(:david).reload
+    assert_nil customers(:david).non_blank_gps_location
+  end
+
+  def test_nil_return_from_converter_results_in_failure_when_allow_nil_is_false
+    assert_raises(NoMethodError) do
+      customers(:barney).gps_location = ""
+    end
+  end
+
+  def test_do_not_run_the_converter_when_nil_was_set
+    customers(:david).non_blank_gps_location = nil
+    assert_nil Customer.gps_conversion_was_run
+  end
+
   def test_custom_constructor
     assert_equal 'Barney GUMBLE', customers(:barney).fullname.to_s
     assert_kind_of Fullname, customers(:barney).fullname
diff --git a/activerecord/test/models/customer.rb b/activerecord/test/models/customer.rb
index 777f6b5ba0..807f25b687 100644
--- a/activerecord/test/models/customer.rb
+++ b/activerecord/test/models/customer.rb
@@ -1,7 +1,12 @@
 class Customer < ActiveRecord::Base
+
+  cattr_accessor :gps_conversion_was_run
+
   composed_of :address, :mapping => [ %w(address_street street), %w(address_city city), %w(address_country country) ], :allow_nil => true
   composed_of :balance, :class_name => "Money", :mapping => %w(balance amount), :converter => Proc.new { |balance| balance.to_money }
   composed_of :gps_location, :allow_nil => true
+  composed_of :non_blank_gps_location, :class_name => "GpsLocation", :allow_nil => true, :mapping => %w(gps_location gps_location),
+              :converter => lambda { |gps| self.gps_conversion_was_run = true; gps.blank? ? nil : GpsLocation.new(gps)}
   composed_of :fullname, :mapping => %w(name to_s), :constructor => Proc.new { |name| Fullname.parse(name) }, :converter => :parse
 end