From 114cc1062af017ff94bef584ebc16872289032d3 Mon Sep 17 00:00:00 2001 From: namusyaka Date: Wed, 27 Jan 2016 13:10:12 +0900 Subject: [PATCH] Deny to pass values except Proc, Symbol or String to :with option of rescue_from --- CHANGELOG.md | 1 + README.md | 4 ++-- UPGRADING.md | 12 ++++++++++++ lib/grape/dsl/request_response.rb | 8 ++++++-- spec/grape/api_spec.rb | 15 --------------- spec/grape/dsl/request_response_spec.rb | 12 +++++++++--- 6 files changed, 30 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 225a45342e..0f07be4b82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ * [#1252](https://github.com/ruby-grape/grape/pull/1252): Allow default to be a subset or equal to allowed values without raising IncompatibleOptionValues - [@jeradphelps](https://github.com/jeradphelps). * [#1255](https://github.com/ruby-grape/grape/pull/1255): Allow param type definition in route_param - [@namusyaka](https://github.com/namusyaka) * [#1257](https://github.com/ruby-grape/grape/pull/1257): Allow to pass method name to :with option of rescue_from - [@namusyaka](https://github.com/namusyaka) +* [#1257](https://github.com/ruby-grape/grape/pull/1257): Deny to pass values except Proc, Symbol or String to :with option of rescue_from - [@namusyaka](https://github.com/namusyaka) * Your contribution here. #### Fixes diff --git a/README.md b/README.md index 82927e5dd2..f170fd662c 100644 --- a/README.md +++ b/README.md @@ -1771,7 +1771,7 @@ end The `rescue_from` block must return a `Rack::Response` object, call `error!` or re-raise an exception. -The `with` keyword is available as `rescue_from` options, it can be passed method name or `Rack::Response` object. +The `with` keyword is available as `rescue_from` options, it can be passed method name or Proc object. ```ruby class Twitter::API < Grape::API @@ -1783,7 +1783,7 @@ class Twitter::API < Grape::API end rescue_from :all, with: :server_error! - rescue_from ArgumentError, with: Rack::Response.new('rescued with a method', 400) + rescue_from ArgumentError, with: -> { Rack::Response.new('rescued with a method', 400) } end ``` diff --git a/UPGRADING.md b/UPGRADING.md index a56d20d170..96d3decbd9 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,18 @@ Upgrading Grape ### Upgrading to >= 0.15.0 +#### Changes to availability of `:with` option of `rescue_from` method + +The `:with` option of `rescue_from` does not accept value except Proc, String or Symbol now. + +If you have been depending the old behavior, you should use lambda block instead. + +```ruby +class API < Grape::API + rescue_from :all, with: -> { Rack::Response.new('rescued with a method', 400) } +end +``` + #### Changes to behavior of `after` method of middleware on error The `after` method of the middleware is now called on error. diff --git a/lib/grape/dsl/request_response.rb b/lib/grape/dsl/request_response.rb index cdd161a025..3a38aad1ca 100644 --- a/lib/grape/dsl/request_response.rb +++ b/lib/grape/dsl/request_response.rb @@ -105,8 +105,12 @@ def rescue_from(*args, &block) end options = args.extract_options! - if options.key?(:with) && !options[:with].instance_of?(Symbol) - handler ||= proc { options[:with] } + if options.key?(:with) + if options[:with].instance_of?(Proc) + handler ||= options[:with] + elsif !(options[:with].instance_of?(Symbol) || options[:with].instance_of?(String)) + fail ArgumentError, ":with option expected Symbol, String or Proc, got #{options[:with].class}" + end end if args.include?(:all) diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 8abae123ab..24215db996 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -1503,21 +1503,6 @@ class CommunicationError < StandardError; end end end - describe '.rescue_from klass, with: method' do - it 'rescues an error with the specified message' do - def rescue_arg_error - Rack::Response.new('rescued with a method', 400) - end - - subject.rescue_from ArgumentError, with: rescue_arg_error - subject.get('/rescue_method') { fail ArgumentError } - - get '/rescue_method' - expect(last_response.status).to eq(400) - expect(last_response.body).to eq('rescued with a method') - end - end - describe '.rescue_from klass, with: :method_name' do it 'rescues an error with the specified method name' do subject.helpers do diff --git a/spec/grape/dsl/request_response_spec.rb b/spec/grape/dsl/request_response_spec.rb index 94f5a822a1..b7230942b3 100644 --- a/spec/grape/dsl/request_response_spec.rb +++ b/spec/grape/dsl/request_response_spec.rb @@ -124,9 +124,14 @@ def self.imbue(key, value) end it 'sets a rescue handler declared through :with option' do + with_block = -> { 'hello' } expect(subject).to receive(:namespace_inheritable).with(:rescue_all, true) expect(subject).to receive(:namespace_inheritable).with(:all_rescue_handler, an_instance_of(Proc)) - subject.rescue_from :all, with: 'ExampleHandler' + subject.rescue_from :all, with: with_block + end + + it 'abort if :with option value is not Symbol, String or Proc' do + expect { subject.rescue_from :all, with: 1234 }.to raise_error(ArgumentError, ':with option expected Symbol, String or Proc, got Fixnum') end end @@ -158,9 +163,10 @@ def self.imbue(key, value) end it 'sets a rescue handler declared through :with option for each key in hash' do + with_block = -> { 'hello' } expect(subject).to receive(:namespace_stackable).with(:rescue_handlers, StandardError => an_instance_of(Proc)) - expect(subject).to receive(:namespace_stackable).with(:rescue_options, with: 'ExampleHandler') - subject.rescue_from StandardError, with: 'ExampleHandler' + expect(subject).to receive(:namespace_stackable).with(:rescue_options, with: with_block) + subject.rescue_from StandardError, with: with_block end end end