Merge pull request #1498 from jlfaber/bug/1433/given_block_validation

Skip running param validators inside inactive given blocks (#1433)

Author: dblock

CHANGELOG.md

@@ -9,6 +9,7 @@
 
 #### Fixes
 
+* [#1498](https://github.com/ruby-grape/grape/pull/1498): Skip validations in inactive given blocks - [@jlfaber](https://github.com/jlfaber).
 * [#1479](https://github.com/ruby-grape/grape/pull/1479): Support inserting middleware before/after anonymous classes in the middleware stack - [@rosa](https://github.com/rosa).
 * [#1488](https://github.com/ruby-grape/grape/pull/1488): Ensure calling before filters when receiving OPTIONS request - [@namusyaka](https://github.com/namusyaka), [@jlfaber](https://github.com/jlfaber).
 * [#1493](https://github.com/ruby-grape/grape/pull/1493): Coercion and lambda fails params validation - [@jonmchan](https://github.com/jonmchan).

lib/grape/validations/validators/allow_blank.rb

@@ -7,19 +7,6 @@ def validate_param!(attr_name, params)
         value = params[attr_name]
         value = value.strip if value.respond_to?(:strip)
 
-        key_exists = params.key?(attr_name)
-
-        should_validate = if @scope.root?
-                            # root scope. validate if it's a required param. if it's optional, validate only if key exists in hash
-                            @required || key_exists
-                          else # nested scope
-                            (@required && params.present?) ||
-                              # optional param but key inside scoping element exists
-                              (!@required && params.key?(attr_name))
-                          end
-
-        return unless should_validate
-
         return if false == value || value.present?
 
         raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:blank)

lib/grape/validations/validators/base.rb

@@ -24,6 +24,7 @@ def initialize(attrs, options, required, scope)
       # @raise [Grape::Exceptions::Validation] if validation failed
       # @return [void]
       def validate(request)
+        return unless @scope.should_validate?(request.params)
         validate!(request.params)
       end
 

lib/grape/validations/validators/default.rb

@@ -18,8 +18,6 @@ def validate_param!(attr_name, params)
       end
 
       def validate!(params)
-        return unless @scope.should_validate?(params)
-
         attrs = AttributesIterator.new(self, @scope, params)
         attrs.each do |resource_params, attr_name|
           if resource_params.is_a?(Hash) && resource_params[attr_name].nil?

lib/grape/validations/validators/presence.rb

@@ -1,11 +1,6 @@
 module Grape
   module Validations
     class PresenceValidator < Base
-      def validate!(params)
-        return unless @scope.should_validate?(params)
-        super
-      end
-
       def validate_param!(attr_name, params)
         return if params.respond_to?(:key?) && params.key?(attr_name)
         raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:presence)

spec/grape/validations/params_scope_spec.rb

@@ -181,7 +181,6 @@ def initialize(value)
 
       subject.post('/required') { 'coercion with proc works' }
       post '/required', numbers: '10'
-      p last_response.body
       expect(last_response.status).to eq(201)
       expect(last_response.body).to eq('coercion with proc works')
     end
@@ -372,6 +371,44 @@ def initialize(value)
       expect(last_response.status).to eq(200)
     end
 
+    it 'applies only the appropriate validation' do
+      subject.params do
+        optional :a
+        optional :b
+        mutually_exclusive :a, :b
+        given :a do
+          requires :c, type: String
+        end
+        given :b do
+          requires :c, type: Integer
+        end
+      end
+      subject.get('/multiple') { declared(params).to_json }
+
+      get '/multiple'
+      expect(last_response.status).to eq(200)
+
+      get '/multiple', a: true, c: 'test'
+      expect(last_response.status).to eq(200)
+      expect(JSON.parse(last_response.body).symbolize_keys).to eq a: 'true', b: nil, c: 'test'
+
+      get '/multiple', b: true, c: '3'
+      expect(last_response.status).to eq(200)
+      expect(JSON.parse(last_response.body).symbolize_keys).to eq a: nil, b: 'true', c: 3
+
+      get '/multiple', a: true
+      expect(last_response.status).to eq(400)
+      expect(last_response.body).to eq('c is missing')
+
+      get '/multiple', b: true
+      expect(last_response.status).to eq(400)
+      expect(last_response.body).to eq('c is missing')
+
+      get '/multiple', a: true, b: true, c: 'test'
+      expect(last_response.status).to eq(400)
+      expect(last_response.body).to eq('a, b are mutually exclusive, c is invalid')
+    end
+
     it 'raises an error if the dependent parameter was never specified' do
       expect do
         subject.params do
@@ -437,89 +474,114 @@ def initialize(value)
   end
 
   context 'when validations are dependent on a parameter with specific value' do
-    before do
-      subject.params do
-        optional :a
-        given a: ->(val) { val == 'x' } do
-          requires :b
+    # build test cases from all combinations of declarations and options
+    a_decls = %i(optional requires)
+    a_options = [{}, { values: %w(x y z) }]
+    b_options = [{}, { type: String }, { allow_blank: false }, { type: String, allow_blank: false }]
+    combinations = a_decls.product(a_options, b_options)
+    combinations.each_with_index do |combination, i|
+      a_decl, a_opts, b_opts = combination
+
+      context "(case #{i})" do
+        before do
+          # puts "a_decl: #{a_decl}, a_opts: #{a_opts}, b_opts: #{b_opts}"
+          subject.params do
+            send a_decl, :a, **a_opts
+            given(a: ->(val) { val == 'x' }) { requires :b, **b_opts }
+            given(a: ->(val) { val == 'y' }) { requires :c, **b_opts }
+          end
+          subject.get('/test') { declared(params).to_json }
         end
-      end
-      subject.get('/test') { declared(params).to_json }
-    end
-
-    it 'applies the validations only if the parameter has the specific value' do
-      get '/test'
-      expect(last_response.status).to eq(200)
 
-      get '/test', a: 'x'
-      expect(last_response.status).to eq(400)
-      expect(last_response.body).to eq('b is missing')
+        if a_decl == :optional
+          it 'skips validation when base param is missing' do
+            get '/test'
+            expect(last_response.status).to eq(200)
+          end
+        end
 
-      get '/test', a: 'x', b: true
-      expect(last_response.status).to eq(200)
-    end
+        it 'skips validation when base param does not have a specified value' do
+          get '/test', a: 'z'
+          expect(last_response.status).to eq(200)
 
-    it 'raises an error if the dependent parameter was never specified' do
-      expect do
-        subject.params do
-          given :c do
-          end
+          get '/test', a: 'z', b: ''
+          expect(last_response.status).to eq(200)
         end
-      end.to raise_error(Grape::Exceptions::UnknownParameter)
-    end
 
-    it 'includes the parameter within #declared(params)' do
-      get '/test', a: true, b: true
+        it 'applies the validation when base param has the specific value' do
+          get '/test', a: 'x'
+          expect(last_response.status).to eq(400)
+          expect(last_response.body).to include('b is missing')
 
-      expect(JSON.parse(last_response.body)).to eq('a' => 'true', 'b' => 'true')
+          get '/test', a: 'x', b: true
+          expect(last_response.status).to eq(200)
+
+          get '/test', a: 'x', b: true, c: ''
+          expect(last_response.status).to eq(200)
+        end
+
+        it 'includes the parameter within #declared(params)' do
+          get '/test', a: 'x', b: true
+          expect(JSON.parse(last_response.body)).to eq('a' => 'x', 'b' => 'true', 'c' => nil)
+        end
+      end
     end
+  end
 
-    it 'returns a sensible error message within a nested context' do
+  it 'raises an error if the dependent parameter was never specified' do
+    expect do
       subject.params do
-        requires :bar, type: Hash do
-          optional :a
-          given a: ->(val) { val == 'x' } do
-            requires :b
-          end
+        given :c do
         end
       end
-      subject.get('/nested') { 'worked' }
+    end.to raise_error(Grape::Exceptions::UnknownParameter)
+  end
 
-      get '/nested', bar: { a: 'x' }
-      expect(last_response.status).to eq(400)
-      expect(last_response.body).to eq('bar[b] is missing')
+  it 'returns a sensible error message within a nested context' do
+    subject.params do
+      requires :bar, type: Hash do
+        optional :a
+        given a: ->(val) { val == 'x' } do
+          requires :b
+        end
+      end
     end
+    subject.get('/nested') { 'worked' }
 
-    it 'includes the nested parameter within #declared(params)' do
-      subject.params do
-        requires :bar, type: Hash do
-          optional :a
-          given a: ->(val) { val == 'x' } do
-            requires :b
-          end
+    get '/nested', bar: { a: 'x' }
+    expect(last_response.status).to eq(400)
+    expect(last_response.body).to eq('bar[b] is missing')
+  end
+
+  it 'includes the nested parameter within #declared(params)' do
+    subject.params do
+      requires :bar, type: Hash do
+        optional :a
+        given a: ->(val) { val == 'x' } do
+          requires :b
         end
       end
-      subject.get('/nested') { declared(params).to_json }
-
-      get '/nested', bar: { a: 'x', b: 'yes' }
-      expect(JSON.parse(last_response.body)).to eq('bar' => { 'a' => 'x', 'b' => 'yes' })
     end
+    subject.get('/nested') { declared(params).to_json }
 
-    it 'includes level 2 nested parameters outside the given within #declared(params)' do
-      subject.params do
-        requires :bar, type: Hash do
-          optional :a
-          given a: ->(val) { val == 'x' } do
-            requires :c, type: Hash do
-              requires :b
-            end
+    get '/nested', bar: { a: 'x', b: 'yes' }
+    expect(JSON.parse(last_response.body)).to eq('bar' => { 'a' => 'x', 'b' => 'yes' })
+  end
+
+  it 'includes level 2 nested parameters outside the given within #declared(params)' do
+    subject.params do
+      requires :bar, type: Hash do
+        optional :a
+        given a: ->(val) { val == 'x' } do
+          requires :c, type: Hash do
+            requires :b
           end
         end
       end
-      subject.get('/nested') { declared(params).to_json }
-
-      get '/nested', bar: { a: 'x', c: { b: 'yes' } }
-      expect(JSON.parse(last_response.body)).to eq('bar' => { 'a' => 'x', 'c' => { 'b' => 'yes' } })
     end
+    subject.get('/nested') { declared(params).to_json }
+
+    get '/nested', bar: { a: 'x', c: { b: 'yes' } }
+    expect(JSON.parse(last_response.body)).to eq('bar' => { 'a' => 'x', 'c' => { 'b' => 'yes' } })
   end
 end

spec/grape/validations/validators/allow_blank_spec.rb

@@ -9,7 +9,12 @@ class API < Grape::API
         params do
           requires :name, allow_blank: false
         end
-        get
+        get '/disallow_blank'
+
+        params do
+          optional :name, type: String, allow_blank: false
+        end
+        get '/opt_disallow_string_blank'
 
         params do
           optional :name, allow_blank: false
@@ -247,26 +252,31 @@ def app
 
   context 'invalid input' do
     it 'refuses empty string' do
-      get '/', name: ''
+      get '/disallow_blank', name: ''
       expect(last_response.status).to eq(400)
 
       get '/disallow_datetime_blank', val: ''
       expect(last_response.status).to eq(400)
     end
 
     it 'refuses only whitespaces' do
-      get '/', name: '   '
+      get '/disallow_blank', name: '   '
       expect(last_response.status).to eq(400)
 
-      get '/', name: "  \n "
+      get '/disallow_blank', name: "  \n "
       expect(last_response.status).to eq(400)
 
-      get '/', name: "\n"
+      get '/disallow_blank', name: "\n"
       expect(last_response.status).to eq(400)
     end
 
     it 'refuses nil' do
-      get '/', name: nil
+      get '/disallow_blank', name: nil
+      expect(last_response.status).to eq(400)
+    end
+
+    it 'refuses missing' do
+      get '/disallow_blank'
       expect(last_response.status).to eq(400)
     end
   end
@@ -432,8 +442,13 @@ def app
   end
 
   context 'valid input' do
+    it 'allows missing optional strings' do
+      get 'opt_disallow_string_blank'
+      expect(last_response.status).to eq(200)
+    end
+
     it 'accepts valid input' do
-      get '/', name: 'bob'
+      get '/disallow_blank', name: 'bob'
       expect(last_response.status).to eq(200)
     end