From 67ba1ef3be54ae523214b6bdab672035e207a0fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CPavel?= <“pavel.jurasek@bcgdv.com”> Date: Sat, 7 May 2016 12:09:30 +0200 Subject: [PATCH 1/4] Fix potential false positive results --- spec/parameter_raise_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/parameter_raise_spec.rb b/spec/parameter_raise_spec.rb index 997b063..d4a1cfe 100644 --- a/spec/parameter_raise_spec.rb +++ b/spec/parameter_raise_spec.rb @@ -5,7 +5,7 @@ it 'should raise error when option is specified' do expect { get('/raise/validation/required') - }.to raise_error + }.to raise_error Sinatra::Param::InvalidParameterError end end @@ -13,13 +13,13 @@ params = {a: 1, b: 2, c: 3} expect { get('/raise/one_of/3', params) - }.to raise_error + }.to raise_error Sinatra::Param::InvalidParameterError end it 'should raise error when no parameters are specified' do params = {} expect { get('/raise/any_of', params) - }.to raise_error + }.to raise_error Sinatra::Param::InvalidParameterError end end From fdc81bcf9283bc52491e554a4cec4275846dcb19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CPavel?= <“pavel.jurasek@bcgdv.com”> Date: Sun, 8 May 2016 14:54:18 +0200 Subject: [PATCH 2/4] Add errors handling without exception as option. --- .gitignore | 2 + .rspec | 1 + Gemfile | 1 + README.md | 37 +++ lib/sinatra/param.rb | 170 ++++++++++++-- sinatra-param.gemspec | 1 + spec/dummy/app.rb | 12 +- spec/dummy/app_with_flash.rb | 299 +++++++++++++++++++++++++ spec/flash/parameter_flash_spec.rb | 39 ++++ spec/parameter_exclusivity_spec.rb | 4 +- spec/parameter_inclusivity_spec.rb | 4 +- spec/parameter_raise_spec.rb | 4 +- spec/parameter_spec.rb | 4 +- spec/parameter_transformations_spec.rb | 4 +- spec/parameter_type_coercion_spec.rb | 4 +- spec/parameter_validations_spec.rb | 4 +- spec/spec_helper.rb | 5 +- 17 files changed, 550 insertions(+), 45 deletions(-) create mode 100644 spec/dummy/app_with_flash.rb create mode 100644 spec/flash/parameter_flash_spec.rb diff --git a/.gitignore b/.gitignore index 4aa6c6e..f213dc9 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,8 @@ /doc/ /rdoc/ +.byebug_history + # Environment /.bundle/ /lib/bundler/man/ diff --git a/.rspec b/.rspec index 0912718..a76097e 100644 --- a/.rspec +++ b/.rspec @@ -1,2 +1,3 @@ --color --order random +--require spec_helper.rb diff --git a/Gemfile b/Gemfile index b4e2a20..51dfb5b 100644 --- a/Gemfile +++ b/Gemfile @@ -1,3 +1,4 @@ source "https://rubygems.org" +gem 'byebug' gemspec diff --git a/README.md b/README.md index d5eaf51..9b38e87 100644 --- a/README.md +++ b/README.md @@ -118,6 +118,43 @@ param :y, String any_of :x, :y ``` +### Errors without exception + +If you need to consume error with custom business logic (e.g. do not show error one by one but all errors at once). You can do this with this PR. + +Set flag to change slightly way how to sinatra param use "bang methods" + + +```ruby +class AppWithFlash < Sinatra::Base + helpers Sinatra::Param + + configure do + set :ruby_best_practice_sinatra_param, true + end +end +``` + +after seting up this flag will be modified helper method "param", "one_of" and "any_of" and will be created 3 additional methods "param!", "one_of!" and "any_of!" as it's standard in Ruby community methods with bang character in the end will raise error same as for instance save vs save! in ActiveRecord. + +So now param helper do not raise exception and halt execution but return value if error happend otherwise just nil value represent "nothing to show". + +```ruby +error_for_a = param(:a, String) +``` + +error_for_a will contain same error message as people are use to to get from standard Sinatra param library. + +as bonus we create helper method for collect or errors and remove nil values. + +```ruby +errors = errors_for_params do |errors| + param(:a, String) + param(:b, String) + param(:c, String) +end +``` + ### Exceptions By default, when a parameter precondition fails, `Sinatra::Param` will `halt 400` with an error message: diff --git a/lib/sinatra/param.rb b/lib/sinatra/param.rb index 7762b84..bbb5e39 100644 --- a/lib/sinatra/param.rb +++ b/lib/sinatra/param.rb @@ -12,32 +12,94 @@ class InvalidParameterError < StandardError end def param(name, type, options = {}) - name = name.to_s + if (settings.ruby_best_practice_sinatra_param rescue false) + param_without_exception(name, type, options) + else + param_with_exception(name, type, options) + end + end - return unless params.member?(name) or options[:default] or options[:required] + def errors_for_params(&block) + errors ||= [] + block.call(errors).compact + end + + def param!(name, type, options = {}) + param_with_exception(name, type, options) + end + + def one_of(*args) + if (settings.ruby_best_practice_sinatra_param rescue false) + one_of_without_exception(*args) + else + one_of_with_exception(*args) + end + end + + def one_of!(*args) + one_of_with_exception(*args) + end + + def any_of(*args) + if (settings.ruby_best_practice_sinatra_param rescue false) + any_of_without_exception(*args) + else + any_of_with_exception(*args) + end + end + + def any_of!(*args) + any_of_with_exception(*args) + end + + private + + def any_of_without_exception(*args) + options = args.last.is_a?(Hash) ? args.pop : {} + names = args.collect(&:to_s) + + return unless names.length >= 2 + + error = validate_any_of(params, names, options) + + error.nil? ? nil : error + end + + def any_of_with_exception(*args) + options = args.last.is_a?(Hash) ? args.pop : {} + names = args.collect(&:to_s) + + return unless names.length >= 2 begin - params[name] = coerce(params[name], type, options) - params[name] = (options[:default].call if options[:default].respond_to?(:call)) || options[:default] if params[name].nil? and options[:default] - params[name] = options[:transform].to_proc.call(params[name]) if params[name] and options[:transform] - validate!(params[name], options) + validate_any_of!(params, names, options) rescue InvalidParameterError => exception if options[:raise] or (settings.raise_sinatra_param_exceptions rescue false) - exception.param, exception.options = name, options + exception.param, exception.options = names, options raise exception end - error = exception.to_s - + error = "Invalid parameters [#{names.join(', ')}]" if content_type and content_type.match(mime_type(:json)) - error = {message: error, errors: {name => exception.message}}.to_json + error = {message: error, errors: {names => exception.message}}.to_json end halt 400, error end end - def one_of(*args) + def one_of_without_exception(*args) + options = args.last.is_a?(Hash) ? args.pop : {} + names = args.collect(&:to_s) + + return unless names.length >= 2 + + error = validate_one_of(params, names, options) + + error.nil? ? nil : error + end + + def one_of_with_exception(*args) options = args.last.is_a?(Hash) ? args.pop : {} names = args.collect(&:to_s) @@ -60,31 +122,51 @@ def one_of(*args) end end - def any_of(*args) - options = args.last.is_a?(Hash) ? args.pop : {} - names = args.collect(&:to_s) + def param_without_exception(name, type, options = {}) + name = name.to_s - return unless names.length >= 2 + return unless params.member?(name) or options[:default] or options[:required] begin - validate_any_of!(params, names, options) + params[name] = coerce(params[name], type, options) + + params[name] = (options[:default].call if options[:default].respond_to?(:call)) || options[:default] if params[name].nil? and options[:default] + params[name] = options[:transform].to_proc.call(params[name]) if params[name] and options[:transform] + + errors = validate(params[name], options) + rescue InvalidParameterError => exception + errors = [exception.message] + end + + errors && errors.empty? ? nil : { name => errors } + end + + def param_with_exception(name, type, options = {}) + name = name.to_s + + return unless params.member?(name) or options[:default] or options[:required] + + begin + params[name] = coerce(params[name], type, options) + params[name] = (options[:default].call if options[:default].respond_to?(:call)) || options[:default] if params[name].nil? and options[:default] + params[name] = options[:transform].to_proc.call(params[name]) if params[name] and options[:transform] + validate!(params[name], options) rescue InvalidParameterError => exception if options[:raise] or (settings.raise_sinatra_param_exceptions rescue false) - exception.param, exception.options = names, options + exception.param, exception.options = name, options raise exception end - error = "Invalid parameters [#{names.join(', ')}]" + error = exception.to_s + if content_type and content_type.match(mime_type(:json)) - error = {message: error, errors: {names => exception.message}}.to_json + error = {message: error, errors: {name => exception.message}}.to_json end halt 400, error end end - private - def coerce(param, type, options = {}) begin return nil if param.nil? @@ -142,6 +224,44 @@ def validate!(param, options) end end + def validate(param, options) + options.each_with_object([]) do |(key, value), errors| + case key + when :required + errors << "Parameter is required" if value && param.nil? + when :blank + errors << "Parameter cannot be blank" if !value && case param + when String + !(/\S/ === param) + when Array, Hash + param.empty? + else + param.nil? + end + when :format + errors << "Parameter must be a string if using the format validation" unless param.kind_of?(String) + errors << "Parameter must match format #{value}" unless param =~ value + when :is + errors << "Parameter must be #{value}" unless param === value + when :in, :within, :range + errors << "Parameter must be within #{value}" unless param.nil? || case value + when Range + value.include?(param) + else + Array(value).include?(param) + end + when :min + errors << "Parameter cannot be less than #{value}" unless param.nil? || value <= param + when :max + errors << "Parameter cannot be greater than #{value}" unless param.nil? || value >= param + when :min_length + errors << "Parameter cannot have length less than #{value}" unless param.nil? || value <= param.length + when :max_length + errors << "Parameter cannot have length greater than #{value}" unless param.nil? || value >= param.length + end + end + end + def validate_one_of!(params, names, options) raise InvalidParameterError, "Only one of [#{names.join(', ')}] is allowed" if names.count{|name| present?(params[name])} > 1 end @@ -150,6 +270,14 @@ def validate_any_of!(params, names, options) raise InvalidParameterError, "One of parameters [#{names.join(', ')}] is required" if names.count{|name| present?(params[name])} < 1 end + def validate_one_of(params, names, options) + return "Only one of [#{names.join(', ')}] is allowed" if names.count{|name| present?(params[name])} > 1 + end + + def validate_any_of(params, names, options) + return "One of parameters [#{names.join(', ')}] is required" if names.count{|name| present?(params[name])} < 1 + end + # ActiveSupport #present? and #blank? without patching Object def present?(object) !blank?(object) diff --git a/sinatra-param.gemspec b/sinatra-param.gemspec index fb2dcf1..069b066 100644 --- a/sinatra-param.gemspec +++ b/sinatra-param.gemspec @@ -15,6 +15,7 @@ Gem::Specification.new do |s| s.add_dependency "sinatra", "~> 1.3" s.add_development_dependency "rake" + s.add_development_dependency "rack" s.add_development_dependency "rspec" s.add_development_dependency "rack-test" s.add_development_dependency "simplecov" diff --git a/spec/dummy/app.rb b/spec/dummy/app.rb index f577123..c3e357e 100644 --- a/spec/dummy/app.rb +++ b/spec/dummy/app.rb @@ -7,8 +7,10 @@ class App < Sinatra::Base helpers Sinatra::Param - set :show_exceptions, false - set :raise_errors, true + configure do + set :show_exceptions, false + set :raise_errors, true + end before do content_type :json @@ -238,6 +240,12 @@ class App < Sinatra::Base params.to_json end + get '/flash/validations/required' do + param :arg, Integer, required: true, raise: true, min: 12 + + expect(flash.keys).to be('') + end + get '/raise/one_of/3' do param :a, String param :b, String diff --git a/spec/dummy/app_with_flash.rb b/spec/dummy/app_with_flash.rb new file mode 100644 index 0000000..524213d --- /dev/null +++ b/spec/dummy/app_with_flash.rb @@ -0,0 +1,299 @@ +require 'sinatra/base' +require 'sinatra/param' +require 'date' +require 'time' +require 'json' + +class AppWithFlash < Sinatra::Base + helpers Sinatra::Param + + configure do + set :show_exceptions, false + set :raise_errors, true + set :ruby_best_practice_sinatra_param, true + end + + before do + content_type :json + end + + get '/' do + param! :a, String + param! :b, String, required: true + param! :c, String, default: 'test' + param! :d, String + + params.to_json + end + + get '/keys/stringify' do + param! :q, String, transform: :upcase + + params['q'] + end + + get '/coerce/string' do + params['arg'] = params['arg'].to_i + param! :arg, String + params.to_json + end + + get '/coerce/integer' do + param! :arg, Integer + params.to_json + end + + get '/coerce/float' do + param! :arg, Float + params.to_json + end + + get '/coerce/time' do + param! :arg, Time + params.to_json + end + + get '/coerce/date' do + param! :arg, Date + params.to_json + end + + get '/coerce/datetime' do + param! :arg, DateTime + params.to_json + end + + get '/coerce/array' do + param! :arg, Array + params.to_json + end + + get '/coerce/hash' do + param! :arg, Hash + params.to_json + end + + get '/coerce/boolean' do + param! :arg, Boolean + params.to_json + end + + get '/default' do + param! :sort, String, default: "title" + params.to_json + end + + get '/default/hash' do + param! :attributes, Hash, default: {} + params.to_json + end + + get '/default/proc' do + param! :year, Integer, default: proc { 2014 } + params.to_json + end + + get '/default/boolean/true' do + param! :arg, Boolean, default: true + params.to_json + end + + get '/default/boolean/false' do + param! :arg, Boolean, default: false + params.to_json + end + + get '/transform' do + param! :order, String, transform: :upcase + params.to_json + end + + get '/transform/required' do + param! :order, String, required: true, transform: :upcase + params.to_json + end + + get '/validation/required' do + param! :arg, String, required: true + params.to_json + end + + get '/validation/blank/string' do + param! :arg, String, blank: false + end + + get '/validation/blank/array' do + param! :arg, Array, blank: false + end + + get '/validation/blank/hash' do + param! :arg, Hash, blank: false + end + + get '/validation/blank/other' do + param! :arg, Class, blank: false + end + + get '/validation/nonblank/string' do + param! :arg, String, blank: true + end + + get '/validation/format/9000' do + param! :arg, Integer, format: /9000/ + params.to_json + end + + get '/validation/format/hello' do + param! :arg, String, format: /hello/ + params.to_json + end + + get '/validation/is' do + param! :arg, String, is: 'foo' + params.to_json + end + + get '/validation/in' do + param! :arg, String, in: ['ASC', 'DESC'] + params.to_json + end + + get '/validation/within' do + param! :arg, Integer, within: 1..10 + params.to_json + end + + get '/validation/range' do + param! :arg, Integer, range: 1..10 + params.to_json + end + + get '/validation/min' do + param! :arg, Integer, min: 12 + params.to_json + end + + get '/validation/max' do + param! :arg, Integer, max: 20 + params.to_json + end + + get '/validation/min_length' do + param! :arg, String, min_length: 5 + params.to_json + end + + get '/validation/max_length' do + param! :arg, String, max_length: 10 + params.to_json + end + + get '/one_of/1' do + param! :a, String + param! :b, String + param! :c, String + + one_of :a + + { + message: 'OK' + }.to_json + end + + get '/one_of/2' do + param! :a, String + param! :b, String + param! :c, String + + one_of :a, :b + + { + message: 'OK' + }.to_json + end + + get '/one_of/3' do + param! :a, String + param! :b, String + param! :c, String + + one_of :a, :b, :c + + { + message: 'OK' + }.to_json + end + + get '/any_of' do + param! :a, String + param! :b, String + param! :c, String + + any_of :a, :b, :c + + { + message: 'OK' + }.to_json + end + + get '/raise/validation/required' do + param! :arg, String, required: true, raise: true + params.to_json + end + + get '/errors/validation/required' do + errors = errors_for_params do |errors| + errors << param(:arg, Integer, required: true, min: 12, in: 2..100) + errors << param(:arg2, Integer, required: true, min: 0, in: 0..100) + end + errors.to_json + end + + get '/raise/one_of/3' do + param! :a, String + param! :b, String + param! :c, String + + one_of :a, :b, :c, raise: true + + { + message: 'OK' + }.to_json + end + + get '/errors/one_of/3' do + errors = errors_for_params do |errors| + errors << param(:a, String) + errors << param(:b, String) + errors << param(:c, String) + + errors << one_of(:a, :b, :c) + end + + errors.to_json + end + + get '/raise/any_of' do + param! :a, String + param! :b, String + param! :c, String + + any_of :a, :b, :c, raise: true + + { + message: 'OK' + }.to_json + end + + get '/errors/any_of' do + errors = errors_for_params do |errors| + param(:a, String) + param(:b, String) + param(:c, String) + + errors << any_of(:a, :b, :c) + end + + errors.to_json + end +end diff --git a/spec/flash/parameter_flash_spec.rb b/spec/flash/parameter_flash_spec.rb new file mode 100644 index 0000000..18c53df --- /dev/null +++ b/spec/flash/parameter_flash_spec.rb @@ -0,0 +1,39 @@ +RSpec.describe 'Flash' do + def app + @app = AppWithFlash + end + + describe 'set up error' do + it 'should return simple json message with all errors' do + get('/errors/validation/required', arg: 1, arg2: 0) + + json_response = JSON.parse(last_response.body) + + expect(json_response[0].keys.first).to eq('arg') + expect(json_response[0]['arg']).to include( + 'Parameter cannot be less than 12', + 'Parameter must be within 2..100' + ) + end + end + + it 'should raise error when more than one parameter is specified' do + params = {a: 1, b: 2, c: 3} + + get('/errors/one_of/3', params) + + json_response = JSON.parse(last_response.body) + + expect(json_response[0]).to eq('Only one of [a, b, c] is allowed') + end + + it 'should raise error when no parameters are specified' do + params = {} + + get('/errors/any_of', params) + + json_response = JSON.parse(last_response.body) + + expect(json_response[0]).to eq('One of parameters [a, b, c] is required') + end +end diff --git a/spec/parameter_exclusivity_spec.rb b/spec/parameter_exclusivity_spec.rb index b23ee60..d9b98a6 100644 --- a/spec/parameter_exclusivity_spec.rb +++ b/spec/parameter_exclusivity_spec.rb @@ -1,6 +1,4 @@ -require 'spec_helper' - -describe 'Parameter Sets' do +RSpec.describe 'Parameter Sets' do describe 'one_of' do it 'returns 400 on requests that contain more than one mutually exclusive parameter' do params = [ diff --git a/spec/parameter_inclusivity_spec.rb b/spec/parameter_inclusivity_spec.rb index 4e3f502..85383ce 100644 --- a/spec/parameter_inclusivity_spec.rb +++ b/spec/parameter_inclusivity_spec.rb @@ -1,6 +1,4 @@ -require 'spec_helper' - -describe 'Parameter Sets' do +RSpec.describe 'Parameter Sets' do describe 'any_of' do it 'returns 400 on requests that contain fewer than one required parameter' do get('/any_of', {}) do |response| diff --git a/spec/parameter_raise_spec.rb b/spec/parameter_raise_spec.rb index d4a1cfe..28831ba 100644 --- a/spec/parameter_raise_spec.rb +++ b/spec/parameter_raise_spec.rb @@ -1,6 +1,4 @@ -require 'spec_helper' - -describe 'Exception' do +RSpec.describe 'Exception' do describe 'raise' do it 'should raise error when option is specified' do expect { diff --git a/spec/parameter_spec.rb b/spec/parameter_spec.rb index 50957e1..bdc503f 100644 --- a/spec/parameter_spec.rb +++ b/spec/parameter_spec.rb @@ -1,6 +1,4 @@ -require 'spec_helper' - -describe 'Parameter' do +RSpec.describe 'Parameter' do it 'only sets parameters present in request or with a default value' do get('/', a: 'a', b: 'b') do |response| response_body = JSON.parse(response.body) diff --git a/spec/parameter_transformations_spec.rb b/spec/parameter_transformations_spec.rb index b2d1891..86f5360 100644 --- a/spec/parameter_transformations_spec.rb +++ b/spec/parameter_transformations_spec.rb @@ -1,6 +1,4 @@ -require 'spec_helper' - -describe 'Parameter Transformations' do +RSpec.describe 'Parameter Transformations' do describe 'default' do it 'sets a default value when none is given' do get('/default') do |response| diff --git a/spec/parameter_type_coercion_spec.rb b/spec/parameter_type_coercion_spec.rb index 3db4cbe..e47beb3 100644 --- a/spec/parameter_type_coercion_spec.rb +++ b/spec/parameter_type_coercion_spec.rb @@ -1,6 +1,4 @@ -require 'spec_helper' - -describe 'Parameter Types' do +RSpec.describe 'Parameter Types' do describe 'String' do it 'coerces strings' do get('/coerce/string', arg: '1234') do |response| diff --git a/spec/parameter_validations_spec.rb b/spec/parameter_validations_spec.rb index 46114e4..dcc97ec 100644 --- a/spec/parameter_validations_spec.rb +++ b/spec/parameter_validations_spec.rb @@ -1,6 +1,4 @@ -require 'spec_helper' - -describe 'Parameter Validations' do +RSpec.describe 'Parameter Validations' do describe 'required' do it 'returns 400 on requests without required fields' do get('/validation/required') do |response| diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b91e9c9..a6e5a3e 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -12,9 +12,12 @@ require 'rack/test' require 'dummy/app' +require 'dummy/app_with_flash' def app App end -include Rack::Test::Methods +RSpec.configure do |conf| + conf.include Rack::Test::Methods +end From 87e456402be9ce9617b6aedd44a0a0c29413b201 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CPavel?= <“pavel.jurasek@bcgdv.com”> Date: Sun, 8 May 2016 14:56:58 +0200 Subject: [PATCH 3/4] Remove byebug gem. --- Gemfile | 1 - 1 file changed, 1 deletion(-) diff --git a/Gemfile b/Gemfile index 51dfb5b..b4e2a20 100644 --- a/Gemfile +++ b/Gemfile @@ -1,4 +1,3 @@ source "https://rubygems.org" -gem 'byebug' gemspec From 97efda127baa016d0cd11c4e5ef32c3548084f78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CPavel?= <“pavel.jurasek@bcgdv.com”> Date: Sun, 8 May 2016 14:59:35 +0200 Subject: [PATCH 4/4] Add travis --- .travis.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..67afc72 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,6 @@ + +language: ruby +rvm: + - 1.9.3 +before_install: + - gem install bundler \ No newline at end of file