Exceptions aren't rescued in a predictable order

CHANGELOG.md

@@ -11,6 +11,7 @@
 
 #### Fixes
 
+* [#1405](https://github.com/ruby-grape/grape/pull/1405): Fix priority of `rescue_from` clauses applying - [@hedgesky](https://github.com/hedgesky).
 * [#1365](https://github.com/ruby-grape/grape/pull/1365): Fix finding exception handler in error middleware - [@ktimothy](https://github.com/ktimothy).
 * [#1380](https://github.com/ruby-grape/grape/pull/1380): Fix `allow_blank: false` for `Time` attributes with valid values causes `NoMethodError` - [@ipkes](https://github.com/ipkes).
 * [#1384](https://github.com/ruby-grape/grape/pull/1384): Fix parameter validation with an empty optional nested `Array` - [@ipkes](https://github.com/ipkes).

README.md

@@ -1969,6 +1969,30 @@ class Twitter::API < Grape::API
 end
 ```
 
+#### Rescuing exceptions inside namespaces
+
+You could put `rescue_from` clauses inside a namespace and they will take precedence over ones
+defined in the root scope:
+
+```ruby
+class Twitter::API < Grape::API
+ rescue_from ArgumentError do |e|
+ error!("outer")
+ end
+
+ namespace :statuses do
+ rescue_from ArgumentError do |e|
+ error!("inner")
+ end
+ get do
+ raise ArgumentError.new
+ end
+ end
+end
+```
+
+Here `'inner'` will be result of handling occured `ArgumentError`.
+
 #### Unrescuable Exceptions
 
 `Grape::Exceptions::InvalidVersionHeader`, which is raised when the version in the request header doesn't match the currently evaluated version for the endpoint, will _never_ be rescued from a `rescue_from` block (even a `rescue_from :all`) This is because Grape relies on Rack to catch that error and try the next versioned-route for cases where there exist identical Grape endpoints with different versions.

UPGRADING.md

@@ -3,6 +3,11 @@ Upgrading Grape
 
 ### Upgrading to >= 0.16.0
 
+#### Changed priority of `rescue_from` clauses applying
+
+Since 0.16.3 `rescue_from` clauses declared inside namespace would take a priority over ones declared in the root scope.
+This could possibly affect those users who use different `rescue_from` clauses in root scope and in namespaces.
+
 #### Replace rack-mount with new router
 
 The `Route#route_xyz` methods have been deprecated since 0.15.1.

lib/grape.rb

@@ -106,6 +106,7 @@ module Util
  extend ActiveSupport::Autoload
  autoload :InheritableValues
  autoload :StackableValues
+ autoload :ReverseStackableValues
  autoload :InheritableSetting
  autoload :StrictHashConfiguration
  autoload :Registrable

lib/grape/dsl/request_response.rb

@@ -122,7 +122,7 @@ def rescue_from(*args, &block)
  :base_only_rescue_handlers
  end
 
- namespace_stackable handler_type, Hash[args.map { |arg| [arg, handler] }]
+ namespace_reverse_stackable handler_type, Hash[args.map { |arg| [arg, handler] }]
  end
 
  namespace_stackable(:rescue_options, options)

lib/grape/dsl/settings.rb

@@ -94,12 +94,28 @@ def namespace_stackable(key, value = nil)
  get_or_set :namespace_stackable, key, value
  end
 
+ def namespace_reverse_stackable(key, value = nil)
+ get_or_set :namespace_reverse_stackable, key, value
+ end
+
  def namespace_stackable_with_hash(key)
  settings = get_or_set :namespace_stackable, key, nil
  return if settings.blank?
  settings.each_with_object({}) { |value, result| result.deep_merge!(value) }
  end
 
+ def namespace_reverse_stackable_with_hash(key)
+ settings = get_or_set :namespace_reverse_stackable, key, nil
+ return if settings.blank?
+ result = {}
+ settings.each do |setting|
+ setting.each do |field, value|
+ result[field] ||= value
+ end
+ end
+ result
+ end
+
  # (see #unset_global_setting)
  def unset_namespace_stackable(key)
  unset :namespace_stackable, key

lib/grape/endpoint.rb

@@ -256,7 +256,7 @@ def build_stack
  default_error_formatter: namespace_inheritable(:default_error_formatter),
  error_formatters: namespace_stackable_with_hash(:error_formatters),
  rescue_options: namespace_stackable_with_hash(:rescue_options) || {},
- rescue_handlers: namespace_stackable_with_hash(:rescue_handlers) || {},
+ rescue_handlers: namespace_reverse_stackable_with_hash(:rescue_handlers) || {},
  base_only_rescue_handlers: namespace_stackable_with_hash(:base_only_rescue_handlers) || {},
  all_rescue_handler: namespace_inheritable(:all_rescue_handler)
 

lib/grape/middleware/error.rb

@@ -56,7 +56,7 @@ def find_handler(klass)
 
  def rescuable?(klass)
  return false if klass == Grape::Exceptions::InvalidVersionHeader
- options[:rescue_all] || (options[:rescue_handlers] || []).any? { |error, _handler| klass <= error } || (options[:base_only_rescue_handlers] || []).include?(klass)
+ rescue_all? || rescue_class_or_its_ancestor?(klass) || rescue_with_base_only_handler?(klass)
  end
 
  def exec_handler(e, &handler)
@@ -109,6 +109,20 @@ def format_message(message, backtrace)
  throw :error, status: 406, message: "The requested format '#{format}' is not supported." unless formatter
  formatter.call(message, backtrace, options, env)
  end
+
+ private
+
+ def rescue_all?
+ options[:rescue_all]
+ end
+
+ def rescue_class_or_its_ancestor?(klass)
+ (options[:rescue_handlers] || []).any? { |error, _handler| klass <= error }
+ end
+
+ def rescue_with_base_only_handler?(klass)
+ (options[:base_only_rescue_handlers] || []).include?(klass)
+ end
  end
  end
 end

lib/grape/util/inheritable_setting.rb

@@ -3,7 +3,8 @@ module Util
  # A branchable, inheritable settings object which can store both stackable
  # and inheritable values (see InheritableValues and StackableValues).
  class InheritableSetting
- attr_accessor :route, :api_class, :namespace, :namespace_inheritable, :namespace_stackable
+ attr_accessor :route, :api_class, :namespace
+ attr_accessor :namespace_inheritable, :namespace_stackable, :namespace_reverse_stackable
  attr_accessor :parent, :point_in_time_copies
 
  # Retrieve global settings.
@@ -28,6 +29,7 @@ def initialize
  # used with a mount, or should every API::Class be a separate namespace by default?
  self.namespace_inheritable = InheritableValues.new
  self.namespace_stackable = StackableValues.new
+ self.namespace_reverse_stackable = ReverseStackableValues.new
 
  self.point_in_time_copies = []
 
@@ -50,6 +52,7 @@ def inherit_from(parent)
 
  namespace_inheritable.inherited_values = parent.namespace_inheritable
  namespace_stackable.inherited_values = parent.namespace_stackable
+ namespace_reverse_stackable.inherited_values = parent.namespace_reverse_stackable
  self.route = parent.route.merge(route)
 
  point_in_time_copies.map { |cloned_one| cloned_one.inherit_from parent }
@@ -67,6 +70,7 @@ def point_in_time_copy
  new_setting.namespace = namespace.clone
  new_setting.namespace_inheritable = namespace_inheritable.clone
  new_setting.namespace_stackable = namespace_stackable.clone
+ new_setting.namespace_reverse_stackable = namespace_reverse_stackable.clone
  new_setting.route = route.clone
  new_setting.api_class = api_class
 
@@ -87,7 +91,8 @@ def to_hash
  route: route.clone,
  namespace: namespace.to_hash,
  namespace_inheritable: namespace_inheritable.to_hash,
- namespace_stackable: namespace_stackable.to_hash
+ namespace_stackable: namespace_stackable.to_hash,
+ namespace_reverse_stackable: namespace_reverse_stackable.to_hash
  }
  end
  end

lib/grape/util/reverse_stackable_values.rb

@@ -0,0 +1,45 @@
+module Grape
+ module Util
+ class ReverseStackableValues
+ attr_accessor :inherited_values
+ attr_accessor :new_values
+
+ def initialize(inherited_values = {})
+ @inherited_values = inherited_values
+ @new_values = {}
+ end
+
+ def [](name)
+ [].tap do |value|
+ value.concat(@new_values[name] || [])
+ value.concat(@inherited_values[name] || [])
+ end
+ end
+
+ def []=(name, value)
+ @new_values[name] ||= []
+ @new_values[name].push value
+ end
+
+ def delete(key)
+ new_values.delete key
+ end
+
+ def keys
+ (@new_values.keys + @inherited_values.keys).sort.uniq
+ end
+
+ def to_hash
+ keys.each_with_object({}) do |key, result|
+ result[key] = self[key]
+ end
+ end
+
+ def initialize_copy(other)
+ super
+ self.inherited_values = other.inherited_values
+ self.new_values = other.new_values.dup
+ end
+ end
+ end
+end

lib/grape/util/stackable_values.rb

@@ -2,7 +2,7 @@ module Grape
  module Util
  class StackableValues
  attr_accessor :inherited_values
- attr_reader :new_values
+ attr_accessor :new_values
  attr_reader :frozen_values
 
  def initialize(inherited_values = {})
@@ -30,8 +30,6 @@ def delete(key)
  new_values.delete key
  end
 
- attr_writer :new_values
-
  def keys
  (@new_values.keys + @inherited_values.keys).sort.uniq
  end

spec/grape/api_spec.rb

@@ -2638,22 +2638,78 @@ def static
  expect(last_response.body).to eq('yo')
  end
 
- it 'inherits rescues even when some defined by mounted' do
- subject.rescue_from :all do |e|
- rack_response("rescued from #{e.message}", 202)
+ context 'when some rescues are defined by mounted' do
+ it 'inherits parent rescues' do
+ subject.rescue_from :all do |e|
+ rack_response("rescued from #{e.message}", 202)
+ end
+
+ app = Class.new(Grape::API)
+
+ subject.namespace :mounted do
+ app.rescue_from ArgumentError
+ app.get('/fail') { raise 'doh!' }
+ mount app
+ end
+
+ get '/mounted/fail'
+ expect(last_response.status).to eql 202
+ expect(last_response.body).to eq('rescued from doh!')
  end
+ it 'prefers rescues defined by mounted if they rescue similar error class' do
+ subject.rescue_from StandardError do
+ rack_response('outer rescue')
+ end
 
- app = Class.new(Grape::API)
+  app = Class.new(Grape::API)
 
- subject.namespace :mounted do
- app.rescue_from ArgumentError
- app.get('/fail') { raise 'doh!' }
- mount app
+ subject.namespace :mounted do
+ rescue_from StandardError do
+ rack_response('inner rescue')
+ end
+ app.get('/fail') { raise 'doh!' }
+ mount app
+ end
+
+ get '/mounted/fail'
+ expect(last_response.body).to eq('inner rescue')
  end
+ it 'prefers rescues defined by mounted even if outer is more specific' do
+ subject.rescue_from ArgumentError do
+ rack_response('outer rescue')
+ end
 
- get '/mounted/fail'
- expect(last_response.status).to eql 202
- expect(last_response.body).to eq('rescued from doh!')
+ app = Class.new(Grape::API)
+
+ subject.namespace :mounted do
+ rescue_from StandardError do
+ rack_response('inner rescue')
+ end
+ app.get('/fail') { raise ArgumentError.new }
+ mount app
+ end
+
+ get '/mounted/fail'
+ expect(last_response.body).to eq('inner rescue')
+ end
+ it 'prefers more specific rescues defined by mounted' do
+ subject.rescue_from StandardError do
+ rack_response('outer rescue')
+ end
+
+ app = Class.new(Grape::API)
+
+ subject.namespace :mounted do
+ rescue_from ArgumentError do
+ rack_response('inner rescue')
+ end
+ app.get('/fail') { raise ArgumentError.new }
+ mount app
+ end
+
+ get '/mounted/fail'
+ expect(last_response.body).to eq('inner rescue')
+ end
  end
 
  it 'collects the routes of the mounted api' do

spec/grape/dsl/request_response_spec.rb

@@ -145,34 +145,34 @@ def self.imbue(key, value)
 
  describe 'list of exceptions is passed' do
  it 'sets hash of exceptions as rescue handlers' do
- expect(subject).to receive(:namespace_stackable).with(:rescue_handlers, StandardError => nil)
+ expect(subject).to receive(:namespace_reverse_stackable).with(:rescue_handlers, StandardError => nil)
  expect(subject).to receive(:namespace_stackable).with(:rescue_options, {})
  subject.rescue_from StandardError
  end
 
  it 'rescues only base handlers if rescue_subclasses: false option is passed' do
- expect(subject).to receive(:namespace_stackable).with(:base_only_rescue_handlers, StandardError => nil)
+ expect(subject).to receive(:namespace_reverse_stackable).with(:base_only_rescue_handlers, StandardError => nil)
  expect(subject).to receive(:namespace_stackable).with(:rescue_options, rescue_subclasses: false)
  subject.rescue_from StandardError, rescue_subclasses: false
  end
 
  it 'sets given proc as rescue handler for each key in hash' do
  rescue_handler_proc = proc {}
- expect(subject).to receive(:namespace_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc)
+ expect(subject).to receive(:namespace_reverse_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc)
  expect(subject).to receive(:namespace_stackable).with(:rescue_options, {})
  subject.rescue_from StandardError, rescue_handler_proc
  end
 
  it 'sets given block as rescue handler for each key in hash' do
  rescue_handler_proc = proc {}
- expect(subject).to receive(:namespace_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc)
+ expect(subject).to receive(:namespace_reverse_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc)
  expect(subject).to receive(:namespace_stackable).with(:rescue_options, {})
  subject.rescue_from StandardError, &rescue_handler_proc
  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_reverse_stackable).with(:rescue_handlers, StandardError => an_instance_of(Proc))
  expect(subject).to receive(:namespace_stackable).with(:rescue_options, {})
  subject.rescue_from StandardError, with: with_block
  end