Let a cascading route hand over to every remaining route - #2829
Merged
Conversation
ericproulx
force-pushed
the
fix-router-cascade-sibling-routes
branch
from
July 26, 2026 20:01
7278253 to
a3224f0
Compare
Danger ReportNo issues found. |
dblock
approved these changes
Jul 27, 2026
ericproulx
force-pushed
the
fix-router-cascade-sibling-routes
branch
from
July 27, 2026 05:12
a3224f0 to
5401b58
Compare
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 <noreply@anthropic.com>
ericproulx
force-pushed
the
fix-router-cascade-sibling-routes
branch
from
July 27, 2026 05:28
5401b58 to
a9e5cec
Compare
ericproulx
added a commit
that referenced
this pull request
Jul 27, 2026
#2824 made process_route build a fresh routing-args Hash per attempt, so a candidate tried after a cascading match (X-Cascade pass) could not observe the previous attempt's :route_info or path captures. #2829 was squash-merged from a branch cut before #2824 landed, so its squashed diff replayed the stale router.rb over the fix and dropped the regression spec along with it. Git reported no conflict, and #2824's CHANGELOG entry survived, so the tree claims a fix it no longer has. Symptoms are back on master: with a cascading v2 and a serving v1, the `route` helper returns v2's route (origin "/:id" instead of "/:name") and v2's capture leaks into params as {"id"=>"123", "name"=>"123"} -- an endpoint sees a param it never declared. Restore the fix and its spec verbatim. The router is the only writer of env['grape.routing_args'], so the unconditional assignment is safe. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A route that answers with
X-Cascade: passis declining the request, asking the router to try the next matching route.Grape::Router#transactiondid not do that. When the matched route cascaded and an ANY ('*') route existed, it took a shortcut:last_neighbor_routeis the greedy route — built per path pattern fromroutes.last(seeAPI::Instance#collect_route_config_per_pattern), i.e. the last route registered for that path. So a cascading match handed over to exactly one route, the last one, and returned its answer as final. Every route registered in between was unreachable.With two mounted versions this looks correct — "the last route" is the only sibling — which is why it survived a decade (the branch dates to the 2016 router rewrite,
499f6c6f, "Modify the priority of ANY routes, fixes #1089") and why the existingwith catch-allshared example, which mounts exactly v1 and v2, passes.Add a third version and the middle one disappears:
Accept: …-v1+txt200 v1 endpoint200 v1 endpointAccept: …-v2+txt406 API version not found200 v2 endpointAccept: …-v3+txt200 v3 endpoint200 v3 endpointAccept: …-v999+txt406 API version not found200 catch-all: versionv1 works because it is matched first; v3 works because it is the greedy route; v2 is served by neither.
Fix
When a match cascades, try the method's remaining routes (
#rotation) before falling back to the ANY routes and the greedy neighbour. The rest of the priority order — auto-OPTIONS, ANY routes, the greedy 405 — is unchanged, and#rotationis skipped entirely when nothing matched, since the compiled union it consults is the disjunction of the very patterns#rotationwould walk.#identitydisappears (its two halves are now the first lines of#transaction), andRouter#callno longer needs to run#rotationitself — the reason a request could previously reach#rotationonly after the ANY route had already answered.Behavior change: an unmatched version reaches a catch-all
Beyond the middle-version fix, one deliberate consequence, noted in UPGRADING: 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 (last row of the table). That is what
cascade: true— the default — asks for, and it is the same rule that already routes an unversioned path to the catch-all.version ..., cascade: falsestill answers 406 immediately and never consults the catch-all, so the hard-failure knob is unchanged.Cascading bodies are now closed
#rotationdiscarded superseded cascading responses without closing their bodies. That was unreachable-ish before (rotation ran last); as soon as a third sibling made the path live,Rack::LintflaggedBody has not been closed. Superseded responses are now released through a shared#close_body, which#halt?also uses.Verification
Beyond the suite, I diffed a 47-scenario probe of router behavior (versioned/unversioned, 1–3 versions, with and without a catch-all,
cascade: false, path/header versioning, auto-OPTIONS, 405, HEAD, ANY-only, mixed specific+ANY, bare Rack mounts, nested mounts, 404s) between master and this branch. Exactly the four rows in the table above changed; the other 43 are byte-identical.New specs: the shared versioning examples'
with catch-allcontext gains a third version — so the existingv2cases become middle-version cases across all four versioning strategies — plus an unknown-version case;router_spec.rbgains router-level specs pinning the contract without going through versioning. All new specs fail on master.Relationship to #2824
#2824 fixes the routing-args leak on this same path (a cascaded-to route inheriting the previous attempt's
route_infoand path captures). They are independent, but complementary: this PR makes middle versions reachable, and #2824 is what makes the route that serves them see its ownroute_infoandparams. Both touchlib/grape/router.rbandspec/grape/router_spec.rb, and both add an UPGRADING entry, so whichever merges second needs a trivial rebase.🤖 Generated with Claude Code