Merge pull request ruby-grape#212 from adamgotterer/master

Updates to validation and coercion: Fix ruby-grape#211 and force order of operations for presence and coercion.

Author: dblock

CHANGELOG.markdown

@@ -1,6 +1,7 @@
 Next Release
 ============
 
+* [#211](https://github.com/intridea/grape/pull/211): Updates to validation and coercion: Fix #211 and force order of operations for presence and coercion - [@adamgotterer](https://github.com/adamgotterer).
 * [#210](https://github.com/intridea/grape/pull/210): Fix: `Endpoint#body_params` causing undefined method 'size' - [@adamgotterer](https://github.com/adamgotterer).
 * [#201](https://github.com/intridea/grape/pull/201): Rewritten `params` DSL, including support for coercion and validations - [@schmurfy](https://github.com/schmurfy).
 * [#205](https://github.com/intridea/grape/pull/205): Fix: Corrected parsing of empty JSON body on POST/PUT - [@tim-vandecasteele](https://github.com/tim-vandecasteele).

README.markdown

@@ -226,9 +226,13 @@ get ':id' do
 end
 ```
 
+When a type is specified an implicit validation is done after the coercion to ensure 
+the output type is the one declared.
+
+### Namespace Validation and Coercion
 Namespaces allow parameter definitions and apply to every method within the namespace.
 
-``` ruby
+```ruby
 namespace :shelves do
   params do
     requires :shelf_id, type: Integer, desc: "A shelf."
@@ -246,8 +250,41 @@ namespace :shelves do
 end
 ```
 
-When a type is specified an implicit validation is done after the coercion to ensure 
-the output type is the one declared.
+### Custom Validators
+```ruby
+class doit < 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  
+```
+
+```ruby
+params do
+  requires :name, :doit => true
+end
+```
+
+You can also create custom classes that take additional parameters
+```ruby
+class Length < Grape::Validations::SingleOptionValidator
+  def validate_param!(attr_name, params)
+    unless params[attr_name].length == @option
+      throw :error, :status => 400, :message => "#{attr_name}: must be #{@option} characters long"
+    end    
+  end
+end  
+``` 
+
+```ruby
+params do
+  requires :name, :length => 5
+end
+```
+
+
 
 ## Headers
 

lib/grape/validations.rb

@@ -8,8 +8,9 @@ module Validations
     # All validators must inherit from this class.
     # 
     class Validator
-      def initialize(attrs, options)
+      def initialize(attrs, options, required)
         @attrs = Array(attrs)
+        @required = required
 
         if options.is_a?(Hash) && !options.empty?
           raise "unknown options: #{options.keys}"
@@ -18,7 +19,9 @@ def initialize(attrs, options)
 
       def validate!(params)
         @attrs.each do |attr_name|
-          validate_param!(attr_name, params)
+          if @required || params.has_key?(attr_name)
+            validate_param!(attr_name, params)
+          end
         end
       end
 
@@ -37,7 +40,7 @@ def self.convert_to_short_name(klass)
     ##
     # Base class for all validators taking only one param.
     class SingleOptionValidator < Validator
-      def initialize(attrs, options)
+      def initialize(attrs, options, required)
         @option = options
         super
       end
@@ -106,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)
-          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[:required])
+        else
+          raise "unknown validator: #{type}"
         end
-      
       end
 
     end

spec/grape/validations/coerce_spec.rb

@@ -1,111 +1,116 @@
 require 'spec_helper'
 
 describe Grape::Validations::CoerceValidator do
-  module ValidationsSpec
-    module CoerceValidatorSpec
-      class User
-        include Virtus
-        attribute :id, Integer
-        attribute :name, String
-      end
+  subject { Class.new(Grape::API) }
+  def app; subject end
 
-      class API < Grape::API
-        default_format :json
+  describe 'coerce' do
+    it 'error on malformed input' do
+      subject.params { requires :int, :type => Integer }
+      subject.get '/single' do 'int works'; end
 
-        params do
-          requires :int, :coerce => Integer
-        end
-        get '/single' do
-        end
+      get '/single', :int => '43a' 
+      last_response.status.should == 400
+      last_response.body.should == 'invalid parameter: int'
 
-        params do
-          requires :ids, :type => Array[Integer]
-        end
-        get '/arr' do
-        end
+      get '/single', :int => '43'
+      last_response.status.should == 200
+      last_response.body.should == 'int works'
+    end
 
-        params do
-          requires :user, :type => ValidationsSpec::CoerceValidatorSpec::User
-        end
-        get '/user' do
-        end
+    it 'error on malformed input (Array)' do
+      subject.params { requires :ids, :type => Array[Integer] }
+      subject.get '/array' do 'array int works'; end
 
-        params do
-          requires :int, :coerce => Integer
-          optional :int2, :coerce => Integer
-          optional :arr, :coerce => Array[Integer]
-          optional :bool, :coerce => Array[Boolean]
-        end
-        get '/coerce' do
-          {
-            :int    => params[:int].class,
-            :arr    => params[:arr] ? params[:arr][0].class : nil,
-            :bool   => params[:bool] ? (params[:bool][0] == true) && (params[:bool][1] == false) : nil
-          }
-        end
-        params do
-          requires :uploaded_file, :type => Rack::Multipart::UploadedFile
-        end
-        post '/file' do
-          {
-            :dpx_file => params[:uploaded_file]
-          }
+      get 'array', { :ids => ['1', '2', 'az'] }
+      last_response.status.should == 400
+      last_response.body.should == 'invalid parameter: ids'
+
+      get 'array', { :ids => ['1', '2', '890'] }
+      last_response.status.should == 200
+      last_response.body.should == 'array int works'
+    end
+
+    context 'complex objects' do
+      module CoerceValidatorSpec
+        class User
+          include Virtus
+          attribute :id, Integer
+          attribute :name, String
         end
       end
-    end
-  end
 
-  def app
-    ValidationsSpec::CoerceValidatorSpec::API
-  end
+      it 'error on malformed input for complex objects' do
+        subject.params { requires :user, :type => CoerceValidatorSpec::User }
+        subject.get '/user' do 'complex works'; end
 
-  it "should return an error on malformed input" do
-    get '/single', :int => "43a"
-    last_response.status.should == 400
+        get '/user', :user => "32"
+        last_response.status.should == 400
+        last_response.body.should == 'invalid parameter: user'
 
-    get '/single', :int => "43"
-    last_response.status.should == 200
-  end
+        get '/user', :user => { :id => 32, :name => 'Bob' }
+        last_response.status.should == 200
+        last_response.body.should == 'complex works'
+      end
+    end
 
-  it "should return an error on malformed input (array)" do
-    get '/arr', :ids => ["1", "2", "az"]
-    last_response.status.should == 400
+    context 'coerces' do
+      it 'Integer' do
+        subject.params { requires :int, :coerce => Integer }
+        subject.get '/int' do params[:int].class; end
 
-    get '/arr', :ids => ["1", "2", "890"]
-    last_response.status.should == 200
-  end
+        get '/int', { :int => "45" }
+        last_response.status.should == 200
+        last_response.body.should == 'Fixnum'
+      end
 
-  it "should return an error on malformed input (complex object)" do
-    # this request does raise an error inside Virtus
-    get '/user', :user => "32"
-    last_response.status.should == 400
+      it 'Array of Integers' do
+        subject.params { requires :arry, :coerce => Array[Integer] }
+        subject.get '/array' do params[:arry][0].class; end
 
-    get '/user', :user => { :id => 32, :name => "Bob"}
-    last_response.status.should == 200
-  end
+        get '/array', { :arry => [ '1', '2', '3' ] }
+        last_response.status.should == 200
+        last_response.body.should == 'Fixnum'
+      end
 
-  it 'should coerce inputs' do
-    get('/coerce', :int => "43", :int2 => "42")
-    last_response.status.should == 200
-    ret = MultiJson.load(last_response.body)
-    ret["int"].should == "Fixnum"
-
-    get('/coerce', :int => "40", :int2 => "42", :arr => ["1","20","3"], :bool => [1, 0])
-    # last_response.body.should == ""
-    last_response.status.should == 200
-    ret = MultiJson.load(last_response.body)
-    ret["int"].should == "Fixnum"
-    ret["arr"].should == "Fixnum"
-    ret["bool"].should == true
-  end
+      it 'Array of Bools' do
+        subject.params { requires :arry, :coerce => Array[Virtus::Attribute::Boolean] }
+        subject.get '/array' do params[:arry][0].class; end
 
-  it 'should not return an error when an optional parameter is nil' do
-    get('/coerce', :int => "40")
-    last_response.status.should == 200
-  end
+        get 'array', { :arry => [1, 0] }
+        last_response.status.should == 200
+        last_response.body.should == 'TrueClass'
+      end
+
+      it 'Bool' do
+        subject.params { requires :bool, :coerce => Virtus::Attribute::Boolean }
+        subject.get '/bool' do params[:bool].class; end
+
+        get '/bool', { :bool => 1 }
+        last_response.status.should == 200
+        last_response.body.should == 'TrueClass'
+        
+        get '/bool', { :bool => 0 }
+        last_response.status.should == 200
+        last_response.body.should == 'FalseClass'
+
+        get '/bool', { :bool => 'false' }
+        last_response.status.should == 200
+        last_response.body.should == 'FalseClass'
+
+        get '/bool', { :bool => 'true' }
+        last_response.status.should == 200
+        last_response.body.should == 'TrueClass'
+      end
+
+      it 'file' do
+        subject.params { requires :file, :coerce => Rack::Multipart::UploadedFile }
+        subject.post '/upload' do params[:file].filename; end
 
-  it 'should coerce a file' do
-    post('/file', :uploaded_file => Rack::Test::UploadedFile.new(__FILE__))
-    last_response.status.should == 201
+        post '/upload', { :file => Rack::Test::UploadedFile.new(__FILE__) }
+        last_response.status.should == 201
+        last_response.body.should == File.basename(__FILE__).to_s
+      end
+    end
   end
 end