Added :constructor and :converter options to composed_of and deprecated the conversion block

Signed-off-by: Michael Koziarski <michael@koziarski.com>

4 files changed, 124 insertions, 33 deletions

diff --git a/activerecord/lib/active_record/aggregations.rb b/activerecord/lib/active_record/aggregations.rb
index a5d3a50ef1..f3d73591e5 100644
--- a/activerecord/lib/active_record/aggregations.rb
+++ b/activerecord/lib/active_record/aggregations.rb
@@ -10,10 +10,10 @@ module ActiveRecord
       end unless self.new_record?
     end
 
-    # Active Record implements aggregation through a macro-like class method called +composed_of+ for representing attributes 
+    # Active Record implements aggregation through a macro-like class method called +composed_of+ for representing attributes
     # as value objects. It expresses relationships like "Account [is] composed of Money [among other things]" or "Person [is]
-    # composed of [an] address". Each call to the macro adds a description of how the value objects are created from the 
-    # attributes of the entity object (when the entity is initialized either as a new object or from finding an existing object) 
+    # composed of [an] address". Each call to the macro adds a description of how the value objects are created from the
+    # attributes of the entity object (when the entity is initialized either as a new object or from finding an existing object)
     # and how it can be turned back into attributes (when the entity is saved to the database). Example:
     #
     #   class Customer < ActiveRecord::Base
@@ -30,10 +30,10 @@ module ActiveRecord
     #  class Money
     #    include Comparable
     #    attr_reader :amount, :currency
-    #    EXCHANGE_RATES = { "USD_TO_DKK" => 6 }  
-    # 
-    #    def initialize(amount, currency = "USD") 
-    #      @amount, @currency = amount, currency 
+    #    EXCHANGE_RATES = { "USD_TO_DKK" => 6 }
+    #
+    #    def initialize(amount, currency = "USD")
+    #      @amount, @currency = amount, currency
     #    end
     #
     #    def exchange_to(other_currency)
@@ -56,19 +56,19 @@ module ActiveRecord
     #
     #  class Address
     #    attr_reader :street, :city
-    #    def initialize(street, city) 
-    #      @street, @city = street, city 
+    #    def initialize(street, city)
+    #      @street, @city = street, city
     #    end
     #
-    #    def close_to?(other_address) 
-    #      city == other_address.city 
+    #    def close_to?(other_address)
+    #      city == other_address.city
     #    end
     #
     #    def ==(other_address)
     #      city == other_address.city && street == other_address.street
     #    end
     #  end
-    #  
+    #
     # Now it's possible to access attributes from the database through the value objects instead. If you choose to name the
     # composition the same as the attribute's name, it will be the only way to access that attribute. That's the case with our
     # +balance+ attribute. You interact with the value objects just like you would any other attribute, though:
@@ -87,8 +87,8 @@ module ActiveRecord
     #   customer.address_city   = "Copenhagen"
     #   customer.address        # => Address.new("Hyancintvej", "Copenhagen")
     #   customer.address = Address.new("May Street", "Chicago")
-    #   customer.address_street # => "May Street" 
-    #   customer.address_city   # => "Chicago" 
+    #   customer.address_street # => "May Street"
+    #   customer.address_city   # => "Chicago"
     #
     # == Writing value objects
     #
@@ -103,9 +103,9 @@ module ActiveRecord
     # returns a new value object instead of changing its own values. Active Record won't persist value objects that have been
     # changed through means other than the writer method.
     #
-    # The immutable requirement is enforced by Active Record by freezing any object assigned as a value object. Attempting to 
+    # The immutable requirement is enforced by Active Record by freezing any object assigned as a value object. Attempting to
     # change it afterwards will result in a ActiveSupport::FrozenObjectError.
-    # 
+    #
     # Read more about value objects on http://c2.com/cgi/wiki?ValueObject and on the dangers of not keeping value objects
     # immutable on http://c2.com/cgi/wiki?ValueObjectsShouldBeImmutable
     #
@@ -130,39 +130,59 @@ module ActiveRecord
       # * <tt>:allow_nil</tt> - specifies that the aggregate object will not be instantiated when all mapped
       #   attributes are +nil+.  Setting the aggregate class to +nil+ has the effect of writing +nil+ to all mapped attributes.
       #   This defaults to +false+.
-      #
-      # An optional block can be passed to convert the argument that is passed to the writer method into an instance of
-      # <tt>:class_name</tt>. The block will only be called if the argument is not already an instance of <tt>:class_name</tt>.
+      # * <tt>:constructor</tt> - a symbol specifying the name of the constructor method or a Proc that will be used to convert the
+      #   attributes that are mapped to the aggregation to instantiate a <tt>:class_name</tt> object. The default is +:new+.
+      # * <tt>:converter</tt> - a symbol specifying the name of a class method of <tt>:class_name</tt> or a Proc that will be used to convert
+      #   the argument that is passed to the writer method into an instance of <tt>:class_name</tt>. The converter will only be called
+      #   if the argument is not already an instance of <tt>:class_name</tt>.
       #
       # Option examples:
       #   composed_of :temperature, :mapping => %w(reading celsius)
-      #   composed_of(:balance, :class_name => "Money", :mapping => %w(balance amount)) {|balance| balance.to_money }
+      #   composed_of :balance, :class_name => "Money", :mapping => %w(balance amount), :converter => Proc.new { |balance| balance.to_money }
       #   composed_of :address, :mapping => [ %w(address_street street), %w(address_city city) ]
       #   composed_of :gps_location
       #   composed_of :gps_location, :allow_nil => true
+      #   composed_of :ip_address,
+      #               :class_name => 'IPAddr',
+      #               :mapping => %w(ip to_i),
+      #               :constructor => Proc.new { |ip| IPAddr.new(ip, Socket::AF_INET) },
+      #               :converter => Proc.new { |ip| ip.is_a?(Integer) ? IPAddr.new(ip, Socket::AF_INET) : IPAddr.new(ip.to_s) }
       #
       def composed_of(part_id, options = {}, &block)
-        options.assert_valid_keys(:class_name, :mapping, :allow_nil)
+        options.assert_valid_keys(:class_name, :mapping, :allow_nil, :constructor, :converter)
 
         name        = part_id.id2name
-        class_name  = options[:class_name] || name.camelize
-        mapping     = options[:mapping]    || [ name, name ]
+        class_name  = options[:class_name]  || name.camelize
+        mapping     = options[:mapping]     || [ name, name ]
         mapping     = [ mapping ] unless mapping.first.is_a?(Array)
-        allow_nil   = options[:allow_nil]  || false
+        allow_nil   = options[:allow_nil]   || false
+        constructor = options[:constructor] || :new
+        converter   = options[:converter]   || block
+
+        ActiveSupport::Deprecation.warn('The conversion block has been deprecated, use the :converter option instead.', caller) if block_given?
+
+        reader_method(name, class_name, mapping, allow_nil, constructor)
+        writer_method(name, class_name, mapping, allow_nil, converter)
 
-        reader_method(name, class_name, mapping, allow_nil)
-        writer_method(name, class_name, mapping, allow_nil, block)
-        
         create_reflection(:composed_of, part_id, options, self)
       end
 
       private
-        def reader_method(name, class_name, mapping, allow_nil)
+        def reader_method(name, class_name, mapping, allow_nil, constructor)
           module_eval do
             define_method(name) do |*args|
               force_reload = args.first || false
               if (instance_variable_get("@#{name}").nil? || force_reload) && (!allow_nil || mapping.any? {|pair| !read_attribute(pair.first).nil? })
-                instance_variable_set("@#{name}", class_name.constantize.new(*mapping.collect {|pair| read_attribute(pair.first)}))
+                attrs = mapping.collect {|pair| read_attribute(pair.first)}
+                object = case constructor
+                  when Symbol
+                    class_name.constantize.send(constructor, *attrs)
+                  when Proc, Method
+                    constructor.call(*attrs)
+                  else
+                    raise ArgumentError, 'Constructor must be a symbol denoting the constructor method to call or a Proc to be invoked.'
+                  end
+                instance_variable_set("@#{name}", object)
               end
               instance_variable_get("@#{name}")
             end
@@ -170,14 +190,23 @@ module ActiveRecord
 
         end
 
-        def writer_method(name, class_name, mapping, allow_nil, conversion)
+        def writer_method(name, class_name, mapping, allow_nil, converter)
           module_eval do
             define_method("#{name}=") do |part|
               if part.nil? && allow_nil
                 mapping.each { |pair| self[pair.first] = nil }
                 instance_variable_set("@#{name}", nil)
               else
-                part = conversion.call(part) unless part.is_a?(class_name.constantize) || conversion.nil?
+                unless part.is_a?(class_name.constantize) || converter.nil?
+                  part = case converter
+                    when Symbol
+                     class_name.constantize.send(converter, part)
+                    when Proc, Method
+                      converter.call(part)
+                    else
+                      raise ArgumentError, 'Converter must be a symbol denoting the converter method to call or a Proc to be invoked.'
+                    end
+                end
                 mapping.each { |pair| self[pair.first] = part.send(pair.last) }
                 instance_variable_set("@#{name}", part.freeze)
               end
diff --git a/activerecord/test/cases/aggregations_test.rb b/activerecord/test/cases/aggregations_test.rb
index 75d1f27e07..b6656c8631 100644
--- a/activerecord/test/cases/aggregations_test.rb
+++ b/activerecord/test/cases/aggregations_test.rb
@@ -107,6 +107,41 @@ class AggregationsTest < ActiveRecord::TestCase
     customers(:david).gps_location = nil
     assert_equal nil, customers(:david).gps_location
   end
+
+  def test_custom_constructor
+    assert_equal 'Barney GUMBLE', customers(:barney).fullname.to_s
+    assert_kind_of Fullname, customers(:barney).fullname
+  end
+
+  def test_custom_converter
+    customers(:barney).fullname = 'Barnoit Gumbleau'
+    assert_equal 'Barnoit GUMBLEAU', customers(:barney).fullname.to_s
+    assert_kind_of Fullname, customers(:barney).fullname
+  end
+end
+
+class DeprecatedAggregationsTest < ActiveRecord::TestCase
+  class Person < ActiveRecord::Base; end
+
+  def test_conversion_block_is_deprecated
+    assert_deprecated 'conversion block has been deprecated' do
+      Person.composed_of(:balance, :class_name => "Money", :mapping => %w(balance amount)) { |balance| balance.to_money }
+    end
+  end
+
+  def test_conversion_block_used_when_converter_option_is_nil
+    Person.composed_of(:balance, :class_name => "Money", :mapping => %w(balance amount)) { |balance| balance.to_money }
+    assert_raise(NoMethodError) { Person.new.balance = 5 }
+  end
+
+  def test_converter_option_overrides_conversion_block
+    Person.composed_of(:balance, :class_name => "Money", :mapping => %w(balance amount), :converter => Proc.new { |balance| Money.new(balance) }) { |balance| balance.to_money }
+
+    person = Person.new
+    assert_nothing_raised { person.balance = 5 }
+    assert_equal 5, person.balance.amount
+    assert_kind_of Money, person.balance
+  end
 end
 
 class OverridingAggregationsTest < ActiveRecord::TestCase
diff --git a/activerecord/test/fixtures/customers.yml b/activerecord/test/fixtures/customers.yml
index f802aac5ea..0399ff83b9 100644
--- a/activerecord/test/fixtures/customers.yml
+++ b/activerecord/test/fixtures/customers.yml
@@ -6,7 +6,7 @@ david:
   address_city: Scary Town
   address_country: Loony Land
   gps_location: 35.544623640962634x-105.9309951055148
-  
+
 zaphod:
   id: 2
   name: Zaphod
@@ -14,4 +14,13 @@ zaphod:
   address_street: Avenue Road
   address_city: Hamlet Town
   address_country: Nation Land
+  gps_location: NULL
+
+barney:
+  id: 3
+  name: Barney Gumble
+  balance: 1
+  address_street: Quiet Road
+  address_city: Peaceful Town
+  address_country: Tranquil Land
   gps_location: NULL
\ No newline at end of file
diff --git a/activerecord/test/models/customer.rb b/activerecord/test/models/customer.rb
index 030bbc6237..e258ccdb6c 100644
--- a/activerecord/test/models/customer.rb
+++ b/activerecord/test/models/customer.rb
@@ -1,7 +1,8 @@
 class Customer < ActiveRecord::Base
   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)) { |balance| balance.to_money }
+  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 :fullname, :mapping => %w(name to_s), :constructor => Proc.new { |name| Fullname.parse(name) }, :converter => :parse
 end
 
 class Address
@@ -53,3 +54,20 @@ class GpsLocation
     self.latitude == other.latitude && self.longitude == other.longitude
   end
 end
+
+class Fullname
+  attr_reader :first, :last
+
+  def self.parse(str)
+    return nil unless str
+    new(*str.to_s.split)
+  end
+
+  def initialize(first, last = nil)
+    @first, @last = first, last
+  end
+
+  def to_s
+    "#{first} #{last.upcase}"
+  end
+end
\ No newline at end of file