diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 445837988d..e680a59ebf 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,5 +1,5 @@ # This configuration was generated by `rubocop --auto-gen-config` -# on 2014-12-14 14:50:02 -0500 using RuboCop version 0.28.0. +# on 2014-12-16 11:52:50 -0500 using RuboCop version 0.28.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -10,14 +10,14 @@ Lint/UnusedBlockArgument: Enabled: false -# Offense count: 25 +# Offense count: 26 # Cop supports --auto-correct. Lint/UnusedMethodArgument: Enabled: false # Offense count: 35 Metrics/AbcSize: - Max: 51 + Max: 50 # Offense count: 1 Metrics/BlockNesting: @@ -30,9 +30,9 @@ Metrics/ClassLength: # Offense count: 15 Metrics/CyclomaticComplexity: - Max: 21 + Max: 19 -# Offense count: 546 +# Offense count: 545 # Configuration parameters: AllowURI, URISchemes. Metrics/LineLength: Max: 198 diff --git a/CHANGELOG.md b/CHANGELOG.md index 58d39a0a27..826db38001 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ * [#813](https://github.com/intridea/grape/pull/813): Routing methods dsl refactored to get rid of explicit `paths` parameter - [@AlexYankee](https://github.com/AlexYankee). * [#826](https://github.com/intridea/grape/pull/826): Find `coerce_type` for `Array` when not specified - [@manovotn](https://github.com/manovotn). * [#645](https://github.com/intridea/grape/issues/645): Invoking `body false` will return `204 No Content` - [@dblock](https://github.com/dblock). -* [#801](https://github.com/intridea/grape/issues/801): Evaluate permitted parameter `values` lazily on each request when declared as a proc - [@dblock](https://github.com/dblock). +* [#801](https://github.com/intridea/grape/issues/801): Only evaluate permitted parameter `values` and `default` lazily on each request when declared as a proc - [@dblock](https://github.com/dblock). * [#679](https://github.com/intridea/grape/issues/679): Fixed `OPTIONS` method returning 404 when combined with `prefix`- [@dblock](https://github.com/dblock). * [#679](https://github.com/intridea/grape/issues/679): Fixed unsupported methods returning 404 instead of 405 when combined with `prefix`- [@dblock](https://github.com/dblock). * Your contribution here. diff --git a/README.md b/README.md index 9e390ca32a..640a312fbd 100644 --- a/README.md +++ b/README.md @@ -738,7 +738,7 @@ Parameters can be restricted to a specific set of values with the `:values` opti Default values are eagerly evaluated. Above `:non_random_number` will evaluate to the same number for each call to the endpoint of this `params` block. To have the default evaluate -at calltime use a lambda, like `:random_number` above. +lazily with each request use a lambda, like `:random_number` above. ```ruby params do @@ -747,8 +747,9 @@ params do end ``` -The `:values` option can also be supplied with a `Proc` to be evalutated at runtime for each request. For example, given a status -model you may want to restrict by hashtags that you have previously defined in the `HashTag` model. +The `:values` option can also be supplied with a `Proc`, evaluated lazily with each request. +For example, given a status model you may want to restrict by hashtags that you have +previously defined in the `HashTag` model. ```ruby params do diff --git a/UPGRADING.md b/UPGRADING.md index a1bdbc65f7..640254da43 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -191,11 +191,11 @@ See the [the updated API Formats documentation](https://github.com/intridea/grap #### Changes to Evaluation of Permitted Parameter Values -Permitted parameter values are now evaluated lazily for each request when declared as a proc. The following code would produce the same allowed value in 0.9.0 for each request and a new value since 0.10.0. +Permitted and default parameter values are now only evaluated lazily for each request when declared as a proc. The following code would raise an error at startup time. ```ruby params do - optional :v, values: -> { [SecureRandom.uuid] } + optional :v, values: -> { [:x, :y] }, default: -> { :z } } end ``` @@ -203,7 +203,7 @@ Remove the proc to get the previous behavior. ```ruby params do - optional :v, values: [SecureRandom.uuid] + optional :v, values: [:x, :y], default: :z } end ``` diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index df9ce8b9f3..cdabd6e492 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -104,22 +104,16 @@ def validates(attrs, validations) default = validations[:default] doc_attrs[:default] = default if default - default = default.call if default.is_a?(Proc) - values = validations[:values] doc_attrs[:values] = values if values - evaluated_values = values.is_a?(Proc) ? values.call : values - - coerce_type = evaluated_values.first.class if evaluated_values && coerce_type == Array && !evaluated_values.empty? + coerce_type = guess_coerce_type(coerce_type, values) - # default value should be present in values array, if both exist - if default && evaluated_values && !evaluated_values.include?(default) - fail Grape::Exceptions::IncompatibleOptionValues.new(:default, default, :values, evaluated_values) - end + # 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, evaluated_values) if coerce_type && evaluated_values + validate_value_coercion(coerce_type, values) doc_attrs[:documentation] = validations.delete(:documentation) if validations.key?(:documentation) @@ -145,6 +139,19 @@ def validates(attrs, validations) end end + def guess_coerce_type(coerce_type, values) + return coerce_type if !values || values.is_a?(Proc) + return values.first.class if coerce_type == Array && !values.empty? + coerce_type + end + + def check_incompatible_option_values(values, default) + return unless values && default + return if values.is_a?(Proc) || default.is_a?(Proc) + return if values.include?(default) + fail Grape::Exceptions::IncompatibleOptionValues.new(:default, default, :values, values) + end + def validate(type, options, attrs, doc_attrs) validator_class = Validations.validators[type.to_s] @@ -157,6 +164,8 @@ def validate(type, options, attrs, doc_attrs) end def validate_value_coercion(coerce_type, values) + return unless coerce_type && values + return if values.is_a?(Proc) coerce_type = coerce_type.first if coerce_type.kind_of?(Array) if values.any? { |v| !v.kind_of?(coerce_type) } fail Grape::Exceptions::IncompatibleOptionValues.new(:type, coerce_type, :values, values) diff --git a/spec/grape/validations/params_scope_spec.rb b/spec/grape/validations/params_scope_spec.rb index b76a4e3966..fdf1b0cad3 100644 --- a/spec/grape/validations/params_scope_spec.rb +++ b/spec/grape/validations/params_scope_spec.rb @@ -50,7 +50,7 @@ def app it 'raises exception when values are of different type' do expect do - subject.params { requires :numbers, type: Array, values: -> { [1, 'definitely not a number', 3] } } + subject.params { requires :numbers, type: Array, values: [1, 'definitely not a number', 3] } end.to raise_error Grape::Exceptions::IncompatibleOptionValues end end diff --git a/spec/grape/validations/validators/values_spec.rb b/spec/grape/validations/validators/values_spec.rb index b44670d67b..ed052c839a 100644 --- a/spec/grape/validations/validators/values_spec.rb +++ b/spec/grape/validations/validators/values_spec.rb @@ -76,11 +76,6 @@ class API < Grape::API end end get '/optional_with_required_values' - - params do - optional :type, type: String, values: -> { [SecureRandom.uuid] } - end - get '/random_values' end end end @@ -161,14 +156,7 @@ def app it 'raises IncompatibleOptionValues on an invalid default value from proc' do subject = Class.new(Grape::API) expect do - subject.params { optional :type, values: ['valid-type1', 'valid-type2', 'valid-type3'], default: -> { ValuesModel.values.sample + '_invalid' } } - end.to raise_error Grape::Exceptions::IncompatibleOptionValues - end - - it 'raises IncompatibleOptionValues on an invalid default value from proc validating against values in a proc' do - subject = Class.new(Grape::API) - expect do - subject.params { optional :type, values: -> { ValuesModel.values }, default: -> { ValuesModel.values.sample + '_invalid' } } + subject.params { optional :type, values: ['valid-type1', 'valid-type2', 'valid-type3'], default: ValuesModel.values.sample + '_invalid' } end.to raise_error Grape::Exceptions::IncompatibleOptionValues end @@ -205,9 +193,32 @@ def app end.to raise_error Grape::Exceptions::IncompatibleOptionValues end - it 'evaluates values dynamically with each request' do - allow(SecureRandom).to receive(:uuid).and_return('foo') - get '/random_values', type: 'foo' - expect(last_response.status).to eq 200 + context 'with a lambda values' do + subject do + Class.new(Grape::API) do + params do + optional :type, type: String, values: -> { [SecureRandom.uuid] }, default: -> { SecureRandom.uuid } + end + get '/random_values' + end + end + + def app + subject + end + + before do + expect(SecureRandom).to receive(:uuid).and_return('foo').once + end + + it 'only evaluates values dynamically with each request' do + get '/random_values', type: 'foo' + expect(last_response.status).to eq 200 + end + + it 'chooses default' do + get '/random_values' + expect(last_response.status).to eq 200 + end end end