Merge pull request #23140 from rails/fix-search-for-custom-routes

Fix marking of custom routes for Journey

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -184,26 +184,32 @@ def request_method
         def build_path(ast, requirements, anchor)
           pattern = Journey::Path::Pattern.new(ast, requirements, JOINED_SEPARATORS, 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?
-          }.map(&:left)
-
-          # Get all the symbol nodes preceded by literals.
-          symbols.concat ast.find_all { |n|
-            n.cat? && n.left.literal? && n.right.cat? && n.right.left.symbol?
-          }.map { |n| n.right.left }
-
-          symbols.each { |x|
-            x.regexp = /(?:#{Regexp.union(x.regexp, '-')})+/
+          # Find all the symbol nodes that are adjacent to literal nodes and alter
+          # the regexp so that Journey will partition them into custom routes.
+          ast.find_all { |node|
+            next unless node.cat?
+
+            if node.left.literal? && node.right.symbol?
+              symbol = node.right
+            elsif node.left.literal? && node.right.cat? && node.right.left.symbol?
+              symbol = node.right.left
+            elsif node.left.symbol? && node.right.literal?
+              symbol = node.left
+            elsif node.left.symbol? && node.right.cat? && node.right.left.literal?
+              symbol = node.left
+            else
+              next
+            end
+
+            if symbol
+              symbol.regexp = /(?:#{Regexp.union(symbol.regexp, '-')})+/
+            end
           }
 
           pattern
         end
         private :build_path
 
-
         private
           def add_wildcard_options(options, formatted, path_ast)
             # Add a constraint for wildcard route to make it non-greedy and match the

actionpack/test/dispatch/routing_test.rb

@@ -4654,3 +4654,66 @@ def test_legit_routing_not_found_responses
     assert_equal 404, response.status
   end
 end
+
+class TestPartialDynamicPathSegments < ActionDispatch::IntegrationTest
+  Routes = ActionDispatch::Routing::RouteSet.new
+  Routes.draw do
+    ok = lambda { |env| [200, { 'Content-Type' => 'text/plain' }, []] }
+
+    get '/songs/song-:song', to: ok
+    get '/songs/:song-song', to: ok
+    get '/:artist/song-:song', to: ok
+    get '/:artist/:song-song', to: ok
+
+    get '/optional/songs(/song-:song)', to: ok
+    get '/optional/songs(/:song-song)', to: ok
+    get '/optional/:artist(/song-:song)', to: ok
+    get '/optional/:artist(/:song-song)', to: ok
+  end
+
+  APP = build_app Routes
+
+  def app
+    APP
+  end
+
+  def test_paths_with_partial_dynamic_segments_are_recognised
+    get '/david-bowie/changes-song'
+    assert_equal 200, response.status
+    assert_params artist: 'david-bowie', song: 'changes'
+
+    get '/david-bowie/song-changes'
+    assert_equal 200, response.status
+    assert_params artist: 'david-bowie', song: 'changes'
+
+    get '/songs/song-changes'
+    assert_equal 200, response.status
+    assert_params song: 'changes'
+
+    get '/songs/changes-song'
+    assert_equal 200, response.status
+    assert_params song: 'changes'
+
+    get '/optional/songs/song-changes'
+    assert_equal 200, response.status
+    assert_params song: 'changes'
+
+    get '/optional/songs/changes-song'
+    assert_equal 200, response.status
+    assert_params song: 'changes'
+
+    get '/optional/david-bowie/changes-song'
+    assert_equal 200, response.status
+    assert_params artist: 'david-bowie', song: 'changes'
+
+    get '/optional/david-bowie/song-changes'
+    assert_equal 200, response.status
+    assert_params artist: 'david-bowie', song: 'changes'
+  end
+
+  private
+
+  def assert_params(params)
+    assert_equal(params, request.path_parameters)
+  end
+end