diff --git a/CHANGELOG.md b/CHANGELOG.md index 0049465b9..13b9d75a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,7 @@ * [#2827](https://github.com/ruby-grape/grape/pull/2827): Make the `cascade` DSL getter return the configured value (`cascade false` read back as `true`) - [@ericproulx](https://github.com/ericproulx). * [#2829](https://github.com/ruby-grape/grape/pull/2829): Fix a cascading route handing over only to the last route registered for the path, making a middle version (3+ mounted versions with a catch-all) answer 406 - [@ericproulx](https://github.com/ericproulx). * [#2826](https://github.com/ruby-grape/grape/pull/2826): Fix `api.version` not being set for the root route of a path-versioned API (`GET /v1`) - [@ericproulx](https://github.com/ericproulx). +* [#2842](https://github.com/ruby-grape/grape/pull/2842): Warn at definition time when a `rescue_from` class is already covered by one registered earlier in the same scope, since the later handler never runs - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 3.3.5 (2026-07-30) diff --git a/README.md b/README.md index efae8b900..f8ac3ab84 100644 --- a/README.md +++ b/README.md @@ -2747,6 +2747,17 @@ end In this case ```UserDefinedError``` must be inherited from ```StandardError```. +When several classes could match, the one registered **first** in a scope wins — as with the clauses of a Ruby `rescue`. Register the more specific class before the broader one, or the narrower handler never runs: + +```ruby +class Twitter::API < Grape::API + rescue_from ArgumentError do ... end # matched first + rescue_from StandardError do ... end # everything else +end +``` + +Grape warns when a `rescue_from` is registered for a class an earlier one in the same scope already covers. This is about ordering within a scope; a handler in a nested namespace or a mounted API always takes precedence over one inherited from an enclosing scope, whatever the classes are. + Notice that you could combine these two approaches (rescuing custom errors takes precedence). For example, it's useful for handling all exceptions except Grape validation errors. ```ruby diff --git a/lib/grape/util/inheritable_setting.rb b/lib/grape/util/inheritable_setting.rb index 21593255f..25bbcf519 100644 --- a/lib/grape/util/inheritable_setting.rb +++ b/lib/grape/util/inheritable_setting.rb @@ -384,6 +384,7 @@ def base_only_rescue_handlers def add_rescue_handlers(mapping, subclasses:) @rescue_handler_maps ||= {} own = (@rescue_handler_maps[subclasses ? :rescue_handlers : :base_only_rescue_handlers] ||= {}) + ShadowedRescueHandlers.warn_about(own, mapping) if subclasses own.merge!(mapping) { |_klass, registered, _new| registered } end diff --git a/lib/grape/util/shadowed_rescue_handlers.rb b/lib/grape/util/shadowed_rescue_handlers.rb new file mode 100644 index 000000000..b5180adc5 --- /dev/null +++ b/lib/grape/util/shadowed_rescue_handlers.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module Grape + module Util + # Diagnostics for +rescue_from+ registrations that can never run. + # + # Middleware::Error resolves with +find+, so within a scope the first + # matching class wins and one registered for a class an earlier handler + # already covers is dead code — silently, before this warned: + # + # rescue_from StandardError do ... end # wins + # rescue_from ArgumentError do ... end # never runs + # + # Warn rather than reorder: which should win is the author's call, and + # +rescue_from :all+ (consulted only after the registered handlers) already + # offers "broad first, specific still wins" to anyone who wants it. + module ShadowedRescueHandlers + module_function + + # @param registered [Hash] the scope's own handlers, in registration order + # @param mapping [Hash] the handlers being registered now + # @return [void] + # + # Only a scope's own registrations are compared: across scopes the nearest + # one deliberately wins, so an inner +rescue_from StandardError+ shadowing + # an outer +rescue_from ArgumentError+ is the documented behaviour rather + # than a mistake. Classes sharing a handler object are skipped too, since + # +rescue_from A, B+ registers one handler for both and the entry that + # loses to the other changes nothing. + def warn_about(registered, mapping) + return if registered.empty? + + mapping.each do |klass, handler| + covered_by, = registered.find { |already, existing| klass <= already && !existing.equal?(handler) } + next unless covered_by + + warn(message_for(klass, covered_by)) + end + end + + def message_for(klass, covered_by) + return "Grape: rescue_from #{klass} was already registered in this scope; the first handler is kept and this one will never run." if klass == covered_by + + "Grape: rescue_from #{klass} will never run — #{covered_by} was registered earlier in the same scope " \ + 'and is matched first. Register the more specific class before the broader one.' + end + end + end +end diff --git a/spec/grape/util/inheritable_setting_spec.rb b/spec/grape/util/inheritable_setting_spec.rb index 729866879..f057514fe 100644 --- a/spec/grape/util/inheritable_setting_spec.rb +++ b/spec/grape/util/inheritable_setting_spec.rb @@ -227,6 +227,62 @@ subject.add_rescue_handlers({ StandardError => :child }, subclasses: true) expect(subject.rescue_handlers).to eq(StandardError => :child) end + + # Middleware::Error resolves with #find, so the first matching class in a + # scope wins and a narrower one registered after it is dead code. + describe 'shadowing warnings' do + def add(mapping, subclasses: true) + subject.add_rescue_handlers(mapping, subclasses:) + end + + it 'warns when a broader class was registered first' do + add({ StandardError => :broad }) + + expect { add({ ArgumentError => :narrow }) } + .to output(/rescue_from ArgumentError will never run — StandardError was registered earlier/).to_stderr + end + + it 'warns when the same class is registered twice' do + add({ ArgumentError => :first }) + + expect { add({ ArgumentError => :second }) } + .to output(/rescue_from ArgumentError was already registered in this scope/).to_stderr + end + + it 'does not warn when the narrower class was registered first' do + add({ ArgumentError => :narrow }) + + expect { add({ StandardError => :broad }) }.not_to output.to_stderr + end + + it 'does not warn for unrelated classes' do + add({ ArgumentError => :one }) + + expect { add({ TypeError => :two }) }.not_to output.to_stderr + end + + # `rescue_from A, B` registers one handler for both, so the entry that + # loses to the other changes nothing. + it 'does not warn when both classes share a handler' do + expect { add({ StandardError => :shared, ArgumentError => :shared }) }.not_to output.to_stderr + end + + # Exact-match handlers are consulted before the subclass-matching ones and + # never match a descendant, so they cannot shadow each other. + it 'does not warn for exact-match handlers' do + add({ StandardError => :broad }, subclasses: false) + + expect { add({ ArgumentError => :narrow }, subclasses: false) }.not_to output.to_stderr + end + + # Across scopes the nearest registration deliberately wins. + it 'does not warn about a handler inherited from an enclosing scope' do + parent = described_class.new.tap { |s| s.add_rescue_handlers({ StandardError => :outer }, subclasses: true) } + subject.inherit_from parent + + expect { add({ ArgumentError => :inner }) }.not_to output.to_stderr + end + end end describe '#route' do