From 5b51c90ed122eefeb95d415d5fff673196706fa6 Mon Sep 17 00:00:00 2001 From: Alex Hornung Date: Wed, 1 Jan 2014 08:01:58 +0000 Subject: [PATCH] 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. --- CHANGELOG.md | 1 + lib/grape.rb | 1 + lib/grape/validations.rb | 32 ++- lib/grape/validations/coerce.rb | 5 + lib/grape/validations/presence.rb | 2 +- spec/grape/api_spec.rb | 3 + spec/grape/endpoint_spec.rb | 12 +- spec/grape/validations/coerce_spec.rb | 2 +- spec/grape/validations/default_spec.rb | 7 +- spec/grape/validations/presence_spec.rb | 24 +- spec/grape/validations_spec.rb | 287 +++++++++++++++++++++--- spec/support/content_type_helpers.rb | 11 + 12 files changed, 330 insertions(+), 57 deletions(-) create mode 100644 spec/support/content_type_helpers.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 649edb44f7..7874f2f5c8 100644 --- 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 index 06623d2bcb..67bd009cf9 100644 --- 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 index 7ceb7c24b4..886cddab1a 100644 --- 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 index 59243bd7f1..b11c1726db 100644 --- 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 index b4a92da023..bb3e8761e7 100644 --- 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 index fd6bf555a1..53404e7010 100644 --- 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 index 47b9cd9615..faac50bb20 100644 --- 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 index 321ad998dc..af185de920 100644 --- 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 index c0afe7d462..0bae92210b 100644 --- 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 index f2cc115abb..42fad2aa63 100644 --- a/spec/grape/validations/presence_spec.rb +++ b/spec/grape/validations/presence_spec.rb @@ -28,8 +28,9 @@ 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 @@ -37,11 +38,12 @@ class API < Grape::API 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 diff --git a/spec/grape/validations_spec.rb b/spec/grape/validations_spec.rb index 90e9df2cd8..12824ceccc 100644 --- a/spec/grape/validations_spec.rb +++ b/spec/grape/validations_spec.rb @@ -78,13 +78,67 @@ def app end end - context 'required with a block' do + context 'required with an Array block' do before do + subject.params do + requires :items, type: Array do + requires :key + end + end + subject.get '/required' do + 'required works' + end + end + + it 'errors when param not present' do + get '/required' + last_response.status.should == 400 + last_response.body.should == 'items is missing' + end + + it "errors when param is not an Array" do + get '/required', items: "hello" + last_response.status.should == 400 + last_response.body.should == 'items is invalid, items[key] is missing' + + get '/required', items: { key: 'foo' } + last_response.status.should == 400 + last_response.body.should == 'items is invalid' + end + + it "doesn't throw a missing param when param is present" do + get '/required', items: [{ key: 'hello' }, { key: 'world' }] + last_response.status.should == 200 + last_response.body.should == 'required works' + end + + it "doesn't allow any key in the options hash other than type" do + expect { + subject.params do + requires(:items, desc: 'Foo') do + requires :key + end + end + }.to raise_error ArgumentError + end + + it 'adds to declared parameters' do subject.params do requires :items do requires :key end end + subject.settings[:declared_params].should == [items: [:key]] + end + end + + context 'required with a Hash block' do + before do + subject.params do + requires :items, type: Hash do + requires :key + end + end subject.get '/required' do 'required works' end @@ -93,16 +147,26 @@ def app it 'errors when param not present' do get '/required' last_response.status.should == 400 - last_response.body.should == 'items[key] is missing' + last_response.body.should == 'items is missing, items[key] is missing' + end + + it "errors when param is not a Hash" do + get '/required', items: "hello" + last_response.status.should == 400 + last_response.body.should == 'items is invalid, items[key] is missing' + + get '/required', items: [{ key: 'foo' }] + last_response.status.should == 400 + last_response.body.should == 'items is invalid' end it "doesn't throw a missing param when param is present" do - get '/required', items: [key: 'hello', key: 'world'] + get '/required', items: { key: 'hello' } last_response.status.should == 200 last_response.body.should == 'required works' end - it "doesn't allow more than one parameter" do + it "doesn't allow any key in the options hash other than type" do expect { subject.params do requires(:items, desc: 'Foo') do @@ -137,7 +201,7 @@ def app it 'errors when param not present' do get '/required' last_response.status.should == 400 - last_response.body.should == 'items[key] is missing' + last_response.body.should == 'items is missing' end it "doesn't throw a missing param when param is present" do @@ -161,7 +225,7 @@ def app subject.params do group :children do requires :name - group :mother do + group :parents do requires :name end end @@ -173,8 +237,8 @@ def app it 'can handle new scopes within child elements' do get '/within_array', children: [ - { name: 'John', mother: { name: 'Jane' } }, - { name: 'Joe', mother: { name: 'Josie' } } + { name: 'John', parents: [{ name: 'Jane' }, { name: 'Bob' }] }, + { name: 'Joe', parents: [{ name: 'Josie' }] } ] last_response.status.should == 200 last_response.body.should == 'within array works' @@ -182,27 +246,180 @@ def app it 'errors when a parameter is not present' do get '/within_array', children: [ - { name: 'Jim', mother: {} }, - { name: 'Job', mother: { name: 'Joy' } } + { name: 'Jim', parents: [{}] }, + { name: 'Job', parents: [{ name: 'Joy' }] } ] + # NOTE: with body parameters in json or XML or similar this + # should actually fail with: children[parents][name] is missing. last_response.status.should == 400 - last_response.body.should == 'children[mother][name] is missing' + last_response.body.should == 'children[parents] is missing' end it 'safely handles empty arrays and blank parameters' do + # NOTE: with body parameters in json or XML or similar this + # should actually return 200, since an empty array is valid. get '/within_array', children: [] last_response.status.should == 400 - last_response.body.should == 'children[name] is missing, children[mother][name] is missing' + last_response.body.should == 'children is missing' get '/within_array', children: [name: 'Jay'] last_response.status.should == 400 - last_response.body.should == 'children[mother][name] is missing' + last_response.body.should == 'children[parents] is missing' + end + + it "errors when param is not an Array" do + # NOTE: would be nicer if these just returned 'children is invalid' + get '/within_array', children: "hello" + last_response.status.should == 400 + last_response.body.should == 'children is invalid, children[name] is missing, children[parents] is missing, children[parents] is invalid, children[parents][name] is missing' + + get '/within_array', children: { name: 'foo' } + last_response.status.should == 400 + last_response.body.should == 'children is invalid, children[parents] is missing' + + get '/within_array', children: [name: 'Jay', parents: { name: 'Fred' }] + last_response.status.should == 400 + last_response.body.should == 'children[parents] is invalid' end end - context 'optional with a block' do + context 'with block param' do before do subject.params do - optional :items do + requires :planets do + requires :name + end + end + subject.get '/req' do + 'within array works' + end + subject.put '/req' do + '' + end + + subject.params do + group :stars do + requires :name + end + end + subject.get '/grp' do + 'within array works' + end + subject.put '/grp' do + '' + end + + subject.params do + requires :name + optional :moons do + requires :name + end + end + subject.get '/opt' do + 'within array works' + end + subject.put '/opt' do + '' + end + end + + it 'requires defaults to Array type' do + get '/req', planets: "Jupiter, Saturn" + last_response.status.should == 400 + last_response.body.should == 'planets is invalid, planets[name] is missing' + + get '/req', planets: { name: 'Jupiter' } + last_response.status.should == 400 + last_response.body.should == 'planets is invalid' + + get '/req', planets: [{ name: 'Venus' }, { name: 'Mars' }] + last_response.status.should == 200 + + put_with_json '/req', planets: [] + last_response.status.should == 200 + end + + it 'optional defaults to Array type' do + get '/opt', name: "Jupiter", moons: "Europa, Ganymede" + last_response.status.should == 400 + last_response.body.should == 'moons is invalid, moons[name] is missing' + + get '/opt', name: "Jupiter", moons: { name: 'Ganymede' } + last_response.status.should == 400 + last_response.body.should == 'moons is invalid' + + get '/opt', name: "Jupiter", moons: [{ name: 'Io' }, { name: 'Callisto' }] + last_response.status.should == 200 + + put_with_json '/opt', name: "Venus" + last_response.status.should == 200 + + put_with_json '/opt', name: "Mercury", moons: [] + last_response.status.should == 200 + end + + it 'group defaults to Array type' do + get '/grp', stars: "Sun" + last_response.status.should == 400 + last_response.body.should == 'stars is invalid, stars[name] is missing' + + get '/grp', stars: { name: 'Sun' } + last_response.status.should == 400 + last_response.body.should == 'stars is invalid' + + get '/grp', stars: [{ name: 'Sun' }] + last_response.status.should == 200 + + put_with_json '/grp', stars: [] + last_response.status.should == 200 + end + end + + context 'validation within arrays with JSON' do + before do + subject.params do + group :children do + requires :name + group :parents do + requires :name + end + end + end + subject.put '/within_array' do + 'within array works' + end + end + + it 'can handle new scopes within child elements' do + put_with_json '/within_array', children: [ + { name: 'John', parents: [{ name: 'Jane' }, { name: 'Bob' }] }, + { name: 'Joe', parents: [{ name: 'Josie' }] } + ] + last_response.status.should == 200 + last_response.body.should == 'within array works' + end + + it 'errors when a parameter is not present' do + put_with_json '/within_array', children: [ + { name: 'Jim', parents: [{}] }, + { name: 'Job', parents: [{ name: 'Joy' }] } + ] + last_response.status.should == 400 + last_response.body.should == 'children[parents][name] is missing' + end + + it 'safely handles empty arrays and blank parameters' do + put_with_json '/within_array', children: [] + last_response.status.should == 200 + put_with_json '/within_array', children: [name: 'Jay'] + last_response.status.should == 400 + last_response.body.should == 'children[parents] is missing' + end + end + + context 'optional with an Array block' do + before do + subject.params do + optional :items, type: Array do requires :key end end @@ -218,17 +435,27 @@ def app end it "doesn't throw a missing param when both group and param are given" do - get '/optional_group', items: { key: 'foo' } + get '/optional_group', items: [{ key: 'foo' }] last_response.status.should == 200 last_response.body.should == 'optional group works' end it "errors when group is present, but required param is not" do - get '/optional_group', items: { not_key: 'foo' } + get '/optional_group', items: [{ not_key: 'foo' }] last_response.status.should == 400 last_response.body.should == 'items[key] is missing' end + it "errors when param is present but isn't an Array" do + get '/optional_group', items: "hello" + last_response.status.should == 400 + last_response.body.should == 'items is invalid, items[key] is missing' + + get '/optional_group', items: { key: 'foo' } + last_response.status.should == 400 + last_response.body.should == 'items is invalid' + end + it 'adds to declared parameters' do subject.params do optional :items do @@ -239,13 +466,13 @@ def app end end - context 'nested optional blocks' do + context 'nested optional Array blocks' do before do subject.params do - optional :items do + optional :items, type: Array do requires :key - optional(:optional_subitems) { requires :value } - requires(:required_subitems) { requires :value } + optional(:optional_subitems, type: Array) { requires :value } + requires(:required_subitems, type: Array) { requires :value } end end subject.get('/nested_optional_group') { 'nested optional group works' } @@ -258,35 +485,35 @@ def app end it 'does internal validations if the outer group is present' do - get '/nested_optional_group', items: { key: 'foo' } + get '/nested_optional_group', items: [{ key: 'foo' }] last_response.status.should == 400 - last_response.body.should == 'items[required_subitems][value] is missing' + last_response.body.should == 'items[required_subitems] is missing' - get '/nested_optional_group', items: { key: 'foo', required_subitems: { value: 'bar' } } + get '/nested_optional_group', items: [{ key: 'foo', required_subitems: [{ value: 'bar' }] }] last_response.status.should == 200 last_response.body.should == 'nested optional group works' end it 'handles deep nesting' do - get '/nested_optional_group', items: { key: 'foo', required_subitems: { value: 'bar' }, optional_subitems: { not_value: 'baz' } } + get '/nested_optional_group', items: [{ key: 'foo', required_subitems: [{ value: 'bar' }], optional_subitems: [{ not_value: 'baz' }] }] last_response.status.should == 400 last_response.body.should == 'items[optional_subitems][value] is missing' - get '/nested_optional_group', items: { key: 'foo', required_subitems: { value: 'bar' }, optional_subitems: { value: 'baz' } } + get '/nested_optional_group', items: [{ key: 'foo', required_subitems: [{ value: 'bar' }], optional_subitems: [{ value: 'baz' }] }] last_response.status.should == 200 last_response.body.should == 'nested optional group works' end it 'handles validation within arrays' do - get '/nested_optional_group', items: [key: 'foo'] + get '/nested_optional_group', items: [{ key: 'foo' }] last_response.status.should == 400 - last_response.body.should == 'items[required_subitems][value] is missing' + last_response.body.should == 'items[required_subitems] is missing' - get '/nested_optional_group', items: [key: 'foo', required_subitems: { value: 'bar' }] + get '/nested_optional_group', items: [{ key: 'foo', required_subitems: [{ value: 'bar' }] }] last_response.status.should == 200 last_response.body.should == 'nested optional group works' - get '/nested_optional_group', items: [key: 'foo', required_subitems: { value: 'bar' }, optional_subitems: { not_value: 'baz' }] + get '/nested_optional_group', items: [{ key: 'foo', required_subitems: [{ value: 'bar' }], optional_subitems: [{ not_value: 'baz' }] }] last_response.status.should == 400 last_response.body.should == 'items[optional_subitems][value] is missing' end diff --git a/spec/support/content_type_helpers.rb b/spec/support/content_type_helpers.rb new file mode 100644 index 0000000000..443e40808a --- /dev/null +++ b/spec/support/content_type_helpers.rb @@ -0,0 +1,11 @@ +module ContentTypeHelpers + %w(put patch post delete).each do |method| + define_method :"#{method}_with_json" do |uri, params = {}, env = {}, &block| + params = params.to_json + env["CONTENT_TYPE"] ||= "application/json" + send(method, uri, params, env, &block) + end + end +end + +include(ContentTypeHelpers)