Restore the per-attempt routing args in Grape::Router - #2834
Merged
Conversation
#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>
Danger ReportNo issues found. |
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.
Summary
#2824's fix is no longer in
master. This restores it, with its regression spec.lib/grape/router.rb#process_routeis back to seedingenv['grape.routing_args']once with||=and merging each attempt's captures in place:How it was lost
#2824 landed as a merge commit (
0a08e97d). #2829 was squash-merged (59188e2a) from a branch cut before #2824, so its squashed diff replayed the stalerouter.rbover the fix and removed the regression spec fromrouter_spec.rbin the same stroke. Git reported no conflict — a squash replays the branch diff onto the tip.658e0d7fis still an ancestor ofHEADand #2824's CHANGELOG entry survived, so nothing about the history or the changelog suggests the fix is missing. With the spec gone too, the suite stayed green.Impact (verified on current master)
Restoring the spec against master fails:
When a route cascades (
X-Cascade: pass) and a later candidate answers:routehelper returns the cascaded route, not the serving one;params— the endpoint seesid, which it never declared. This is the more consequential half, since it reaches user code anddeclared.Notes
658e0d7f. The router is the only writer of that env key, so assigning unconditionally is safe.bundle exec rubocopandbundle exec rspec(2516 examples) pass.🤖 Generated with Claude Code