From 1eace9402ba9ad17b51552b5c61e25c3ced84475 Mon Sep 17 00:00:00 2001
From: Andrew White <andrew.white@unboxedconsulting.com>
Date: Wed, 20 Jan 2016 17:59:13 +0000
Subject: Fix marking of custom routes for Journey

The Mapper build_path method marks routes where path parameters are part
of a path segment as custom routes by altering the regular expression, e.g:

    get '/foo-:bar', to: 'foo#bar'

There were some edge cases where certain constructs weren't being picked
up and this commit fixes those.

Fixes #23069.
---
 actionpack/lib/action_dispatch/routing/mapper.rb | 34 +++++++------
 actionpack/test/dispatch/routing_test.rb         | 63 ++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 14 deletions(-)

(limited to 'actionpack')

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 522012063d..afbaa45d20 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -184,26 +184,32 @@ module ActionDispatch
         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
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 62d65ec5c0..5ead9357ae 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -4654,3 +4654,66 @@ class TestErrorsInController < ActionDispatch::IntegrationTest
     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
-- 
cgit v1.2.3