Avoid coercion of a value if it is valid (#1686)

* don't coerce param if it is valid

* still run any custom coersions

* fix enumerable type check

* one more nil check

* use coercer class instead of respond_to

* style

* add tests

* changelog

* coerce if method exists and is not json

* rename to requires_coercion?

* add spec

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 #### Features
 
-* Your contribution here.
+* [#1686](https://github.com/ruby-grape/grape/pull/1686): Avoid coercion of a value if it is valid - [@timothysu](https://github.com/timothysu).
 
 #### Fixes
 

lib/grape/validations/types/custom_type_coercer.rb

@@ -137,7 +137,7 @@ def infer_type_check(type)
             # passed, or if the type also implements a parse() method.
             type
           elsif type.is_a?(Enumerable)
-            ->(value) { value.all? { |item| item.is_a? type[0] } }
+            ->(value) { value.respond_to?(:all?) && value.all? { |item| item.is_a? type[0] } }
           else
             # By default, do a simple type check
             ->(value) { value.is_a? type }

lib/grape/validations/validators/coerce.rb

@@ -16,6 +16,7 @@ def validate(request)
 
       def validate_param!(attr_name, params)
         raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:coerce) unless params.is_a? Hash
+        return unless requires_coercion?(params[attr_name])
         new_value = coerce_value(params[attr_name])
         raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:coerce) unless valid_type?(new_value)
         params[attr_name] = new_value
@@ -63,6 +64,11 @@ def coerce_value(val)
       def type
         @option[:type].is_a?(Hash) ? @option[:type][:value] : @option[:type]
       end
+
+      def requires_coercion?(value)
+        # JSON types do not require coercion if value is valid
+        !valid_type?(value) || converter.coercer.respond_to?(:method) && !converter.is_a?(Grape::Validations::Types::Json)
+      end
     end
   end
 end

spec/grape/validations/validators/coerce_spec.rb

@@ -419,6 +419,23 @@ class User
         expect(JSON.parse(last_response.body)).to eq([0, 0, 0, 0])
       end
 
+      it 'parses parameters even if type is valid' do
+        subject.params do
+          requires :values, type: Array, coerce_with: ->(array) { array.map { |val| val.to_i + 1 } }
+        end
+        subject.get '/ints' do
+          params[:values]
+        end
+
+        get '/ints', values: [1, 2, 3, 4]
+        expect(last_response.status).to eq(200)
+        expect(JSON.parse(last_response.body)).to eq([2, 3, 4, 5])
+
+        get '/ints', values: %w(a b c d)
+        expect(last_response.status).to eq(200)
+        expect(JSON.parse(last_response.body)).to eq([1, 1, 1, 1])
+      end
+
       it 'uses parse where available' do
         subject.params do
           requires :ints, type: Array, coerce_with: JSON do
@@ -477,7 +494,7 @@ class User
     end
 
     context 'first-class JSON' do
-      it 'parses objects and arrays' do
+      it 'parses objects, hashes, and arrays' do
         subject.params do
           requires :splines, type: JSON do
             requires :x, type: Integer, values: [1, 2, 3]
@@ -499,17 +516,33 @@ class User
         expect(last_response.status).to eq(200)
         expect(last_response.body).to eq('woof')
 
+        get '/', splines: { x: 1, ints: [1, 2, 3], obj: { y: 'woof' } }
+        expect(last_response.status).to eq(200)
+        expect(last_response.body).to eq('woof')
+
         get '/', splines: '[{"x":2,"ints":[]},{"x":3,"ints":[4],"obj":{"y":"quack"}}]'
         expect(last_response.status).to eq(200)
         expect(last_response.body).to eq('arrays work')
 
+        get '/', splines: [{ x: 2, ints: [] }, { x: 3, ints: [4], obj: { y: 'quack' } }]
+        expect(last_response.status).to eq(200)
+        expect(last_response.body).to eq('arrays work')
+
         get '/', splines: '{"x":4,"ints":[2]}'
         expect(last_response.status).to eq(400)
         expect(last_response.body).to eq('splines[x] does not have a valid value')
 
+        get '/', splines: { x: 4, ints: [2] }
+        expect(last_response.status).to eq(400)
+        expect(last_response.body).to eq('splines[x] does not have a valid value')
+
         get '/', splines: '[{"x":1,"ints":[]},{"x":4,"ints":[]}]'
         expect(last_response.status).to eq(400)
         expect(last_response.body).to eq('splines[x] does not have a valid value')
+
+        get '/', splines: [{ x: 1, ints: [] }, { x: 4, ints: [] }]
+        expect(last_response.status).to eq(400)
+        expect(last_response.body).to eq('splines[x] does not have a valid value')
       end
 
       it 'works when declared optional' do