aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authoreileencodes <eileencodes@gmail.com>2016-01-16 15:00:18 -0500
committereileencodes <eileencodes@gmail.com>2016-01-16 15:00:18 -0500
commit5d1b7c3b441654e8008dcd303f5367883ec660a6 (patch)
treed3ded43da2dc15f5448a63d787f34413f26c7184
parentcbb7f7d00484a7bd90cf95f7c76741c19042b7df (diff)
downloadrails-5d1b7c3b441654e8008dcd303f5367883ec660a6.tar.gz
rails-5d1b7c3b441654e8008dcd303f5367883ec660a6.tar.bz2
rails-5d1b7c3b441654e8008dcd303f5367883ec660a6.zip
Remove literal? check to fix issue with prefixed optionals
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} ```
-rw-r--r--actionpack/lib/action_dispatch/routing/mapper.rb2
-rw-r--r--actionpack/test/controller/routing_test.rb12
2 files changed, 13 insertions, 1 deletions
diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 522012063d..f57d88d1a5 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -187,7 +187,7 @@ module ActionDispatch
# 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.
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index a39fede5b9..178cd41922 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -94,6 +94,18 @@ class LegacyRouteSetTests < ActiveSupport::TestCase
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|