Bugfix: values without value breaks type and default (#1615)

* Bugfix: values without value breaks type and default

* Update CHANGELOG.md

* Code Review Feedback

CHANGELOG.md

@@ -6,6 +6,7 @@
 
 #### Fixes
 
+* [#1615](https://github.com/ruby-grape/grape/pull/1615): Fix default and type validator when values is a Hash with no value attribute - [@jlfaber](https://github.com/jlfaber).
 * Your contribution here.
 
 ### 0.19.2 (4/12/2017)

lib/grape/validations/params_scope.rb

@@ -232,16 +232,28 @@ def validates(attrs, validations)
         default = validations[:default]
         doc_attrs[:default] = default if validations.key?(:default)
 
-        values = options_key?(:values, :value, validations) ? validations[:values][:value] : validations[:values]
+        if (values_hash = validations[:values]).is_a? Hash
+          values = values_hash[:value]
+          excepts = values_hash[:except]
+        else
+          values = validations[:values]
+        end
         doc_attrs[:values] = values if values
 
+        # NB. values and excepts should be nil, Proc, Array, or Range.
+        # Specifically, values should NOT be a Hash
+
+        # use values or excepts to guess coerce type when stated type is Array
         coerce_type = guess_coerce_type(coerce_type, values)
+        coerce_type = guess_coerce_type(coerce_type, excepts)
 
         # default value should be present in values array, if both exist and are not procs
         check_incompatible_option_values(values, default)
 
         # type should be compatible with values array, if both exist
         validate_value_coercion(coerce_type, values)
+        # type should be compatible with excepts array, if both exist
+        validate_value_coercion(coerce_type, excepts)
 
         doc_attrs[:documentation] = validations.delete(:documentation) if validations.key?(:documentation)
 
@@ -371,10 +383,6 @@ def validate(type, options, attrs, doc_attrs, opts)
       def validate_value_coercion(coerce_type, values)
         return unless coerce_type && values
         return if values.is_a?(Proc)
-        if values.is_a?(Hash)
-          return if values[:value] && values[:value].is_a?(Proc)
-          return if values[:except] && values[:except].is_a?(Proc)
-        end
         coerce_type = coerce_type.first if coerce_type.is_a?(Array)
         value_types = values.is_a?(Range) ? [values.begin, values.end] : values
         if coerce_type == Virtus::Attribute::Boolean

spec/grape/validations/validators/values_spec.rb

@@ -75,13 +75,27 @@ class API < Grape::API
         end
         get '/empty'
 
+        params do
+          optional :type, values: { value: ValuesModel.values }, default: 'valid-type2'
+        end
+        get '/default/hash/valid' do
+          { type: params[:type] }
+        end
+
         params do
           optional :type, values: ValuesModel.values, default: 'valid-type2'
         end
         get '/default/valid' do
           { type: params[:type] }
         end
 
+        params do
+          optional :type, values: { except: ValuesModel.excepts }, default: 'valid-type2'
+        end
+        get '/default/except' do
+          { type: params[:type] }
+        end
+
         params do
           optional :type, values: -> { ValuesModel.values }, default: 'valid-type2'
         end
@@ -143,13 +157,27 @@ class API < Grape::API
           { type: params[:type] }
         end
 
+        params do
+          requires :type, type: String, values: { except: ValuesModel.excepts }
+        end
+        get '/except/exclusive/type' do
+          { type: params[:type] }
+        end
+
         params do
           requires :type, values: { except: -> { ValuesModel.excepts } }
         end
         get '/except/exclusive/lambda' do
           { type: params[:type] }
         end
 
+        params do
+          requires :type, type: String, values: { except: -> { ValuesModel.excepts } }
+        end
+        get '/except/exclusive/lambda/type' do
+          { type: params[:type] }
+        end
+
         params do
           requires :type, type: Integer, values: { except: -> { [3, 4, 5] } }
         end
@@ -282,6 +310,18 @@ def app
     expect(last_response.body).to eq({ type: 'valid-type2' }.to_json)
   end
 
+  it 'allows a default value with except' do
+    get('/default/except')
+    expect(last_response.status).to eq 200
+    expect(last_response.body).to eq({ type: 'valid-type2' }.to_json)
+  end
+
+  it 'allows a valid default value' do
+    get('/default/hash/valid')
+    expect(last_response.status).to eq 200
+    expect(last_response.body).to eq({ type: 'valid-type2' }.to_json)
+  end
+
   it 'allows a proc for values' do
     get('/lambda', type: 'valid-type1')
     expect(last_response.status).to eq 200
@@ -371,6 +411,13 @@ def app
     end.to raise_error Grape::Exceptions::IncompatibleOptionValues
   end
 
+  it 'raises IncompatibleOptionValues when except contains a value that is not a kind of the type' do
+    subject = Class.new(Grape::API)
+    expect do
+      subject.params { requires :type, values: { except: [10.5, 11] }, type: Integer }
+    end.to raise_error Grape::Exceptions::IncompatibleOptionValues
+  end
+
   context 'with a lambda values' do
     subject do
       Class.new(Grape::API) do
@@ -451,6 +498,12 @@ def app
       expect(last_response.body).to eq({ type: 'value' }.to_json)
     end
 
+    it 'allows any other value outside excepts when type is included' do
+      get '/except/exclusive/type', type: 'value'
+      expect(last_response.status).to eq 200
+      expect(last_response.body).to eq({ type: 'value' }.to_json)
+    end
+
     it 'rejects values that matches except' do
       get '/except/exclusive', type: 'invalid-type1'
       expect(last_response.status).to eq 400
@@ -465,6 +518,12 @@ def app
   end
 
   context 'exclusive excepts with lambda' do
+    it 'allows any other value outside excepts when type is included' do
+      get '/except/exclusive/lambda/type', type: 'value'
+      expect(last_response.status).to eq 200
+      expect(last_response.body).to eq({ type: 'value' }.to_json)
+    end
+
     it 'allows any other value outside excepts' do
       get '/except/exclusive/lambda', type: 'value'
       expect(last_response.status).to eq 200