Fix ruby-grape#801: Only evaluate values proc lazily.

.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:27:05 -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: 56
 
 # Offense count: 1
 Metrics/BlockNesting:
@@ -30,9 +30,9 @@ Metrics/ClassLength:
 
 # Offense count: 15
 Metrics/CyclomaticComplexity:
-  Max: 21
+  Max: 23
 
-# Offense count: 546
+# Offense count: 544
 # Configuration parameters: AllowURI, URISchemes.
 Metrics/LineLength:
   Max: 198
@@ -44,7 +44,7 @@ Metrics/MethodLength:
 
 # Offense count: 13
 Metrics/PerceivedComplexity:
-  Max: 21
+  Max: 23
 
 # Offense count: 26
 # Cop supports --auto-correct.

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` 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

@@ -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` to be evalutated once at startup and then 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.
 
 ```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 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

@@ -107,19 +107,17 @@ def validates(attrs, validations)
         default = default.call if default.is_a?(Proc)
 
         values = validations[:values]
-        doc_attrs[:values] = values if values
+        doc_attrs[:values] = values if values && !values.is_a?(Proc)
 
-        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 = values.first.class if values && !values.is_a?(Proc) && coerce_type == Array && !values.empty?
 
         # 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)
+        if !values.is_a?(Proc) && default && values && !values.include?(default)
+          fail Grape::Exceptions::IncompatibleOptionValues.new(:default, default, :values, values)
         end
 
         # 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) if coerce_type && values
 
         doc_attrs[:documentation] = validations.delete(:documentation) if validations.key?(:documentation)
 
@@ -157,6 +155,7 @@ def validate(type, options, attrs, doc_attrs)
       end
 
       def validate_value_coercion(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/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
@@ -165,13 +160,6 @@ def app
     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' } }
-    end.to raise_error Grape::Exceptions::IncompatibleOptionValues
-  end
-
   it 'raises IncompatibleOptionValues on an invalid default value' do
     subject = Class.new(Grape::API)
     expect do
@@ -205,9 +193,27 @@ 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] }
+        end
+        get '/random_values'
+      end
+    end
+
+    def app
+      subject
+    end
+
+    before do
+      allow(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
   end
 end