From 5808de3165f74b6915f1e4576bea3c919405e36a Mon Sep 17 00:00:00 2001 From: Timothy Su Date: Fri, 29 Sep 2017 15:06:18 -0400 Subject: [PATCH] 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 +- .../validations/types/custom_type_coercer.rb | 2 +- lib/grape/validations/validators/coerce.rb | 6 ++++ .../validations/validators/coerce_spec.rb | 35 ++++++++++++++++++- 4 files changed, 42 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8756fc1ee3..303c4c0b31 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/grape/validations/types/custom_type_coercer.rb b/lib/grape/validations/types/custom_type_coercer.rb index da28474eb0..8e3ca2194b 100644 --- a/lib/grape/validations/types/custom_type_coercer.rb +++ b/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 } diff --git a/lib/grape/validations/validators/coerce.rb b/lib/grape/validations/validators/coerce.rb index 76db19d47c..4d1a32e954 100644 --- a/lib/grape/validations/validators/coerce.rb +++ b/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 diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index 726e439eb8..9cc34c3e9d 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/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