Validation fixes for issue #543

CHANGELOG.md

@@ -12,6 +12,7 @@ Next Release
 * [#527](https://github.com/intridea/grape/pull/527): `before_validation` now a distinct callback (supersedes [#523](https://github.com/intridea/grape/pull/523)) - [@myitcv](https://github.com/myitcv).
 * [#531](https://github.com/intridea/grape/pull/531): Helpers are now available to auth middleware, executing in the context of the endpoint - [@joelvh](https://github.com/joelvh).
 * [#540](https://github.com/intridea/grape/pull/540): Ruby 2.1.0 is now supported - [@salimane](https://github.com/salimane).
+* [#545](https://github.com/intridea/grape/pull/545): Add `type` (Array or Hash) support to `requires`, `optional` and `group` with block and fix several validation issues around these - [@bwalex](https://github.com/bwalex).
 * Your contribution here.
 
 #### Fixes

lib/grape.rb

@@ -6,6 +6,7 @@
 require 'rack/auth/basic'
 require 'rack/auth/digest/md5'
 require 'hashie'
+require 'set'
 require 'active_support/core_ext/hash/indifferent_access'
 require 'active_support/ordered_hash'
 require 'active_support/core_ext/object/conversions'

lib/grape/validations.rb

@@ -91,6 +91,7 @@ def initialize(opts, &block)
         @parent   = opts[:parent]
         @api      = opts[:api]
         @optional = opts[:optional] || false
+        @type     = opts[:type]
         @declared_params = []
 
         instance_eval(&block)
@@ -99,29 +100,33 @@ def initialize(opts, &block)
       end
 
       def should_validate?(parameters)
-        return false if @optional && params(parameters).all?(&:blank?)
+        return false if @optional && params(parameters).respond_to?(:all?) && params(parameters).all?(&:blank?)
         return true if parent.nil?
         parent.should_validate?(parameters)
       end
 
       def requires(*attrs, &block)
-        return new_scope(attrs, &block) if block_given?
+        orig_attrs = attrs.clone
 
         validations = { presence: true }
         validations.merge!(attrs.pop) if attrs.last.is_a?(Hash)
-
-        push_declared_params(attrs)
+        validations[:type] ||= Array if block_given?
         validates(attrs, validations)
+
+        block_given? ? new_scope(orig_attrs, &block) :
+          push_declared_params(attrs)
       end
 
       def optional(*attrs, &block)
-        return new_scope(attrs, true, &block) if block_given?
+        orig_attrs = attrs
 
         validations = {}
         validations.merge!(attrs.pop) if attrs.last.is_a?(Hash)
-
-        push_declared_params(attrs)
+        validations[:type] ||= Array if block_given?
         validates(attrs, validations)
+
+        block_given? ? new_scope(orig_attrs, true, &block) :
+          push_declared_params(attrs)
       end
 
       def group(element, &block)
@@ -132,9 +137,11 @@ def params(params)
         params = @parent.params(params) if @parent
         if @element
           if params.is_a?(Array)
-            params = params.map { |el| el[@element] || {} }
-          else
+            params = params.map { |el| el[@element] || {} }.flatten
+          elsif params.is_a?(Hash)
             params = params[@element] || {}
+          else
+            params = {}
           end
         end
         params
@@ -154,8 +161,9 @@ def push_declared_params(attrs)
       private
 
       def new_scope(attrs, optional = false, &block)
-        raise ArgumentError unless attrs.size == 1
-        ParamsScope.new(api: @api, element: attrs.first, parent: self, optional: optional, &block)
+        opts = attrs[1] || { type: Array }
+        raise ArgumentError unless opts.keys.to_set.subset? [:type].to_set
+        ParamsScope.new(api: @api, element: attrs.first, parent: self, optional: optional, type: opts[:type], &block)
       end
 
       # Pushes declared params to parent or settings
@@ -237,7 +245,7 @@ def reset_validations!
       end
 
       def params(&block)
-        ParamsScope.new(api: self, &block)
+        ParamsScope.new(api: self, type: Hash, &block)
       end
 
       def document_attribute(names, opts)

lib/grape/validations/coerce.rb

@@ -6,6 +6,7 @@ class API
   module Validations
     class CoerceValidator < SingleOptionValidator
       def validate_param!(attr_name, params)
+        raise Grape::Exceptions::Validation, param: @scope.full_name(attr_name), message_key: :coerce unless params.is_a? Hash
         new_value = coerce_value(@option, params[attr_name])
         if valid_type?(new_value)
           params[attr_name] = new_value
@@ -45,6 +46,10 @@ def valid_type?(val)
       end
 
       def coerce_value(type, val)
+        # Don't coerce things other than nil to Arrays or Hashes
+        return val || [] if type == Array
+        return val || {} if type == Hash
+
         converter = Virtus::Attribute.build(type)
         converter.coerce(val)
 

lib/grape/validations/presence.rb

@@ -7,7 +7,7 @@ def validate!(params)
       end
 
       def validate_param!(attr_name, params)
-        unless params.has_key?(attr_name)
+        unless params.respond_to?(:has_key?) && params.has_key?(attr_name)
           raise Grape::Exceptions::Validation, param: @scope.full_name(attr_name), message_key: :presence
         end
       end

spec/grape/api_spec.rb

@@ -1851,8 +1851,10 @@ def self.call(object, env)
       subject.routes.map { |route|
         route.route_params
       }.should eq [{
+        "group1"         => { required: true, type: "Array" },
         "group1[param1]" => { required: false, desc: "group1 param1 desc" },
         "group1[param2]" => { required: true, desc: "group1 param2 desc" },
+        "group2"         => { required: true, type: "Array" },
         "group2[param1]" => { required: false, desc: "group2 param1 desc" },
         "group2[param2]" => { required: true, desc: "group2 param2 desc" }
       }]
@@ -1872,6 +1874,7 @@ def self.call(object, env)
         { description: "nesting",
           params: {
             "root_param" => { required: true, desc: "root param" },
+            "nested" => { required: true, type: "Array" },
             "nested[nested_param]" => { required: true, desc: "nested param" }
           }
         }

spec/grape/endpoint_spec.rb

@@ -177,7 +177,7 @@ def app
         requires :first
         optional :second
         optional :third, default: 'third-default'
-        group :nested do
+        optional :nested, type: Hash do
           optional :fourth
         end
       end
@@ -214,6 +214,16 @@ def app
     end
 
     it 'builds nested params when given array' do
+      subject.get '/dummy' do
+      end
+      subject.params do
+        requires :first
+        optional :second
+        optional :third, default: 'third-default'
+        optional :nested, type: Array do
+          optional :fourth
+        end
+      end
       subject.get '/declared' do
         declared(params)[:nested].size.should == 2
         ""

spec/grape/validations/coerce_spec.rb

@@ -191,7 +191,7 @@ class User
 
       it 'Nests integers' do
         subject.params do
-          group :integers do
+          requires :integers, type: Hash do
             requires :int, coerce: Integer
           end
         end

spec/grape/validations/default_spec.rb

@@ -42,7 +42,12 @@ class API < Grape::API
         end
 
         params do
-          group :foo do
+          # NOTE: The :foo parameter could be made required with json body
+          # params, and then an empty hash would be valid. With query parameters
+          # it must be optional if it isn't provided at all, as otherwise
+          # the validaton for the Hash itself fails because there is no such
+          # thing as an empty hash.
+          optional :foo, type: Hash do
             optional :bar, default: 'foo-bar'
           end
         end

spec/grape/validations/presence_spec.rb

@@ -28,20 +28,22 @@ class API < Grape::API
         end
 
         params do
-          group :user do
-            requires :first_name, :last_name
+          requires :user, type: Hash do
+            requires :first_name
+            requires :last_name
           end
         end
         get '/nested' do
           "Nested"
         end
 
         params do
-          group :admin do
+          requires :admin, type: Hash do
             requires :admin_name
-            group :super do
-              group :user do
-                requires :first_name, :last_name
+            requires :super, type: Hash do
+              requires :user, type: Hash do
+                requires :first_name
+                requires :last_name
               end
             end
           end
@@ -96,7 +98,7 @@ def app
   it 'validates nested parameters' do
     get '/nested'
     last_response.status.should == 400
-    last_response.body.should == '{"error":"user[first_name] is missing"}'
+    last_response.body.should == '{"error":"user is missing, user[first_name] is missing, user[last_name] is missing"}'
 
     get '/nested', user: { first_name: "Billy" }
     last_response.status.should == 400
@@ -110,19 +112,19 @@ def app
   it 'validates triple nested parameters' do
     get '/nested_triple'
     last_response.status.should == 400
-    last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user][first_name] is missing"}'
+    last_response.body.should include '{"error":"admin is missing'
 
     get '/nested_triple', user: { first_name: "Billy" }
     last_response.status.should == 400
-    last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user][first_name] is missing"}'
+    last_response.body.should include '{"error":"admin is missing'
 
     get '/nested_triple', admin: { super: { first_name: "Billy" } }
     last_response.status.should == 400
-    last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user][first_name] is missing"}'
+    last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user] is missing, admin[super][user][first_name] is missing, admin[super][user][last_name] is missing"}'
 
     get '/nested_triple', super: { user: { first_name: "Billy", last_name: "Bob" } }
     last_response.status.should == 400
-    last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user][first_name] is missing"}'
+    last_response.body.should include '{"error":"admin is missing'
 
     get '/nested_triple', admin: { super: { user: { first_name: "Billy" } } }
     last_response.status.should == 400