Address #543 - raise proper validation errors on array/hash types

* Also adds a :type option to group/requires/optional with block, which can either be Array or Hash. It defaults to Array. * Fixes (2) and (3) as mentioned in #543 * There is a quirk around query parameters: an empty array should pass validation if the array itself is required, but with how query parameters work, it doesn't. It does work fine with body parameters using JSON or similar, which do have a concept of an empty array.
ruby-grape · Jan 2, 2014 · 5b51c90 · 5b51c90
1 parent 5a904ba
commit 5b51c90
Show file tree

Hide file tree

Showing 12 changed files with 330 additions and 57 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,7 @@ Next Release
 * [#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).
 * [#544](https://github.com/intridea/grape/pull/544): `rescue_from` now handles subclasses of exceptions by default - [@xevix](https://github.com/xevix).
+* [#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

diff --git a/lib/grape.rb b/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'

diff --git a/lib/grape/validations.rb b/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)

diff --git a/lib/grape/validations/coerce.rb b/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)
 

diff --git a/lib/grape/validations/presence.rb b/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

diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb
@@ -1913,8 +1913,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" }
       }]
@@ -1934,6 +1936,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" }
           }
         }

diff --git a/spec/grape/endpoint_spec.rb b/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
         ""

diff --git a/spec/grape/validations/coerce_spec.rb b/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

diff --git a/spec/grape/validations/default_spec.rb b/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

diff --git a/spec/grape/validations/presence_spec.rb b/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