From 1113ac0148549d549a503c9519e442ee8ed6a2a7 Mon Sep 17 00:00:00 2001 From: Steven Schmid Date: Tue, 6 Aug 2013 13:14:17 +0200 Subject: [PATCH 1/3] improve collection of validation errors rename Grape::Exceptions::Validations to Grape::Exceptions::ValidationError for clarity make it necessary to supply :param when raising Grape::Exception::Validation use rails as inspiration for the structure of the validation errors --- CHANGELOG.md | 2 +- README.md | 10 +++--- lib/grape.rb | 2 +- lib/grape/endpoint.rb | 2 +- lib/grape/exceptions/base.rb | 2 +- lib/grape/exceptions/validation.rb | 5 +-- lib/grape/exceptions/validation_errors.rb | 42 +++++++++++++++++++++++ lib/grape/exceptions/validations.rb | 15 -------- lib/grape/locale/en.yml | 7 ++-- lib/grape/validations/coerce.rb | 3 +- lib/grape/validations/presence.rb | 3 +- lib/grape/validations/regexp.rb | 3 +- spec/grape/validations/coerce_spec.rb | 7 ++-- spec/grape/validations/presence_spec.rb | 24 ++++++------- spec/grape/validations/zh-CN.yml | 7 ++-- spec/grape/validations_spec.rb | 24 ++++++------- 16 files changed, 93 insertions(+), 65 deletions(-) create mode 100644 lib/grape/exceptions/validation_errors.rb delete mode 100644 lib/grape/exceptions/validations.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 2caf6b89b2..942503d16c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ Next Release ============ #### Features -* [#433](https://github.com/intridea/grape/issues/433): Validation errors are now collected and a `Grape::Exceptions::Validations` is raised. The individual validation errors can be accessed via `Grape::Exceptions::Validations#errors` - [@stevschmid](https://github.com/stevschmid). +* [#433](https://github.com/intridea/grape/issues/433): Validation errors are now collected and `Grape::Exceptions::ValidationErrors` is raised. The individual validation errors can be accessed via `#errors` - [@stevschmid](https://github.com/stevschmid). #### Fixes diff --git a/README.md b/README.md index f07d4e32f6..c609cd4099 100644 --- a/README.md +++ b/README.md @@ -384,7 +384,7 @@ The `namespace` method has a number of aliases, including: `group`, `resource`, class AlphaNumeric < Grape::Validations::Validator def validate_param!(attr_name, params) unless params[attr_name] =~ /^[[:alnum:]]+$/ - throw :error, status: 400, message: "#{attr_name}: must consist of alpha-numeric characters" + raise Grape::Exceptions::Validation, param: @scope.full_name(attr_name), message: "must consist of alpha-numeric characters" end end end @@ -402,7 +402,7 @@ You can also create custom classes that take parameters. 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 at the most #{@option} characters long" + raise Grape::Exceptions::Validation, param: @scope.full_name(attr_name), message: "must be at the most #{@option} characters long" end end end @@ -416,12 +416,12 @@ end ### Validation Errors -Validation and coercion errors are collected and an exception of type `Grape::Exceptions::Validations` is raised. +Validation and coercion errors are collected and an exception of type `Grape::Exceptions::ValidationErrors` is raised. If the exception goes uncaught it will respond with a status of 400 and an error message. -You can rescue a `Grape::Exceptions::Validations` and respond with a custom response. +You can rescue a `Grape::Exceptions::ValidationErrors` and respond with a custom response. ```ruby -rescue_from Grape::Exceptions::Validations do |e| +rescue_from Grape::Exceptions::ValidationErrors do |e| Rack::Response.new({ 'status' => e.status, 'message' => e.message, diff --git a/lib/grape.rb b/lib/grape.rb index 4205923abd..c2528baeef 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -31,7 +31,7 @@ module Grape module Exceptions autoload :Base, 'grape/exceptions/base' autoload :Validation, 'grape/exceptions/validation' - autoload :Validations, 'grape/exceptions/validations' + autoload :ValidationErrors, 'grape/exceptions/validation_errors' autoload :MissingVendorOption, 'grape/exceptions/missing_vendor_option' autoload :MissingMimeType, 'grape/exceptions/missing_mime_type' autoload :MissingOption, 'grape/exceptions/missing_option' diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 1b39f2599c..87a0fdec10 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -401,7 +401,7 @@ def run(env) end if validation_errors.any? - raise Grape::Exceptions::Validations, errors: validation_errors + raise Grape::Exceptions::ValidationErrors, errors: validation_errors end run_filters after_validations diff --git a/lib/grape/exceptions/base.rb b/lib/grape/exceptions/base.rb index a917fa65f8..bcbd67a902 100644 --- a/lib/grape/exceptions/base.rb +++ b/lib/grape/exceptions/base.rb @@ -54,7 +54,7 @@ def translate_attribute(key, options = {}) end def translate_message(key, options = {}) - translate("#{BASE_MESSAGES_KEY}.#{key}", {:default => '' }.merge(options)) + translate("#{BASE_MESSAGES_KEY}.#{key}", { :default => '' }.merge(options)) end def translate(key, options = {}) diff --git a/lib/grape/exceptions/validation.rb b/lib/grape/exceptions/validation.rb index 17566a4733..85f21444bf 100644 --- a/lib/grape/exceptions/validation.rb +++ b/lib/grape/exceptions/validation.rb @@ -6,9 +6,10 @@ class Validation < Grape::Exceptions::Base attr_accessor :param def initialize(args = {}) - @param = args[:param].to_s if args.has_key? :param + raise "Param is missing:" unless args.has_key? :param + @param = args[:param].to_s attribute = translate_attribute(@param) - args[:message] = translate_message(args[:message_key], :attribute => attribute) if args.has_key? :message_key + args[:message] = translate_message(args[:message_key]) if args.has_key? :message_key super end end diff --git a/lib/grape/exceptions/validation_errors.rb b/lib/grape/exceptions/validation_errors.rb new file mode 100644 index 0000000000..0c2e45f28a --- /dev/null +++ b/lib/grape/exceptions/validation_errors.rb @@ -0,0 +1,42 @@ +require 'grape/exceptions/base' + +module Grape + module Exceptions + class ValidationErrors < Grape::Exceptions::Base + include Enumerable + + attr_reader :errors + + def initialize(args = {}) + @errors = {} + args[:errors].each do |validation_error| + @errors[validation_error.param] ||= [] + @errors[validation_error.param] << validation_error.message + end + super message: full_messages.join(', '), status: 400 + end + + def each + errors.each_pair do |attribute, messages| + messages.each do |message| + yield attribute, message + end + end + end + + private + + def full_messages + map { |attribute, message| full_message(attribute, message) } + end + + def full_message(attribute, message) + I18n.t(:"grape.errors.format", { + default: "%{attribute} %{message}", + attribute: translate_attribute(attribute), + message: message + }) + end + end + end +end diff --git a/lib/grape/exceptions/validations.rb b/lib/grape/exceptions/validations.rb deleted file mode 100644 index fb2176d6d9..0000000000 --- a/lib/grape/exceptions/validations.rb +++ /dev/null @@ -1,15 +0,0 @@ -require 'grape/exceptions/base' - -module Grape - module Exceptions - class Validations < Grape::Exceptions::Base - attr_accessor :errors - - def initialize(args = {}) - @errors = args[:errors] - errors_as_sentence = @errors.collect { |e| e.message }.join(', ') - super message: errors_as_sentence, status: 400 - end - end - end -end diff --git a/lib/grape/locale/en.yml b/lib/grape/locale/en.yml index d77b13e42a..8239b33197 100644 --- a/lib/grape/locale/en.yml +++ b/lib/grape/locale/en.yml @@ -1,10 +1,11 @@ en: grape: errors: + format: ! '%{attribute} %{message}' messages: - coerce: 'invalid parameter: %{attribute}' - presence: 'missing parameter: %{attribute}' - regexp: 'invalid parameter: %{attribute}' + coerce: 'is invalid' + presence: 'is missing' + regexp: 'is invalid' missing_vendor_option: problem: 'missing :vendor option.' summary: 'when version using header, you must specify :vendor option. ' diff --git a/lib/grape/validations/coerce.rb b/lib/grape/validations/coerce.rb index 9144da680f..e5532646e7 100644 --- a/lib/grape/validations/coerce.rb +++ b/lib/grape/validations/coerce.rb @@ -12,8 +12,7 @@ def validate_param!(attr_name, params) if valid_type?(new_value) params[attr_name] = new_value else - raise Grape::Exceptions::Validation, :status => 400, - :param => @scope.full_name(attr_name), :message_key => :coerce + raise Grape::Exceptions::Validation, :param => @scope.full_name(attr_name), :message_key => :coerce end end diff --git a/lib/grape/validations/presence.rb b/lib/grape/validations/presence.rb index 167ac132c8..021416c40a 100644 --- a/lib/grape/validations/presence.rb +++ b/lib/grape/validations/presence.rb @@ -3,8 +3,7 @@ module Validations class PresenceValidator < Validator def validate_param!(attr_name, params) unless params.has_key?(attr_name) - raise Grape::Exceptions::Validation, :status => 400, - :param => @scope.full_name(attr_name), :message_key => :presence + raise Grape::Exceptions::Validation, :param => @scope.full_name(attr_name), :message_key => :presence end end end diff --git a/lib/grape/validations/regexp.rb b/lib/grape/validations/regexp.rb index a80830f8e6..fe26586b97 100644 --- a/lib/grape/validations/regexp.rb +++ b/lib/grape/validations/regexp.rb @@ -4,8 +4,7 @@ module Validations class RegexpValidator < SingleOptionValidator def validate_param!(attr_name, params) if params[attr_name] && !( params[attr_name].to_s =~ @option ) - raise Grape::Exceptions::Validation, :status => 400, - :param => @scope.full_name(attr_name), :message_key => :regexp + raise Grape::Exceptions::Validation, :param => @scope.full_name(attr_name), :message_key => :regexp end end end diff --git a/spec/grape/validations/coerce_spec.rb b/spec/grape/validations/coerce_spec.rb index 47eb3c563a..62af2fdecf 100644 --- a/spec/grape/validations/coerce_spec.rb +++ b/spec/grape/validations/coerce_spec.rb @@ -19,13 +19,14 @@ def app; subject end I18n.locale = :en end + it 'error on malformed input' do subject.params { requires :int, :type => Integer } subject.get '/single' do 'int works'; end get '/single', :int => '43a' last_response.status.should == 400 - last_response.body.should == 'invalid parameter: int' + last_response.body.should == 'int is invalid' get '/single', :int => '43' last_response.status.should == 200 @@ -38,7 +39,7 @@ def app; subject end get 'array', { :ids => ['1', '2', 'az'] } last_response.status.should == 400 - last_response.body.should == 'invalid parameter: ids' + last_response.body.should == 'ids is invalid' get 'array', { :ids => ['1', '2', '890'] } last_response.status.should == 200 @@ -60,7 +61,7 @@ class User get '/user', :user => "32" last_response.status.should == 400 - last_response.body.should == 'invalid parameter: user' + last_response.body.should == 'user is invalid' get '/user', :user => { :id => 32, :name => 'Bob' } last_response.status.should == 200 diff --git a/spec/grape/validations/presence_spec.rb b/spec/grape/validations/presence_spec.rb index 9726db9be1..b0e6362cb5 100644 --- a/spec/grape/validations/presence_spec.rb +++ b/spec/grape/validations/presence_spec.rb @@ -66,13 +66,13 @@ def app it 'validates id' do post '/' last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: id"}' + last_response.body.should == '{"error":"id is missing"}' io = StringIO.new('{"id" : "a56b"}') post '/', {}, 'rack.input' => io, 'CONTENT_TYPE' => 'application/json', 'CONTENT_LENGTH' => io.length - last_response.body.should == '{"error":"invalid parameter: id"}' + last_response.body.should == '{"error":"id is invalid"}' last_response.status.should == 400 io = StringIO.new('{"id" : 56}') @@ -86,11 +86,11 @@ def app it 'validates name, company' do get('/') last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: name"}' + last_response.body.should == '{"error":"name is missing"}' get('/', :name => "Bob") last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: company"}' + last_response.body.should == '{"error":"company is missing"}' get('/', :name => "Bob", :company => "TestCorp") last_response.status.should == 200 @@ -100,11 +100,11 @@ def app it 'validates nested parameters' do get('/nested') last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: user[first_name]"}' + last_response.body.should == '{"error":"user[first_name] is missing"}' get('/nested', :user => {:first_name => "Billy"}) last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: user[last_name]"}' + last_response.body.should == '{"error":"user[last_name] is missing"}' get('/nested', :user => {:first_name => "Billy", :last_name => "Bob"}) last_response.status.should == 200 @@ -114,27 +114,27 @@ def app it 'validates triple nested parameters' do get('/nested_triple') last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: admin[admin_name], missing parameter: admin[super][user][first_name]"}' + last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user][first_name] is missing"}' get('/nested_triple', :user => {:first_name => "Billy"}) last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: admin[admin_name], missing parameter: admin[super][user][first_name]"}' + last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user][first_name] is missing"}' get('/nested_triple', :admin => {:super => {:first_name => "Billy"}}) last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: admin[admin_name], missing parameter: admin[super][user][first_name]"}' + last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user][first_name] is missing"}' get('/nested_triple', :super => {:user => {:first_name => "Billy", :last_name => "Bob"}}) last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: admin[admin_name], missing parameter: admin[super][user][first_name]"}' + last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user][first_name] is missing"}' get('/nested_triple', :admin => {:super => {:user => {:first_name => "Billy"}}}) last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: admin[admin_name], missing parameter: admin[super][user][last_name]"}' + last_response.body.should == '{"error":"admin[admin_name] is missing, admin[super][user][last_name] is missing"}' get('/nested_triple', :admin => { :admin_name => 'admin', :super => {:user => {:first_name => "Billy"}}}) last_response.status.should == 400 - last_response.body.should == '{"error":"missing parameter: admin[super][user][last_name]"}' + last_response.body.should == '{"error":"admin[super][user][last_name] is missing"}' get('/nested_triple', :admin => { :admin_name => 'admin', :super => {:user => {:first_name => "Billy", :last_name => "Bob"}}}) last_response.status.should == 200 diff --git a/spec/grape/validations/zh-CN.yml b/spec/grape/validations/zh-CN.yml index 1d3f0f37e7..dbcca6ad3d 100644 --- a/spec/grape/validations/zh-CN.yml +++ b/spec/grape/validations/zh-CN.yml @@ -1,9 +1,10 @@ zh-CN: grape: errors: + format: ! '%{attribute}%{message}' attributes: age: 年龄 messages: - coerce: '%{attribute}格式不正确' - presence: '请填写{attribute}' - regexp: '%{attribute}格式不正确' + coerce: '格式不正确' + presence: '请填写' + regexp: '格式不正确' diff --git a/spec/grape/validations_spec.rb b/spec/grape/validations_spec.rb index 52667acf07..3940c7721c 100644 --- a/spec/grape/validations_spec.rb +++ b/spec/grape/validations_spec.rb @@ -12,7 +12,7 @@ def app; subject end get '/optional', { :a_number => 'string' } last_response.status.should == 400 - last_response.body.should == 'invalid parameter: a_number' + last_response.body.should == 'a_number is invalid' get '/optional', { :a_number => 45 } last_response.status.should == 200 @@ -43,7 +43,7 @@ def app; subject end it 'errors when param not present' do get '/required' last_response.status.should == 400 - last_response.body.should == 'missing parameter: key' + last_response.body.should == 'key is missing' end it "doesn't throw a missing param when param is present" do @@ -71,7 +71,7 @@ def app; subject end it 'errors when param not present' do get '/required' last_response.status.should == 400 - last_response.body.should == 'missing parameter: items[key]' + last_response.body.should == 'items[key] is missing' end it "doesn't throw a missing param when param is present" do @@ -99,8 +99,8 @@ def app; subject end it 'throws the validation errors' do get '/two_required' last_response.status.should == 400 - last_response.body.should =~ /missing parameter: yolo/ - last_response.body.should =~ /missing parameter: swag/ + last_response.body.should =~ /yolo is missing/ + last_response.body.should =~ /swag is missing/ end end @@ -109,7 +109,7 @@ module CustomValidations class Customvalidator < Grape::Validations::Validator def validate_param!(attr_name, params) unless params[attr_name] == 'im custom' - raise Grape::Exceptions::Validation, :status => 400, :message => "#{attr_name}: is not custom!" + raise Grape::Exceptions::Validation, :param => @scope.full_name(attr_name), :message => "is not custom!" end end end @@ -128,7 +128,7 @@ def validate_param!(attr_name, params) get '/optional_custom', { :custom => 'im wrong' } last_response.status.should == 400 - last_response.body.should == 'custom: is not custom!' + last_response.body.should == 'custom is not custom!' end it "skips validation when parameter isn't present" do @@ -142,7 +142,7 @@ def validate_param!(attr_name, params) get '/optional_custom', { :custom => 123 } last_response.status.should == 400 - last_response.body.should == 'custom: is not custom!' + last_response.body.should == 'custom is not custom!' end end @@ -155,7 +155,7 @@ def validate_param!(attr_name, params) 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!' + last_response.body.should == 'custom is not custom!' get '/required_custom', { :custom => 'im custom' } last_response.status.should == 200 @@ -165,7 +165,7 @@ def validate_param!(attr_name, params) it 'validates when param is not present' do get '/required_custom' last_response.status.should == 400 - last_response.body.should == 'missing parameter: custom, custom: is not custom!' + last_response.body.should == 'custom is missing, custom is not custom!' end context 'nested namespaces' do @@ -197,13 +197,13 @@ def validate_param!(attr_name, params) specify 'the parent namespace uses the validator' do get '/nested/one', { :custom => 'im wrong, validate me'} last_response.status.should == 400 - last_response.body.should == 'custom: is not custom!' + last_response.body.should == 'custom is not custom!' end specify 'the nested namesapce inherits the custom validator' do get '/nested/nested/two', { :custom => 'im wrong, validate me'} last_response.status.should == 400 - last_response.body.should == 'custom: is not custom!' + last_response.body.should == 'custom is not custom!' end specify 'peer namesapces does not have the validator' do From dbcfcc41d384e9dcc3c1083bc2b215f6f07c962e Mon Sep 17 00:00:00 2001 From: Steven Schmid Date: Tue, 20 Aug 2013 17:58:56 +0200 Subject: [PATCH 2/3] collect the validation errors objects instead of their message when calling #errors --- lib/grape/exceptions/validation.rb | 13 +++++++++++-- lib/grape/exceptions/validation_errors.rb | 14 +++++++------- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/lib/grape/exceptions/validation.rb b/lib/grape/exceptions/validation.rb index 85f21444bf..2482882495 100644 --- a/lib/grape/exceptions/validation.rb +++ b/lib/grape/exceptions/validation.rb @@ -7,11 +7,20 @@ class Validation < Grape::Exceptions::Base def initialize(args = {}) raise "Param is missing:" unless args.has_key? :param - @param = args[:param].to_s - attribute = translate_attribute(@param) + @param = args[:param] args[:message] = translate_message(args[:message_key]) if args.has_key? :message_key super end + + # remove all the unnecessary stuff from Grape::Exceptions::Base like status + # and headers when converting a validation error to json or string + def as_json(*a) + self.to_s + end + + def to_s + message + end end end end diff --git a/lib/grape/exceptions/validation_errors.rb b/lib/grape/exceptions/validation_errors.rb index 0c2e45f28a..8093112ce0 100644 --- a/lib/grape/exceptions/validation_errors.rb +++ b/lib/grape/exceptions/validation_errors.rb @@ -11,15 +11,15 @@ def initialize(args = {}) @errors = {} args[:errors].each do |validation_error| @errors[validation_error.param] ||= [] - @errors[validation_error.param] << validation_error.message + @errors[validation_error.param] << validation_error end super message: full_messages.join(', '), status: 400 end def each - errors.each_pair do |attribute, messages| - messages.each do |message| - yield attribute, message + errors.each_pair do |attribute, errors| + errors.each do |error| + yield attribute, error end end end @@ -27,14 +27,14 @@ def each private def full_messages - map { |attribute, message| full_message(attribute, message) } + map { |attribute, error| full_message(attribute, error) } end - def full_message(attribute, message) + def full_message(attribute, error) I18n.t(:"grape.errors.format", { default: "%{attribute} %{message}", attribute: translate_attribute(attribute), - message: message + message: error.message }) end end From 6a26fb60053600c0e73b6b130dc4642393e74044 Mon Sep 17 00:00:00 2001 From: Steven Schmid Date: Tue, 20 Aug 2013 18:10:07 +0200 Subject: [PATCH 3/3] update changelog and readme --- CHANGELOG.md | 2 +- README.md | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 903422d4e8..34134ddbb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ Next Release * [#450](https://github.com/intridea/grape/pull/450): Added option to pass an exception handler lambda as an argument to `rescue_from` - [@robertopedroso](https://github.com/robertopedroso). * [#443](https://github.com/intridea/grape/pull/443): Let `requires` and `optional` take blocks that initialize new scopes - [@asross](https://github.com/asross). * [#452](https://github.com/intridea/grape/pull/452): Added `with` as a hash option to specify handlers for `rescue_from` and `error_formatter` [@robertopedroso](https://github.com/robertopedroso). -* [#433](https://github.com/intridea/grape/issues/433), [#462](https://github.com/intridea/grape/issues/462): API change: validation errors are now collected and a `Grape::Exceptions::ValidationErrors` is raised; the individual validation errors be accessed via `Grape::Exceptions::ValidationErrors#errors` (hash in the format ``{"parameter_name": [validation_error1, validation_error2, ...]}``) - [@stevschmid](https://github.com/stevschmid). +* [#433](https://github.com/intridea/grape/issues/433), [#462](https://github.com/intridea/grape/issues/462): API change: validation errors are now collected and `Grape::Exceptions::ValidationErrors` is raised - [@stevschmid](https://github.com/stevschmid). * Your contribution here. #### Fixes diff --git a/README.md b/README.md index 4962e16215..8a0a5e5214 100644 --- a/README.md +++ b/README.md @@ -434,6 +434,8 @@ rescue_from Grape::Exceptions::ValidationErrors do |e| end ``` +The validation errors are grouped by parameter name and can be accessed via ``Grape::Exceptions::ValidationErrors#errors``. + ### I18n Grape supports I18n for parameter-related error messages, but will fallback to English if