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)