diff --git a/CHANGELOG.md b/CHANGELOG.md index 0049465b9..17c61b3b8 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). +* [#2841](https://github.com/ruby-grape/grape/pull/2841): Stop `use`, `helpers`, `rescue_from` and other registrations declared below a route from reaching it when an earlier registration had seeded the same key (see UPGRADING) - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 3.3.5 (2026-07-30) diff --git a/UPGRADING.md b/UPGRADING.md index ab4838229..5d339be54 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,41 @@ Upgrading Grape ### Upgrading to >= 4.0.0 +#### `use`, `helpers`, `rescue_from` and other registrations no longer reach routes defined above them + +A route captures the middleware, helpers, callbacks and rescue handlers registered above it. That was already true most of the time, but not always: `Grape::Util::InheritableSetting#point_in_time_copy` copied a scope's stackable store and its rescue-handler maps shallowly, so the nested Arrays and Hashes stayed shared with the scope. A registration added *after* an endpoint was defined therefore still reached that endpoint — but only when the key already held at least one registration when the endpoint was defined, since otherwise the scope allocated a fresh store only for itself. + +The outcome depended on something the API never expressed: + +```ruby +class A < Grape::API + use Middleware1 + get('/x') { } # endpoint defined here + use Middleware2 # applied to GET /x +end + +class B < Grape::API + get('/x') { } # endpoint defined here + use Middleware2 # NOT applied to GET /x +end +``` + +`A` and `B` state the same thing and behaved differently. Both now behave like `B`. The same held for `rescue_from` declared below a route. + +**What can break.** An API that declares `use` (or `helpers`, a filter such as `before`, or `rescue_from`) below its routes and relies on it applying to them. That arrangement only ever worked when an earlier registration for the same key happened to seed the stack, so it was never dependable, but code written against it will now see the middleware or helper silently not run. + +**The fix is to move the registration above the routes it should cover**, which is where Grape's documentation has always placed it: + +```ruby +class A < Grape::API + use Middleware1 + use Middleware2 + get('/x') { } +end +``` + +Nothing changes for the ordinary arrangement — registrations declared before a route, or inherited from an enclosing namespace or a mounting API, still apply exactly as before, including values an enclosing scope gains after the nested scope was created. + #### `Array`/`Set` of an unsupported type is rejected when the API is defined Declaring a collection whose element type Grape cannot coerce — `type: Array[Foo]` or `type: Set[Foo]` where `Foo` is neither a primitive, a structure, nor a valid custom type — now raises as soon as the `params` block is evaluated, i.e. while the API class is being loaded: diff --git a/lib/grape/util/inheritable_setting.rb b/lib/grape/util/inheritable_setting.rb index 21593255f..65cba6a59 100644 --- a/lib/grape/util/inheritable_setting.rb +++ b/lib/grape/util/inheritable_setting.rb @@ -778,11 +778,14 @@ def merged_rescue_handlers(key) def copy_state_from(source) @namespace = source.namespace.dup @namespace_inheritable = source.namespace_inheritable&.dup - # Shallow, matching the store this replaced: the per-key Arrays stay - # shared with the source, so a registration made on the source after - # the copy was taken is still visible through it. - @stackable_values = source.stackable_values&.dup - @rescue_handler_maps = source.rescue_handler_maps&.dup + # The nested stores are duped too, not just the Hash holding them. A + # copy is a point in time: what the source registers afterwards must not + # reach it. Sharing them made that depend on whether the key already + # held a registration — `use` below a route applied to it when some + # earlier `use` had seeded the Array, and did nothing otherwise; the + # same went for `rescue_from` below a route. + @stackable_values = source.stackable_values&.transform_values(&:dup) + @rescue_handler_maps = source.rescue_handler_maps&.transform_values(&:dup) @route = source.route.clone end diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index ff3454f15..8e157141a 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -1517,6 +1517,48 @@ def call(env) expect(last_response.body).to eql 'hello' end + # A route captures the middleware registered above it. Whether some + # earlier `use` had already seeded the scope's stack must not change that. + context 'when declared below a route' do + it 'does not apply to that route' do + subject.get('/') { env['phony.args'].inspect } + subject.use phony_middleware, 'too-late' + + get '/' + expect(last_response.body).to eql 'nil' + end + + it 'does not apply to that route when an earlier use seeded the stack' do + subject.use phony_middleware, 'in-time' + subject.get('/') { env['phony.args'].flatten.inspect } + subject.use phony_middleware, 'too-late' + + get '/' + expect(last_response.body).to eql ['in-time'].inspect + end + end + end + + describe '.rescue_from declared below a route' do + let(:boom) { Class.new(StandardError) } + + it 'does not apply to that route' do + error_class = boom + subject.get('/') { raise error_class } + subject.rescue_from(error_class) { error!('too late', 480) } + + expect { get '/' }.to raise_error(error_class) + end + + it 'does not apply to that route when an earlier rescue_from seeded the map' do + error_class = boom + subject.rescue_from(ArgumentError) { error!('in time', 481) } + subject.get('/') { raise error_class } + subject.rescue_from(error_class) { error!('too late', 480) } + + expect { get '/' }.to raise_error(error_class) + end + it 'adds a block if one is given' do block = -> {} subject.use phony_middleware, &block diff --git a/spec/grape/util/inheritable_setting_spec.rb b/spec/grape/util/inheritable_setting_spec.rb index 729866879..312d74193 100644 --- a/spec/grape/util/inheritable_setting_spec.rb +++ b/spec/grape/util/inheritable_setting_spec.rb @@ -332,6 +332,68 @@ expect(cloned_obj.helpers).to eq [:namespace_stackable_foo_bar] end + # The case above registers only on the parent, so the copy never shares an + # Array with `subject` and passes even when the per-key Arrays are shared. + # Here the key already holds one of `subject`'s own registrations when the + # copy is taken, which is what made the later one leak into it. + context 'when the scope already registered the key itself' do + subject(:setting) do + described_class.new.tap do |settings| + settings.inherit_from parent + settings.add_helper(:own_before_copy) + end + end + + let!(:cloned_obj) { setting.point_in_time_copy } + + it 'does not leak a later registration into the copy' do + setting.add_helper(:own_after_copy) + + expect(setting.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy own_after_copy] + expect(cloned_obj.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy] + end + + it 'does not leak the copy’s own registration back to the source' do + cloned_obj.add_helper(:only_on_copy) + + expect(setting.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy] + expect(cloned_obj.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy only_on_copy] + end + + it 'keeps sibling copies independent' do + sibling = setting.point_in_time_copy + cloned_obj.add_helper(:only_on_first) + + expect(sibling.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy] + end + end + + # Same shape as the stackable case: the per-kind Hashes inside + # @rescue_handler_maps have to be duped, not just the Hash holding them. + context 'when the scope already registered a rescue handler' do + subject(:setting) do + described_class.new.tap do |settings| + settings.add_rescue_handlers({ ArgumentError => :before_copy }, subclasses: true) + end + end + + let!(:cloned_obj) { setting.point_in_time_copy } + + it 'does not leak a later handler into the copy' do + setting.add_rescue_handlers({ TypeError => :after_copy }, subclasses: true) + + expect(setting.rescue_handlers).to eq(ArgumentError => :before_copy, TypeError => :after_copy) + expect(cloned_obj.rescue_handlers).to eq(ArgumentError => :before_copy) + end + + it 'does not leak a later base-only handler into the copy' do + setting.add_rescue_handlers({ TypeError => :after_copy }, subclasses: false) + + expect(setting.base_only_rescue_handlers).to eq(TypeError => :after_copy) + expect(cloned_obj.base_only_rescue_handlers).to be_blank + end + end + it 'decouples route values' do expect(cloned_obj.route[:route_thing]).to eq :route_foo_bar