Merge pull request #200 from maxlinc/core_types

Improved coercion: primitives and error handling

5 files changed, 381 insertions, 12 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8772ab9..412221d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@
 * [#201](https://github.com/intridea/hashie/pull/201): Hashie::Trash transforms can be inherited - [@fobocaster](https://github.com/fobocaster).
 * [#189](https://github.com/intridea/hashie/pull/189): Added Rash#fetch - [@medcat](https://github.com/medcat).
 * Your contribution here.
+* [#200](https://github.com/intridea/hashie/pull/200): Improved coercion: primitives and error handling - [@maxlinc](https://github.com/maxlinc).
 
 ## 3.2.0 (7/10/2014)
 
diff --git a/README.md b/README.md
index 246867e..631ff6d 100644
--- a/README.md
+++ b/README.md
@@ -96,6 +96,47 @@ tweet.relations.class # => Hash
 #    and Relation.new on each value since Relation doesn't define the `coerce` class method
 ```
 
+### Coercing Core Types
+
+Hashie handles coercion to the following by using standard conversion methods:
+
+| type     | method   |
+|----------|----------|
+| Integer  | `#to_i`  |
+| Float    | `#to_f`  |
+| Complex  | `#to_c`  |
+| Rational | `#to_r`  |
+| String   | `#to_s`  |
+| Symbol   | `#to_sym`|
+
+**Note**: The standard Ruby conversion methods are less strict than you may assume. For example, `:foo.to_i` raises an error but `"foo".to_i` returns 0.
+
+You can also use coerce from the following supertypes with `coerce_value`:
+- Integer
+- Numeric
+
+Hashie does not have built-in support for coercion boolean values, since Ruby does not have a built-in boolean type or standard method for to a boolean. You can coerce to booleans using a custom proc.
+
+### Coercion Proc
+
+You can use a custom coercion proc on either `#coerce_key` or `#coerce_value`. This is useful for coercing to booleans or other simple types without creating a new class and `coerce` method. For example:
+
+```ruby
+class Tweet < Hash
+  include Hashie::Extensions::Coercion
+  coerce_key :retweeted, ->(v) do
+    case v
+    when String
+      return !!(v =~ /^(true|t|yes|y|1)$/i)
+    when Numeric
+      return !v.to_i.zero?
+    else
+      return v == true
+    end
+  end
+end
+```
+
 ### KeyConversion
 
 The KeyConversion extension gives you the convenience methods of `symbolize_keys` and `stringify_keys` along with their bang counterparts. You can also include just stringify or just symbolize with `Hashie::Extensions::StringifyKeys` or `Hashie::Extensions::SymbolizeKeys`.
diff --git a/UPGRADING.md b/UPGRADING.md
index 611d307..25e8a84 100644
--- a/UPGRADING.md
+++ b/UPGRADING.md
@@ -1,6 +1,22 @@
 Upgrading Hashie
 ================
 
+### Upgrading to 3.2.1
+
+#### Possible coercion changes
+
+The improvements made to coercions in version 3.2.1 [issue #200](https://github.com/intridea/hashie/pull/200) do not break the documented API, but are significant enough that changes may effect undocumented side-effects. Applications that depended on those side-effects will need to be updated.
+
+**Change**: Type coercion no longer creates new objects if the input matches the target type. Previously coerced properties always resulted in the creation of a new object, even when it wasn't necessary. This had the effect of a `dup` or `clone` on coerced properties but not uncoerced ones.
+
+If necessary, `dup` or `clone` your own objects. Do not assume Hashie will do it for you.
+
+**Change**: Failed coercion attempts now raise Hashie::CoercionError.
+
+Hashie now raises a Hashie::CoercionError that details on the property that could not be coerced, the source and target type of the coercion, and the internal error. Previously only the internal error was raised.
+
+Applications that were attempting to rescuing the internal errors should be updated to rescue Hashie::CoercionError instead.
+
 ### Upgrading to 3.0
 
 #### Compatibility with Rails 4 Strong Parameters
diff --git a/lib/hashie/extensions/coercion.rb b/lib/hashie/extensions/coercion.rb
index dea6e92..c78b29e 100644
--- a/lib/hashie/extensions/coercion.rb
+++ b/lib/hashie/extensions/coercion.rb
@@ -1,6 +1,22 @@
 module Hashie
+  class CoercionError < StandardError; end
+
   module Extensions
     module Coercion
+      CORE_TYPES = {
+        Integer    => :to_i,
+        Float      => :to_f,
+        Complex    => :to_c,
+        Rational   => :to_r,
+        String     => :to_s,
+        Symbol     => :to_sym
+      }
+
+      ABSTRACT_CORE_TYPES = {
+        Integer => [Fixnum, Bignum],
+        Numeric => [Fixnum, Bignum, Float, Complex, Rational]
+      }
+
       def self.included(base)
         base.send :include, InstanceMethods
         base.extend ClassMethods # NOTE: we wanna make sure we first define set_value_with_coercion before extending
@@ -13,23 +29,47 @@ module Hashie
         def set_value_with_coercion(key, value)
           into = self.class.key_coercion(key) || self.class.value_coercion(value)
 
-          return set_value_without_coercion(key, value) unless value && into
-          return set_value_without_coercion(key, coerce_or_init(into).call(value)) unless into.is_a?(Enumerable)
+          return set_value_without_coercion(key, value) if value.nil? || into.nil?
 
-          if into.class >= Hash
-            key_coerce = coerce_or_init(into.flatten[0])
-            value_coerce = coerce_or_init(into.flatten[-1])
-            value = Hash[value.map { |k, v| [key_coerce.call(k), value_coerce.call(v)] }]
-          else # Enumerable but not Hash: Array, Set
-            value_coerce = coerce_or_init(into.first)
-            value = into.class.new(value.map { |v| value_coerce.call(v) })
+          begin
+            return set_value_without_coercion(key, coerce_or_init(into).call(value)) unless into.is_a?(Enumerable)
+
+            if into.class >= Hash
+              key_coerce = coerce_or_init(into.flatten[0])
+              value_coerce = coerce_or_init(into.flatten[-1])
+              value = Hash[value.map { |k, v| [key_coerce.call(k), value_coerce.call(v)] }]
+            else # Enumerable but not Hash: Array, Set
+              value_coerce = coerce_or_init(into.first)
+              value = into.class.new(value.map { |v| value_coerce.call(v) })
+            end
+          rescue NoMethodError, TypeError => e
+            raise CoercionError, "Cannot coerce property #{key.inspect} from #{value.class} to #{into}: #{e.message}"
           end
 
           set_value_without_coercion(key, value)
         end
 
         def coerce_or_init(type)
-          type.respond_to?(:coerce) ? ->(v) { type.coerce(v) } : ->(v) { type.new(v) }
+          return type if type.is_a? Proc
+
+          if CORE_TYPES.key?(type)
+            ->(v) do
+              return v if v.is_a? type
+              return v.send(CORE_TYPES[type])
+            end
+          elsif type.respond_to?(:coerce)
+            ->(v) do
+              return v if v.is_a? type
+              type.coerce(v)
+            end
+          elsif type.respond_to?(:new)
+            ->(v) do
+              return v if v.is_a? type
+              type.new(v)
+            end
+          else
+            fail TypeError, "#{type} is not a coercable type"
+          end
         end
 
         private :coerce_or_init
@@ -102,6 +142,12 @@ module Hashie
         def coerce_value(from, into, options = {})
           options = { strict: true }.merge(options)
 
+          if ABSTRACT_CORE_TYPES.key? from
+            ABSTRACT_CORE_TYPES[from].each do | type |
+              coerce_value type, into, options
+            end
+          end
+
           if options[:strict]
             (@strict_value_coercions ||= {})[from] = into
           else
diff --git a/spec/hashie/extensions/coercion_spec.rb b/spec/hashie/extensions/coercion_spec.rb
index 466c884..2819eea 100644
--- a/spec/hashie/extensions/coercion_spec.rb
+++ b/spec/hashie/extensions/coercion_spec.rb
@@ -1,16 +1,20 @@
 require 'spec_helper'
 
 describe Hashie::Extensions::Coercion do
+  class NotInitializable
+    private_class_method :new
+  end
+
   class Initializable
     attr_reader :coerced, :value
 
-    def initialize(obj, coerced = false)
+    def initialize(obj, coerced = nil)
       @coerced = coerced
       @value = obj.class.to_s
     end
 
     def coerced?
-      !!@coerced
+      !@coerced.nil?
     end
   end
 
@@ -32,6 +36,65 @@ describe Hashie::Extensions::Coercion do
   let(:instance) { subject.new }
 
   describe '#coerce_key' do
+    context 'nesting' do
+      class BaseCoercableHash < Hash
+        include Hashie::Extensions::Coercion
+        include Hashie::Extensions::MergeInitializer
+      end
+
+      class NestedCoercableHash < BaseCoercableHash
+        coerce_key :foo, String
+        coerce_key :bar, Integer
+      end
+
+      class RootCoercableHash < BaseCoercableHash
+        coerce_key :nested, NestedCoercableHash
+        coerce_key :nested_list, Array[NestedCoercableHash]
+        coerce_key :nested_hash, Hash[String => NestedCoercableHash]
+      end
+
+      def test_nested_object(obj)
+        expect(obj).to be_a(NestedCoercableHash)
+        expect(obj[:foo]).to be_a(String)
+        expect(obj[:bar]).to be_an(Integer)
+      end
+
+      subject { RootCoercableHash }
+      let(:instance) { subject.new }
+
+      it 'coeces nested objects' do
+        instance[:nested] = { foo: 123, bar: '456' }
+        test_nested_object(instance[:nested])
+      end
+
+      it 'coeces nested arrays' do
+        instance[:nested_list] = [
+          { foo: 123, bar: '456' },
+          { foo: 234, bar: '567' },
+          { foo: 345, bar: '678' }
+        ]
+        expect(instance[:nested_list]).to be_a Array
+        expect(instance[:nested_list].size).to eq(3)
+        instance[:nested_list].each do | nested |
+          test_nested_object nested
+        end
+      end
+
+      it 'coeces nested hashes' do
+        instance[:nested_hash] = {
+          a: { foo: 123, bar: '456' },
+          b: { foo: 234, bar: '567' },
+          c: { foo: 345, bar: '678' }
+        }
+        expect(instance[:nested_hash]).to be_a Hash
+        expect(instance[:nested_hash].size).to eq(3)
+        instance[:nested_hash].each do | key, nested |
+          expect(key).to be_a(String)
+          test_nested_object nested
+        end
+      end
+    end
+
     it { expect(subject).to be_respond_to(:coerce_key) }
 
     it 'runs through coerce on a specified key' do
@@ -41,6 +104,13 @@ describe Hashie::Extensions::Coercion do
       expect(instance[:foo]).to be_coerced
     end
 
+    it 'skips unnecessary coercions' do
+      subject.coerce_key :foo, Coercable
+
+      instance[:foo] = Coercable.new('bar')
+      expect(instance[:foo]).to_not be_coerced
+    end
+
     it 'supports an array of keys' do
       subject.coerce_keys :foo, :bar, Coercable
 
@@ -92,6 +162,116 @@ describe Hashie::Extensions::Coercion do
       expect(instance[:foo].keys).to all(be_coerced)
     end
 
+    context 'coercing core types' do
+      def test_coercion(literal, target_type, coerce_method)
+        subject.coerce_key :foo, target_type
+        instance[:foo] = literal
+        expect(instance[:foo]).to be_a(target_type)
+        expect(instance[:foo]).to eq(literal.send(coerce_method))
+      end
+
+      RSpec.shared_examples 'coerces from numeric types' do |target_type, coerce_method|
+        it "coerces from String to #{target_type} via #{coerce_method}" do
+          test_coercion '2.0', target_type, coerce_method
+        end
+
+        it "coerces from Integer to #{target_type} via #{coerce_method}" do
+          # Fixnum
+          test_coercion 2, target_type, coerce_method
+          # Bignum
+          test_coercion 12_345_667_890_987_654_321, target_type, coerce_method
+        end
+
+        it "coerces from Rational to #{target_type} via #{coerce_method}" do
+          test_coercion Rational(2, 3), target_type, coerce_method
+        end
+      end
+
+      RSpec.shared_examples 'coerces from alphabetical types' do |target_type, coerce_method|
+        it "coerces from String to #{target_type} via #{coerce_method}" do
+          test_coercion 'abc', target_type, coerce_method
+        end
+
+        it "coerces from Symbol to #{target_type} via #{coerce_method}" do
+          test_coercion :abc, target_type, coerce_method
+        end
+      end
+
+      include_examples 'coerces from numeric types', Integer, :to_i
+      include_examples 'coerces from numeric types', Float, :to_f
+      include_examples 'coerces from numeric types', String, :to_s
+
+      include_examples 'coerces from alphabetical types', String, :to_s
+      include_examples 'coerces from alphabetical types', Symbol, :to_sym
+
+      it 'can coerce String to Rational when possible' do
+        test_coercion '2/3', Rational, :to_r
+      end
+
+      it 'can coerce String to Complex when possible' do
+        test_coercion '2/3+3/4i', Complex, :to_c
+      end
+
+      it 'coerces collections with core types' do
+        subject.coerce_key :foo, Hash[String => String]
+
+        instance[:foo] = {
+          abc: 123,
+          xyz: 987
+        }
+        expect(instance[:foo]).to eq(
+                                       'abc' => '123',
+                                       'xyz' => '987'
+                                     )
+      end
+
+      it 'can coerce via a proc' do
+        subject.coerce_key :foo, ->(v) do
+          case v
+          when String
+            return !!(v =~ /^(true|t|yes|y|1)$/i)
+          when Numeric
+            return !v.to_i.zero?
+          else
+            return v == true
+          end
+        end
+
+        true_values = [true, 'true', 't', 'yes', 'y', '1', 1, -1]
+        false_values = [false, 'false', 'f', 'no', 'n', '0', 0]
+
+        true_values.each do |v|
+          instance[:foo] = v
+          expect(instance[:foo]).to be_a(TrueClass)
+        end
+        false_values.each do |v|
+          instance[:foo] = v
+          expect(instance[:foo]).to be_a(FalseClass)
+        end
+      end
+
+      it 'raises errors for non-coercable types' do
+        subject.coerce_key :foo, NotInitializable
+        expect { instance[:foo] = 'true' }.to raise_error(Hashie::CoercionError, /NotInitializable is not a coercable type/)
+      end
+
+      it 'can coerce false' do
+        subject.coerce_key :foo, Coercable
+
+        instance[:foo] = false
+        expect(instance[:foo]).to be_coerced
+        expect(instance[:foo].value).to eq('FalseClass')
+      end
+
+      it 'does not coerce nil' do
+        subject.coerce_key :foo, String
+
+        instance[:foo] = nil
+        expect(instance[:foo]).to_not eq('')
+        expect(instance[:foo]).to be_nil
+      end
+    end
+
     it 'calls #new if no coerce method is available' do
       subject.coerce_key :foo, Initializable
 
@@ -239,6 +419,91 @@ describe Hashie::Extensions::Coercion do
         expect(instance[:foo]).to be_kind_of(Coercable)
       end
     end
+
+    context 'core types' do
+      it 'coerces String to Integer when possible' do
+        subject.coerce_value String, Integer
+
+        instance[:foo] = '2'
+        instance[:bar] = '2.7'
+        instance[:hi] = 'hi'
+        expect(instance[:foo]).to be_a(Integer)
+        expect(instance[:foo]).to eq(2)
+        expect(instance[:bar]).to be_a(Integer)
+        expect(instance[:bar]).to eq(2)
+        expect(instance[:hi]).to be_a(Integer)
+        expect(instance[:hi]).to eq(0) # not what I expected...
+      end
+
+      it 'coerces non-numeric from String to Integer' do
+        # This was surprising, but I guess it's "correct"
+        # unless there is a stricter `to_i` alternative
+        subject.coerce_value String, Integer
+        instance[:hi] = 'hi'
+        expect(instance[:hi]).to be_a(Integer)
+        expect(instance[:hi]).to eq(0)
+      end
+
+      it 'raises a CoercionError when coercion is not possible' do
+        subject.coerce_value Fixnum, Symbol
+        expect { instance[:hi] = 1 }.to raise_error(Hashie::CoercionError, /Cannot coerce property :hi from Fixnum to Symbol/)
+      end
+
+      it 'coerces Integer to String' do
+        subject.coerce_value Integer, String
+
+        {
+          fixnum: 2,
+          bignum: 12_345_667_890_987_654_321,
+          float: 2.7,
+          rational: Rational(2, 3),
+          complex: Complex(1)
+        }.each do | k, v |
+          instance[k] = v
+          if v.is_a? Integer
+            expect(instance[k]).to be_a(String)
+            expect(instance[k]).to eq(v.to_s)
+          else
+            expect(instance[k]).to_not be_a(String)
+            expect(instance[k]).to eq(v)
+          end
+        end
+      end
+
+      it 'coerces Numeric to String' do
+        subject.coerce_value Numeric, String
+
+        {
+          fixnum: 2,
+          bignum: 12_345_667_890_987_654_321,
+          float: 2.7,
+          rational: Rational(2, 3),
+          complex: Complex(1)
+        }.each do | k, v |
+          instance[k] = v
+          expect(instance[k]).to be_a(String)
+          expect(instance[k]).to eq(v.to_s)
+        end
+      end
+
+      it 'can coerce via a proc' do
+        subject.coerce_value String, ->(v) do
+          return !!(v =~ /^(true|t|yes|y|1)$/i)
+        end
+
+        true_values = %w(true t yes y 1)
+        false_values = %w(false f no n 0)
+
+        true_values.each do |v|
+          instance[:foo] = v
+          expect(instance[:foo]).to be_a(TrueClass)
+        end
+        false_values.each do |v|
+          instance[:foo] = v
+          expect(instance[:foo]).to be_a(FalseClass)
+        end
+      end
+    end
   end
 
   after(:each) do