Changed the order of param validation and coercion

Param validation will check for presence first and then handle any coercion. This fixes a bug where coercsion happens after the fact and validation icorrectly fails or passes

Author: adamgotterer

lib/grape/validations.rb

@@ -109,15 +109,31 @@ def validates(attrs, validations)
 
         @api.document_attribute(attrs, doc_attrs)
 
+        # Validate for presence before any other validators
+        if validations.has_key?(:presence) && validations[:presence]
+          validate('presence', validations[:presence], attrs, doc_attrs)
+        end
+
+        # Before we run the rest of the validators, lets handle
+        # whatever coercion so that we are working with correctly
+        # type casted values
+        if validations.has_key? :coerce
+          validate('coerce', validations[:coerce], attrs, doc_attrs)
+          validations.delete(:coerce)
+        end
+
         validations.each do |type, options|
-          validator_class = Validations::validators[type.to_s]
-          if validator_class
-            @api.settings[:validations] << validator_class.new(attrs, options, doc_attrs)
-          else
-            raise "unknown validator: #{type}"
-          end
+          validate(type, options, attrs, doc_attrs)
+        end
+      end
+
+      def validate(type, options, attrs, doc_attrs)
+        validator_class = Validations::validators[type.to_s]
+        if validator_class
+          @api.settings[:validations] << validator_class.new(attrs, options, doc_attrs)
+        else
+          raise "unknown validator: #{type}"
         end
-      
       end
 
     end

spec/grape/validations_spec.rb

@@ -1,85 +1,119 @@
 require 'spec_helper'
 
-module CustomValidations  
-  class Customvalidator < Grape::Validations::Validator
-    def validate_param!(attr_name, params)
-      unless params[attr_name] == 'im custom'
-        throw :error, :status => 400, :message => "#{attr_name}: is not custom!"
-      end    
-    end
-  end  
-end   
+
 
 describe Grape::API do
   subject { Class.new(Grape::API) }
   def app; subject end
 
   describe 'params' do
-    it 'validates optional parameter if present' do
-      subject.params { optional :a_number, :regexp => /^[0-9]+$/ }
-      subject.get '/optional' do 'optional works!'; end
+    context 'optional' do
+      it 'validates when params is present' do
+        subject.params { optional :a_number, :regexp => /^[0-9]+$/ }
+        subject.get '/optional' do 'optional works!'; end
+
+        get '/optional', { :a_number => 'string' }
+        last_response.status.should == 400
+        last_response.body.should == 'invalid parameter: a_number'
+
+        get '/optional', { :a_number => 45 }
+        last_response.status.should == 200
+        last_response.body.should == 'optional works!'
+      end
 
-      get '/optional', { :a_number => 'string' }
-      last_response.status.should == 400
-      last_response.body.should == 'invalid parameter: a_number'
+      it "doesn't validate when param not present" do
+        subject.params { optional :a_number, :regexp => /^[0-9]+$/ }
+        subject.get '/optional' do 'optional works!'; end
 
-      get '/optional', { :a_number => 45 }
-      last_response.status.should == 200
-      last_response.body.should == 'optional works!'
+        get '/optional'
+        last_response.status.should == 200
+        last_response.body.should == 'optional works!'
+      end
     end
 
-    context 'when using optional with a custom validator' do
+    context 'required' do
       before do
-        subject.params { optional :custom, :customvalidator => true }
-        subject.get '/optional_custom' do 'optional with custom works!'; end 
+        subject.params { requires :key }
+        subject.get '/required' do 'required works'; end
       end
 
-      it 'validates when param is present' do
-        get '/optional_custom', { :custom => 'im custom' }
-        last_response.status.should == 200
-        last_response.body.should == 'optional with custom works!'
-
-        get '/optional_custom', { :custom => 'im wrong' }
+      it 'errors when param not present' do
+        get '/required'
         last_response.status.should == 400
-        last_response.body.should == 'custom: is not custom!'
+        last_response.body.should == 'missing parameter: key'
       end
 
-      it "skip validation when parameter isn't present" do
-        get '/optional_custom'
+      it "doesn't throw a missing param when param is present" do
+        get '/required', { :key => 'cool' }
         last_response.status.should == 200
-        last_response.body.should == 'optional with custom works!'
-      end
-
-      it 'validates with custom validator when param present and incorrect type' do
-        subject.params { optional :custom, :type => String, :customvalidator => true }
-    
-        get '/optional_custom', { :custom => 123 }
-        last_response.status.should == 400
-        last_response.body.should == 'custom: is not custom!'
+        last_response.body.should == 'required works'
       end
     end
 
-    context 'when using requires with a custom validator' do
-      before do
-        subject.params { requires :custom, :customvalidator => true }
-        subject.get '/required_custom' do 'required with custom works!'; end
-      end
+    context 'custom validation' do
+      module CustomValidations  
+        class Customvalidator < Grape::Validations::Validator
+          def validate_param!(attr_name, params)
+            unless params[attr_name] == 'im custom'
+              throw :error, :status => 400, :message => "#{attr_name}: is not custom!"
+            end    
+          end
+        end  
+      end     
 
-      it 'validates when param is present' do
-        get '/required_custom', { :custom => 'im wrong, validate me' }
-        last_response.status.should == 400
-        last_response.body.should == 'custom: is not custom!'
+      context 'when using optional with a custom validator' do
+        before do
+          subject.params { optional :custom, :customvalidator => true }
+          subject.get '/optional_custom' do 'optional with custom works!'; end 
+        end
 
-        get '/required_custom', { :custom => 'im custom' }
-        last_response.status.should == 200
-        last_response.body.should == 'required with custom works!'
+        it 'validates when param is present' do
+          get '/optional_custom', { :custom => 'im custom' }
+          last_response.status.should == 200
+          last_response.body.should == 'optional with custom works!'
+
+          get '/optional_custom', { :custom => 'im wrong' }
+          last_response.status.should == 400
+          last_response.body.should == 'custom: is not custom!'
+        end
+
+        it "skip validation when parameter isn't present" do
+          get '/optional_custom'
+          last_response.status.should == 200
+          last_response.body.should == 'optional with custom works!'
+        end
+
+        it 'validates with custom validator when param present and incorrect type' do
+          subject.params { optional :custom, :type => String, :customvalidator => true }
+      
+          get '/optional_custom', { :custom => 123 }
+          last_response.status.should == 400
+          last_response.body.should == 'custom: is not custom!'
+        end
       end
 
-      it 'validates when param is not present' do
-        get '/required_custom'
-        last_response.status.should == 400
-        last_response.body.should == 'custom: is not custom!'
+      context 'when using requires with a custom validator' do
+        before do
+          subject.params { requires :custom, :customvalidator => true }
+          subject.get '/required_custom' do 'required with custom works!'; end
+        end
+
+        it 'validates when param is present' do
+          get '/required_custom', { :custom => 'im wrong, validate me' }
+          last_response.status.should == 400
+          last_response.body.should == 'custom: is not custom!'
+
+          get '/required_custom', { :custom => 'im custom' }
+          last_response.status.should == 200
+          last_response.body.should == 'required with custom works!'
+        end
+
+        it 'validates when param is not present' do
+          get '/required_custom'
+          last_response.status.should == 400
+          last_response.body.should == 'missing parameter: custom'
+        end
       end
-    end
+    end # end custom validation
   end
 end