Skip to content

Commit

Permalink
Remove literal? check to fix issue with prefixed optionals
Browse files Browse the repository at this point in the history
In commit d993cb3 `build_path` was changed from using `grep` to
`find_all` to save array allocations.

This change was a little too aggressive in that when the dash comes
before the symbol like `/omg-:song` the symbol is skipped.

Removing the check for `n.right.left.literal?` fixes this issue, but
does add back some allocations. The number of allocations are still well
less than before.

I've added a regression test to test this behavior for the future.

Fixes #23069.

Array allocations as of d993cb3:

```
{:T_SYMBOL=>11}
{:T_REGEXP=>17}
{:T_STRUCT=>6500}
{:T_MATCH=>12004}
{:T_OBJECT=>91009}
{:T_DATA=>100088}
{:T_HASH=>114013}
{:T_STRING=>159637}
{:T_ARRAY=>321056}
{:T_IMEMO=>351133}
```

Array allocations after this change:

```
{:T_SYMBOL=>11}
{:T_REGEXP=>1017}
{:T_STRUCT=>6500}
{:T_MATCH=>12004}
{:T_DATA=>84092}
{:T_OBJECT=>87009}
{:T_HASH=>110015}
{:T_STRING=>166152}
{:T_ARRAY=>322056}
{:T_NODE=>343558}
```
  • Loading branch information
eileencodes committed Jan 16, 2016
1 parent cbb7f7d commit 5d1b7c3
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 1 deletion.
2 changes: 1 addition & 1 deletion actionpack/lib/action_dispatch/routing/mapper.rb
Expand Up @@ -187,7 +187,7 @@ def build_path(ast, requirements, anchor)
# Get all the symbol nodes followed by literals that are not the
# dummy node.
symbols = ast.find_all { |n|
n.cat? && n.left.symbol? && n.right.cat? && n.right.left.literal?
n.cat? && n.left.symbol? && n.right.cat?
}.map(&:left)

# Get all the symbol nodes preceded by literals.
Expand Down
12 changes: 12 additions & 0 deletions actionpack/test/controller/routing_test.rb
Expand Up @@ -94,6 +94,18 @@ def test_symbols_with_dashes
assert_equal({"artist"=>"journey", "song"=>"faithfully"}, hash)
end

def test_symbols_with_dashes_reversed
rs.draw do
get ':artist(/omg-:song)', :to => lambda { |env|
resp = ActiveSupport::JSON.encode ActionDispatch::Request.new(env).path_parameters
[200, {}, [resp]]
}
end

hash = ActiveSupport::JSON.decode get(URI('http://example.org/journey/omg-faithfully'))
assert_equal({"artist"=>"journey", "song"=>"faithfully"}, hash)
end

def test_id_with_dash
rs.draw do
get '/journey/:id', :to => lambda { |env|
Expand Down

0 comments on commit 5d1b7c3

Please sign in to comment.