Fix #801: Only evaluate values proc lazily.

Author: dblock

.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

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.

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

UPGRADING.md

@@ -191,19 +191,19 @@ 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
 ```
 
 Remove the proc to get the previous behavior.
 
 ```ruby
 params do
-  optional :v, values: [SecureRandom.uuid]
+  optional :v, values: [:x, :y], default: :z }
 end
 ```
 

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)

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

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