diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cbac7de7..a91a8a76f 100644 --- 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 index 56ef74cdf..fe0a995e8 100644 --- 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 index c7bd23925..4429a1425 100644 --- 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 index 41ac3f58e..972368f0d 100644 --- 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 index 9933bb279..e50ec7c3c 100644 --- 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 index e360c89bc..f83d61645 100644 --- 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 index 4accf0a9d..6de3364cb 100644 --- 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 index d50fa2ba9..1e4176f05 100644 --- 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 index 51685362b..2d3021861 100644 --- 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 new file mode 100644 index 000000000..474325f72 --- /dev/null +++ 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 index c3c885221..72612af5e 100644 --- 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 index 474de6ad4..d9c93667d 100644 --- 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 index 82047d4fa..7e89bc0ef 100644 --- 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 index ed54def9f..12653b80d 100644 --- a/spec/grape/util/inheritable_setting_spec.rb +++ b/spec/grape/util/inheritable_setting_spec.rb @@ -12,6 +12,7 @@ module Util settings.namespace[:namespace_thing] = :namespace_foo_bar settings.namespace_inheritable[:namespace_inheritable_thing] = :namespace_inheritable_foo_bar settings.namespace_stackable[:namespace_stackable_thing] = :namespace_stackable_foo_bar + settings.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = :namespace_reverse_stackable_foo_bar settings.route[:route_thing] = :route_foo_bar end end @@ -21,6 +22,7 @@ module Util settings.namespace[:namespace_thing] = :namespace_foo_bar_other settings.namespace_inheritable[:namespace_inheritable_thing] = :namespace_inheritable_foo_bar_other settings.namespace_stackable[:namespace_stackable_thing] = :namespace_stackable_foo_bar_other + settings.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = :namespace_reverse_stackable_foo_bar_other settings.route[:route_thing] = :route_foo_bar_other end end @@ -118,6 +120,16 @@ module Util end end + describe '#namespace_reverse_stackable' do + it 'works with reverse stackable values' do + expect(subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar] + + subject.inherit_from other_parent + + expect(subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar_other] + end + end + describe '#route' do it 'sets a value until the next route' do subject.route[:some_thing] = :foo_bar @@ -184,6 +196,14 @@ module Util expect(cloned_obj.namespace_stackable[:namespace_stackable_thing]).to eq [:namespace_stackable_foo_bar] end + it 'decouples namespace reverse stackable values' do + expect(cloned_obj.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar] + + subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = :other_thing + expect(subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:other_thing, :namespace_reverse_stackable_foo_bar] + expect(cloned_obj.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar] + end + it 'decouples route values' do expect(cloned_obj.route[:route_thing]).to eq :route_foo_bar @@ -202,6 +222,7 @@ module Util subject.namespace[:namespace_thing] = :namespace_foo_bar subject.namespace_inheritable[:namespace_inheritable_thing] = :namespace_inheritable_foo_bar subject.namespace_stackable[:namespace_stackable_thing] = [:namespace_stackable_foo_bar] + subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = [:namespace_reverse_stackable_foo_bar] subject.route[:route_thing] = :route_foo_bar expect(subject.to_hash).to include(global: { global_thing: :global_foo_bar }) @@ -209,6 +230,8 @@ module Util expect(subject.to_hash).to include(namespace_inheritable: { namespace_inheritable_thing: :namespace_inheritable_foo_bar }) expect(subject.to_hash).to include(namespace_stackable: { namespace_stackable_thing: [:namespace_stackable_foo_bar, [:namespace_stackable_foo_bar]] }) + expect(subject.to_hash).to include(namespace_reverse_stackable: + { namespace_reverse_stackable_thing: [[:namespace_reverse_stackable_foo_bar], :namespace_reverse_stackable_foo_bar] }) expect(subject.to_hash).to include(route: { route_thing: :route_foo_bar }) end end diff --git a/spec/grape/util/reverse_stackable_values_spec.rb b/spec/grape/util/reverse_stackable_values_spec.rb new file mode 100644 index 000000000..2fc623664 --- /dev/null +++ b/spec/grape/util/reverse_stackable_values_spec.rb @@ -0,0 +1,131 @@ +require 'spec_helper' +module Grape + module Util + describe ReverseStackableValues do + let(:parent) { described_class.new } + subject { described_class.new(parent) } + + describe '#keys' do + it 'returns all keys' do + subject[:some_thing] = :foo_bar + subject[:some_thing_else] = :foo_bar + expect(subject.keys).to eq [:some_thing, :some_thing_else].sort + end + + it 'returns merged keys with parent' do + parent[:some_thing] = :foo + parent[:some_thing_else] = :foo + + subject[:some_thing] = :foo_bar + subject[:some_thing_more] = :foo_bar + + expect(subject.keys).to eq [:some_thing, :some_thing_else, :some_thing_more].sort + end + end + + describe '#delete' do + it 'deletes a key' do + subject[:some_thing] = :new_foo_bar + subject.delete :some_thing + expect(subject[:some_thing]).to eq [] + end + + it 'does not delete parent values' do + parent[:some_thing] = :foo + subject[:some_thing] = :new_foo_bar + subject.delete :some_thing + expect(subject[:some_thing]).to eq [:foo] + end + end + + describe '#[]' do + it 'returns an array of values' do + subject[:some_thing] = :foo + expect(subject[:some_thing]).to eq [:foo] + end + + it 'returns parent value when no value is set' do + parent[:some_thing] = :foo + expect(subject[:some_thing]).to eq [:foo] + end + + it 'combines parent and actual values (actual first)' do + parent[:some_thing] = :foo + subject[:some_thing] = :foo_bar + expect(subject[:some_thing]).to eq [:foo_bar, :foo] + end + + it 'parent values are not changed' do + parent[:some_thing] = :foo + subject[:some_thing] = :foo_bar + expect(parent[:some_thing]).to eq [:foo] + end + end + + describe '#[]=' do + it 'sets a value' do + subject[:some_thing] = :foo + expect(subject[:some_thing]).to eq [:foo] + end + + it 'pushes further values' do + subject[:some_thing] = :foo + subject[:some_thing] = :bar + expect(subject[:some_thing]).to eq [:foo, :bar] + end + + it 'can handle array values' do + subject[:some_thing] = :foo + subject[:some_thing] = [:bar, :more] + expect(subject[:some_thing]).to eq [:foo, [:bar, :more]] + + parent[:some_thing_else] = [:foo, :bar] + subject[:some_thing_else] = [:some, :bar, :foo] + + expect(subject[:some_thing_else]).to eq [[:some, :bar, :foo], [:foo, :bar]] + end + end + + describe '#to_hash' do + it 'returns a Hash representation' do + parent[:some_thing] = :foo + subject[:some_thing] = [:bar, :more] + subject[:some_thing_more] = :foo_bar + expect(subject.to_hash).to eq( + some_thing: [[:bar, :more], :foo], + some_thing_more: [:foo_bar] + ) + end + end + + describe '#clone' do + let(:obj_cloned) { subject.clone } + it 'copies all values' do + parent = described_class.new + child = described_class.new parent + grandchild = described_class.new child + + parent[:some_thing] = :foo + child[:some_thing] = [:bar, :more] + grandchild[:some_thing] = :grand_foo_bar + grandchild[:some_thing_more] = :foo_bar + + expect(grandchild.clone.to_hash).to eq( + some_thing: [:grand_foo_bar, [:bar, :more], :foo], + some_thing_more: [:foo_bar] + ) + end + + context 'complex (i.e. not primitive) data types (ex. middleware, please see bug #930)' do + let(:middleware) { double } + + before { subject[:middleware] = middleware } + + it 'copies values; does not duplicate them' do + expect(obj_cloned[:middleware]).to eq [middleware] + end + end + end + end + end +end