Fix: #211: Custom validator shouldn't be triggered when optional and …

…not present

Updated validator to only run custom optional validation when the param
is present. Re-wrote validations_spec to be a little more robust.
 Added section to documentation for custom validation.

CHANGELOG.markdown

@@ -1,6 +1,7 @@
 Next Release
 ============
 
+* [#211](https://github.com/intridea/grape/pull/211): Fix: Custom param validator shouldn't validate when optional and param not present - [@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, doc_attrs)
         @attrs = Array(attrs)
+        @doc_attrs = doc_attrs
 
         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 @doc_attrs[: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, doc_attrs)
         @option = options
         super
       end
@@ -109,7 +112,7 @@ def validates(attrs, validations)
         validations.each do |type, options|
           validator_class = Validations::validators[type.to_s]
           if validator_class
-            @api.settings[:validations] << validator_class.new(attrs, options)
+            @api.settings[:validations] << validator_class.new(attrs, options, doc_attrs)
           else
             raise "unknown validator: #{type}"
           end

spec/grape/validations_spec.rb

@@ -1,33 +1,85 @@
 require 'spec_helper'
 
-describe Grape::Validations do 
-  module ValidationsSpec
-    class API < Grape::API
-      default_format :json
-
-      params do
-        requires :name, :company
-        optional :a_number, :regexp => /^[0-9]+$/
+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
+
+      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
+
+    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 do
-        "Hello"
+
+      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
-    end
-  end
-
-  def app
-    ValidationsSpec::API 
-  end
 
-  it 'validates optional parameter if present' do
-    get('/', :name => "Bob", :company => "TestCorp", :a_number => "string")
-    last_response.status.should == 400
-    last_response.body.should == "invalid parameter: a_number"
+      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('/', :name => "Bob", :company => "TestCorp", :a_number => 45)
-    last_response.status.should == 200
-    last_response.body.should == "Hello"
+        get '/optional_custom', { :custom => 123 }
+        last_response.status.should == 400
+        last_response.body.should == 'custom: is not custom!'
+      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
+
+      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 == 'custom: is not custom!'
+      end
+    end
   end
-
 end