From a9e5cecf98e4dafecf31a72026a8de52b4b28511 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 26 Jul 2026 22:00:00 +0200 Subject: [PATCH] Let a cascading route hand over to every remaining route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Router#transaction sent a cascading match straight to the greedy neighbour — the last route registered for the path — and returned its answer, so any route registered between the first match and that last one was unreachable. With three mounted API versions plus a catch-all `route :any, '*path'`, v1 and v3 were served while v2 answered 406; the shortcut only ever looked correct with exactly two versions, where "the last route" happens to be the sibling. Try the method's remaining routes (#rotation) as soon as a match cascades, before the ANY routes and the greedy neighbour, and keep the priority order otherwise intact. A consequence, and what cascade: true means: with a catch-all present, an unmatched version now falls through to it rather than surfacing the versioner's 406. cascade: false still answers 406 immediately and never consults the catch-all. Also close the body of every superseded cascading response — Rack requires it, and rotation previously leaked them (Rack::Lint flagged this as soon as a third sibling made the path reachable). Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + UPGRADING.md | 8 +++ lib/grape/router.rb | 105 +++++++++++++++++------------ spec/grape/router_spec.rb | 83 +++++++++++++++-------- spec/shared/versioning_examples.rb | 34 ++++++++++ 5 files changed, 159 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1bcc08fb..ff76b8d70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ * [#2824](https://github.com/ruby-grape/grape/pull/2824): Fix cascaded routes (`X-Cascade: pass`) leaking `route_info` and path captures into the next matched route's `route` and `params` - [@ericproulx](https://github.com/ericproulx). * [#2825](https://github.com/ruby-grape/grape/pull/2825): Stop `present` entity autodetection from picking up a top-level `::Entity` constant - [@ericproulx](https://github.com/ericproulx). * [#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). * Your contribution here. ### 3.3.4 (2026-07-25) diff --git a/UPGRADING.md b/UPGRADING.md index ca6031d40..837ecfdb6 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -6,6 +6,14 @@ Upgrading Grape #### The `cascade` getter returns the configured value Calling `cascade` with no argument used to report whether cascading had been *configured at all* (`cascade false` still read back as `true`, contradicting the actual runtime behavior, which was correctly disabled). It now returns the configured value itself — `true`/`false` as set, or `true` when never set. Code that used the getter to detect "was `cascade` called" rather than "does this API cascade" must track that separately. +#### A cascading route hands over to every remaining route + +A route that answers with `X-Cascade: pass` — what a version mismatch does by default — now hands the request to every remaining matching route, in registration order, before the router gives up. Previously it handed over to the *last* route registered for the path and stopped there, so with three or more routes sharing a path (typically three mounted API versions) every route in between was unreachable. + +Two consequences: + +* **A middle version is now served.** `mount v1; mount v2; mount v3` alongside a catch-all `route :any, '*path'` answered `406 API version not found` for v2, while v1 and v3 worked; v2 is now served. +* **An unmatched version reaches a catch-all.** When a catch-all ANY route is present, a request whose version matches nothing now falls through to it instead of surfacing the versioner's `406`, which is what `cascade: true` (the default) asks for. Declare `version ..., cascade: false` to keep the hard 406 — that path is unchanged and never consults the catch-all. #### `forward_match` is no longer exposed on routes diff --git a/lib/grape/router.rb b/lib/grape/router.rb index ad5b476c8..f72d0fda0 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -49,9 +49,7 @@ def associate_routes(greedy_route) def call(env) with_optimization do input = Grape::Util::PathNormalizer.call(env[Rack::PATH_INFO]) - method = env[Rack::REQUEST_METHOD] - response, route = identity(input, method, env) - response || rotation(input, method, env, route) + transaction(input, env[Rack::REQUEST_METHOD], env) end end @@ -67,52 +65,69 @@ def recognize_path(input) private - def identity(input, method, env) - route = nil - response = transaction(input, method, env) do - route = match?(input, method) - process_route(route, input, env) if route - end - [response, route] - end - - def rotation(input, method, env, exact_route) - response = nil - @map[method]&.each do |route| - next if exact_route == route - next unless route.match?(input) - - response = process_route(route, input, env) - break unless cascade?(response) - end - response - end - + # Resolve +input+ against the compiled routes, in priority order: + # + # 1. the routes registered for +method+ — the compiled-union match first, + # then, when that route cascades, its siblings (see #rotation); + # 2. the ANY (+'*'+) routes; + # 3. the greedy neighbour, which answers auto-OPTIONS and 405. + # + # Returns nil when nothing answered, leaving the caller to 404. A response + # that cascades is never final: it is returned only once every later + # candidate has declined too, so the caller (or a mounting app upstream) + # can keep looking. def transaction(input, method, env) - response = yield + exact_route = match?(input, method) + response = process_route(exact_route, input, env) if exact_route return response if halt?(response) - last_response_cascade = !response.nil? + # A cascading route has only declined this request. Its siblings — the + # routes sharing this path but differing in, say, version — must be + # tried before falling back to the ANY routes and the greedy neighbour. + # Skipped when nothing matched: the compiled union is the disjunction of + # the same patterns #rotation walks, so a miss there is a miss here. + cascaded = !response.nil? + if cascaded + response = rotation(input, method, env, exact_route) + return response if response && !cascade?(response) + end + last_neighbor_route = greedy_match?(input) # If last_neighbor_route exists and request method is OPTIONS, # return response by using #include_allow_header. - return process_route(last_neighbor_route, input, env, include_allow_header: true) if !last_response_cascade && method == Rack::OPTIONS && last_neighbor_route + return process_route(last_neighbor_route, input, env, include_allow_header: true) if !cascaded && method == Rack::OPTIONS && last_neighbor_route - route = match?(input, '*') + star_route = match?(input, '*') - return last_neighbor_route.call(env) if last_neighbor_route && last_response_cascade && route + if star_route + close_body(response) if response # superseded by the ANY route + response = process_route(star_route, input, env) + return response if halt?(response) - if route - route_response = process_route(route, input, env) - return route_response if halt?(route_response) - - last_response_cascade = !route_response.nil? + cascaded ||= !response.nil? end - return process_route(last_neighbor_route, input, env, include_allow_header: true) if !last_response_cascade && last_neighbor_route + return process_route(last_neighbor_route, input, env, include_allow_header: true) if !cascaded && last_neighbor_route + + response + end + + # The routes registered for +method+ other than +exact_route+, tried in + # registration order until one answers without cascading. Returns the last + # response processed — a cascading one when every sibling declined, so the + # caller can hand it back — or nil when no sibling matched. + def rotation(input, method, env, exact_route) + response = nil + @map[method]&.each do |route| + next if exact_route == route + next unless route.match?(input) - nil + close_body(response) if response # the previous sibling cascaded + response = process_route(route, input, env) + break unless cascade?(response) + end + response end # Returns true if `response` should be returned as-is from the enclosing @@ -122,18 +137,22 @@ def halt?(response) return false unless response cascade = cascade?(response) - response[2].close if cascade && response[2].respond_to?(:close) + close_body(response) if cascade !cascade end - # Routing args are rebuilt for every attempt: when a route cascades - # (X-Cascade pass), the next candidate must not observe the previous - # attempt's +route_info+ or path captures. + # Releases a response the router has decided not to return. Rack requires + # every body it hands out to be closed, and a cascading candidate is + # discarded as soon as a later one answers. + def close_body(response) + body = response[2] + body.close if body.respond_to?(:close) + end + def process_route(route, input, env, include_allow_header: false) route_params = route.params_for(input) - routing_args = { route_info: route } - routing_args.merge!(route_params) if route_params.present? - env[Grape::Env::GRAPE_ROUTING_ARGS] = routing_args + env[Grape::Env::GRAPE_ROUTING_ARGS] ||= { route_info: route } + env[Grape::Env::GRAPE_ROUTING_ARGS].merge!(route_params) if route_params.present? env[Grape::Env::GRAPE_ALLOWED_METHODS] = route.allow_header if include_allow_header route.call(env) end diff --git a/spec/grape/router_spec.rb b/spec/grape/router_spec.rb index 3b73720bd..2a864791b 100644 --- a/spec/grape/router_spec.rb +++ b/spec/grape/router_spec.rb @@ -49,38 +49,63 @@ end end - # Regression: routing args were seeded once (`||=`) and merged in place, so - # when a route cascaded (X-Cascade pass) the next candidate still saw the - # previous attempt's :route_info and path captures. - describe 'routing args across cascading routes' do - let(:app) do - v2 = Class.new(Grape::API) do - version 'v2', using: :header, vendor: 'grape', cascade: true - get ':id' do - { from: 'v2' } - end - end - v1 = Class.new(Grape::API) do - format :json - version 'v1', using: :header, vendor: 'grape' - get ':name' do - { origin: route.origin, params: params.to_h } - end - end - Class.new(Grape::API) do - format :json - mount v2 - mount v1 - end + # Regression: a cascading route used to hand straight over to the greedy + # neighbour — the *last* route registered for the path — so any route + # between the first match and that last one was unreachable. + describe 'cascading routes' do + subject(:router) { described_class.new } + + let(:pattern) do + Grape::Router::Pattern.new(origin: '/hello', suffix: '', anchor: true, params: {}, version: nil, requirements: {}) end + let(:cascading) { ->(_env) { [404, { 'X-Cascade' => 'pass' }, []] } } + let(:serving) { ->(_env) { [200, {}, ['served']] } } + let(:any_handler) { ->(_env) { [200, {}, ['any']] } } - it 'does not leak route_info or path captures from a cascaded attempt' do - get '/123', {}, 'HTTP_ACCEPT' => 'application/vnd.grape-v1+json' + def append_route(endpoint, method = :get) + router.append(Grape::Router::Route.new(endpoint, method, pattern, {}, forward_match: false)) + end + + def response_body + _, _, body = router.call(Rack::MockRequest.env_for('/hello')) + body.each_with_object(+'') { |chunk, buffer| buffer << chunk } + end - expect(last_response.status).to eq(200) - body = JSON.parse(last_response.body) - expect(body['origin']).to eq('/:name') - expect(body['params']).to eq('name' => '123') + it 'hands over to a sibling route registered after the cascading one' do + append_route(cascading) + append_route(cascading) + append_route(serving) + router.compile! + + expect(response_body).to eq('served') + end + + it 'prefers a sibling of the same method over an ANY route' do + append_route(cascading) + append_route(serving) + append_route(any_handler, '*') + router.compile! + + expect(response_body).to eq('served') + end + + it 'falls through to an ANY route once every sibling has cascaded' do + append_route(cascading) + append_route(cascading) + append_route(any_handler, '*') + router.compile! + + expect(response_body).to eq('any') + end + + it 'returns the last cascading response when nothing else answers' do + append_route(cascading) + append_route(cascading) + router.compile! + + status, headers, = router.call(Rack::MockRequest.env_for('/hello')) + expect(status).to eq(404) + expect(headers['X-Cascade']).to eq('pass') end end end diff --git a/spec/shared/versioning_examples.rb b/spec/shared/versioning_examples.rb index f5c354064..8d794f4e2 100644 --- a/spec/shared/versioning_examples.rb +++ b/spec/shared/versioning_examples.rb @@ -172,12 +172,24 @@ end klass end + # A third version makes v2 a *middle* version: reachable neither as the + # first route matched nor as the last one, which is what regressed when a + # cascading route handed straight over to the catch-all. + let(:v3) do + klass = Class.new(Grape::API) + klass.version 'v3', **options.except(:format) + klass.get 'version' do + 'v3' + end + klass + end before do subject.format :txt subject.mount v1 subject.mount v2 + subject.mount v3 subject.route :any, '*path' do params[:path] @@ -211,5 +223,27 @@ expect(last_response.body).to end_with 'whatever' end end + + context 'v3' do + it 'finds endpoint' do + versioned_get '/version', 'v3', macro_options + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('v3') + end + + it 'finds catch all' do + versioned_get '/whatever', 'v3', macro_options + expect(last_response.status).to eq(200) + expect(last_response.body).to end_with 'whatever' + end + end + + context 'with an unknown version' do + it 'falls through to the catch-all' do + versioned_get '/version', 'v999', macro_options + expect(last_response.status).to eq(200) + expect(last_response.body).to end_with 'version' + end + end end end