From a686efda7cff298952aa687d0675c0b514cfdfe4 Mon Sep 17 00:00:00 2001 From: Joe Faber Date: Thu, 13 Apr 2017 13:46:29 -0400 Subject: [PATCH 1/3] Bugfix: values without value breaks type and default --- CHANGELOG.md | 1 + lib/grape/validations/params_scope.rb | 13 +++- .../validations/validators/values_spec.rb | 59 +++++++++++++++++++ 3 files changed, 71 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b0df90f5c..60a1eefe30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ #### Fixes +* [#1590](https://github.com/ruby-grape/grape/pull/1590): 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) diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index 5b57431804..45838bf2ce 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -232,16 +232,25 @@ 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] + values = validations[:values] + if values.is_a? Hash + excepts = values[:except] + values = values[:value] + values = nil if values.is_a? Proc + end + doc_attrs[:values] = values if values - coerce_type = guess_coerce_type(coerce_type, values) + # use values or excepts to guess coerce type when stated type is Array + coerce_type = guess_coerce_type(coerce_type, values || 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) diff --git a/spec/grape/validations/validators/values_spec.rb b/spec/grape/validations/validators/values_spec.rb index 47d75b8026..76c896b3e3 100644 --- a/spec/grape/validations/validators/values_spec.rb +++ b/spec/grape/validations/validators/values_spec.rb @@ -75,6 +75,13 @@ 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 @@ -82,6 +89,13 @@ class API < Grape::API { 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,6 +157,13 @@ 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 @@ -150,6 +171,13 @@ class API < Grape::API { 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 From 9b8c6c5029173dfd281a14a2f549a49054662d4b Mon Sep 17 00:00:00 2001 From: Joe Faber Date: Thu, 13 Apr 2017 14:21:30 -0400 Subject: [PATCH 2/3] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60a1eefe30..1a171f9ce3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ #### Fixes -* [#1590](https://github.com/ruby-grape/grape/pull/1590): Fix default and type validator when values is a Hash with no value attribute - [@jlfaber](https://github.com/jlfaber). +* [#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) From 8ef7ad2a6caa7fd2abe50cda362f5cc32b44dec3 Mon Sep 17 00:00:00 2001 From: Joe Faber Date: Fri, 14 Apr 2017 10:34:15 -0400 Subject: [PATCH 3/3] Code Review Feedback --- lib/grape/validations/params_scope.rb | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index 45838bf2ce..c3e5cf3731 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -232,17 +232,20 @@ def validates(attrs, validations) default = validations[:default] doc_attrs[:default] = default if validations.key?(:default) - values = validations[:values] - if values.is_a? Hash - excepts = values[:except] - values = values[:value] - values = nil if values.is_a? Proc + 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 || excepts) + 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) @@ -380,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