Refactor handling of :action default in routing #23103
Conversation
|
@jeremy @tenderlove @rafaelfranca I'd appreciate a sanity check on this please - tests seem to pass but I doubt we have coverage over every permutation. |
pixeltrix
force-pushed
the
refactor-handling-of-action-default
branch
2 times, most recently
from
f716d60
to
3c5eb0b
Compare
The longstanding convention in Rails is that if the :action parameter is missing or nil then it defaults to 'index'. Up until Rails 5.0.0.beta1 this was handled slightly differently than other routing defaults by deleting it from the route options and adding it to the recall parameters. With the recent focus of removing unnecessary duplications this has exposed a problem in this strategy - we are now mutating the request's path parameters and causing problems for later url generation. This will typically affect url_for rather a named url helper since the latter explicitly pass :controller, :action, etc. The fix is to add a default for :action in the route class if the path contains an :action segment and no default is passed. This change also revealed an issue with the parameterized part expiry in that it doesn't follow a right to left order - as soon as a dynamic segment is required then all other segments become required. Fixes #23019.
pixeltrix
force-pushed
the
refactor-handling-of-action-default
branch
from
3c5eb0b
to
8ca8a2d
Compare
|
@pixeltrix Code looks good. I've been patching my Rails 5 app with this PR's code for a while now, and haven't seen any issues. It solves the issues with recall being changed. |
| @@ -1964,11 +1964,11 @@ def test_generate_extras | |||
| def test_extras | |||
| params = {:controller => 'people'} | |||
| assert_equal [], @routes.extra_keys(params) | |||
| assert_equal({:controller => 'people'}, params) | |||
| assert_equal({:controller => 'people', :action => 'index'}, params) | |||
There was a problem hiding this comment.
Calling extra_keys(params) will mutate params, is that correct?
| end | ||
|
|
||
| def action | ||
| requirements[:action] || ':action' | ||
| parts.include?(:action) ? ':action' : requirements[:action] |
There was a problem hiding this comment.
Going from constant time to linear here.
There was a problem hiding this comment.
The number of parts is almost always so low that the speed difference doesn't matter – right?
There was a problem hiding this comment.
¯ \ _ (ツ) _ / ¯ I don't remember what parts is right here, if it's a hash we could parts.key?(:action) if it's an array, then maybe when it's initialized we could loop over and pull out those to values :controller and :action so that we only have to iterate once.
The routing code is very performance sensitive. I'm fine with getting a solution on there then working to speed it back up if it's an issue.
There was a problem hiding this comment.
It's also the routing inspector so doesn't need to be the most performant of code, unlike recognition and generation.
|
Thanks for the ping, I subscribed to the issue already. I haven't had the chance to dig into the mutation bug I introduced. I think the approach is sound, the only thing that would worry me is that some existing tests were modified, I'm not clear on the reasons why. @pixeltrix how comfortable are you moving forward with this solution? Are you happy with it or is there more you want to do? |
|
Ping, any new thoughts or blockers here? |
|
@schneems will have another look at this in the next day or two |
|
Let's nail this down so we get a beta4 out. |
|
I'm puzzled that we didn't simply revert 1993e2c. This PR checks out, though. Merging for beta4! |
Refactor handling of :action default in routing
The longstanding convention in Rails is that if the :action parameter is missing or nil then it defaults to 'index'. Up until Rails 5.0.0.beta1 this was handled slightly differently than other routing defaults by
deleting it from the route options and adding it to the recall parameters.
With the recent focus of removing unnecessary duplications this has exposed a problem in this strategy - we are now mutating the request's path parameters and causing problems for later url generation. This will typically affect url_for rather a named url helper since the latter explicitly pass :controller, :action, etc.
The fix is to add a default for :action in the route class if the path contains an :action segment and no default is passed. This change also revealed an issue with the parameterized part expiry in that it doesn't
follow a right to left order - as soon as a dynamic segment is required then all other segments become required.