Use custom visitor class for optimized url helpers

Rather than trying to use gsub to remove the optional route segments,
which will fail with nested optional segments, use a custom visitor
class that returns a empty string for group nodes.

Closes #9524

4 files changed, 19 insertions, 9 deletions

diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb
index 063302e0f2..6fda085681 100644
--- a/actionpack/lib/action_dispatch/journey/route.rb
+++ b/actionpack/lib/action_dispatch/journey/route.rb
@@ -71,6 +71,10 @@ module ActionDispatch
         Visitors::Formatter.new(path_options).accept(path.spec)
       end
 
+      def optimized_path
+        Visitors::OptimizedPath.new.accept(path.spec)
+      end
+
       def optional_parts
         path.optional_names.map { |n| n.to_sym }
       end
diff --git a/actionpack/lib/action_dispatch/journey/visitors.rb b/actionpack/lib/action_dispatch/journey/visitors.rb
index 46bd58c178..2964d80d9f 100644
--- a/actionpack/lib/action_dispatch/journey/visitors.rb
+++ b/actionpack/lib/action_dispatch/journey/visitors.rb
@@ -74,6 +74,14 @@ module ActionDispatch
         end
       end
 
+      class OptimizedPath < String # :nodoc:
+        private
+
+        def visit_GROUP(node)
+          ""
+        end
+      end
+
       # Used for formatting urls (url_for)
       class Formatter < Visitor # :nodoc:
         attr_reader :options, :consumed
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index ca31b5e02e..619dd22ec1 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -165,7 +165,7 @@ module ActionDispatch
               super
               @path_parts   = @route.required_parts
               @arg_size     = @path_parts.size
-              @string_route = string_route(route)
+              @string_route = @route.optimized_path
             end
 
             def call(t, args)
@@ -180,14 +180,6 @@ module ActionDispatch
 
             private
 
-            def string_route(route)
-              string_route = route.ast.to_s.dup
-              while string_route.gsub!(/\([^\)]*\)/, "")
-                true
-              end
-              string_route
-            end
-
             def optimized_helper(args)
               path = @string_route.dup
               klass = Journey::Router::Utils
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 89f406a3d0..2bf7056ff7 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -3198,6 +3198,7 @@ class TestOptimizedNamedRoutes < ActionDispatch::IntegrationTest
     app.draw do
       ok = lambda { |env| [200, { 'Content-Type' => 'text/plain' }, []] }
       get '/foo' => ok, as: :foo
+      get '/post(/:action(/:id))' => ok, as: :posts
     end
   end
 
@@ -3215,6 +3216,11 @@ class TestOptimizedNamedRoutes < ActionDispatch::IntegrationTest
   test 'named route called on included module' do
     assert_equal '/foo', foo_path
   end
+
+  test 'nested optional segments are removed' do
+    assert_equal '/post', Routes.url_helpers.posts_path
+    assert_equal '/post', posts_path
+  end
 end
 
 class TestNamedRouteUrlHelpers < ActionDispatch::IntegrationTest