Merge pull request #1405 from hedgesky/errors_handling_improvements

dblock · dblock · commit b9ba3d8e0517 · 2016-06-02T15:48:04.000-04:00
Exceptions aren't rescued in a predictable order
diff --git a/CHANGELOG.md b/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).
diff --git a/README.md b/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.
diff --git a/UPGRADING.md b/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.
diff --git a/lib/grape.rb b/lib/grape.rb
@@ -106,6 +106,7 @@ module Util
     extend ActiveSupport::Autoload
     autoload :InheritableValues
     autoload :StackableValues
+    autoload :ReverseStackableValues
     autoload :InheritableSetting
     autoload :StrictHashConfiguration
     autoload :Registrable
diff --git a/lib/grape/dsl/request_response.rb b/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)
diff --git a/lib/grape/dsl/settings.rb b/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
diff --git a/lib/grape/endpoint.rb b/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)
 
diff --git a/lib/grape/middleware/error.rb b/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
diff --git a/lib/grape/util/inheritable_setting.rb b/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
diff --git a/lib/grape/util/reverse_stackable_values.rb b/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
diff --git a/lib/grape/util/stackable_values.rb b/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
diff --git a/spec/grape/api_spec.rb b/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
diff --git a/spec/grape/dsl/request_response_spec.rb b/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
diff --git a/spec/grape/util/inheritable_setting_spec.rb b/spec/grape/util/inheritable_setting_spec.rb
diff --git a/spec/grape/util/reverse_stackable_values_spec.rb b/spec/grape/util/reverse_stackable_values_spec.rb
