Refactor handling of :action default in routing

The longstanding convention in Rails is that if the :action parameter
is missing or nil then it defaults to 'index'. Up until Rails 5.0.0.beta1
this was handled slightly differently than other routing defaults by
deleting it from the route options and adding it to the recall parameters.

With the recent focus of removing unnecessary duplications this has
exposed a problem in this strategy - we are now mutating the request's
path parameters and causing problems for later url generation. This will
typically affect url_for rather a named url helper since the latter
explicitly pass :controller, :action, etc.

The fix is to add a default for :action in the route class if the path
contains an :action segment and no default is passed. This change also
revealed an issue with the parameterized part expiry in that it doesn't
follow a right to left order - as soon as a dynamic segment is required
then all other segments become required.

Fixes #23019.

8 files changed, 74 insertions, 32 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 931313612c..de256a59ee 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Refactor handling of `:action` default in routing so that the request's
+    `path_parameters` is not mutated when generating a route.
+
+    Fixes #23019.
+
+    *Andrew White*
+
 *   Add request encoding and response parsing to integration tests.
 
     What previously was:
diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb
index 0323360faa..200477b002 100644
--- a/actionpack/lib/action_dispatch/journey/formatter.rb
+++ b/actionpack/lib/action_dispatch/journey/formatter.rb
@@ -32,8 +32,13 @@ module ActionDispatch
 
           defaults       = route.defaults
           required_parts = route.required_parts
-          parameterized_parts.keep_if do |key, value|
-            (defaults[key].nil? && value.present?) || value.to_s != defaults[key].to_s || required_parts.include?(key)
+
+          route.parts.reverse_each do |key|
+            break if defaults[key].nil? && parameterized_parts[key].present?
+            break if parameterized_parts[key].to_s != defaults[key].to_s
+            break if required_parts.include?(key)
+
+            parameterized_parts.delete(key)
           end
 
           return [route.format(parameterized_parts), params]
diff --git a/actionpack/lib/action_dispatch/routing/inspector.rb b/actionpack/lib/action_dispatch/routing/inspector.rb
index 983f1daeb3..4ee5717b3e 100644
--- a/actionpack/lib/action_dispatch/routing/inspector.rb
+++ b/actionpack/lib/action_dispatch/routing/inspector.rb
@@ -33,11 +33,11 @@ module ActionDispatch
       end
 
       def controller
-        requirements[:controller] || ':controller'
+        parts.include?(:controller) ? ':controller' : requirements[:controller]
       end
 
       def action
-        requirements[:action] || ':action'
+        parts.include?(:action) ? ':action' : requirements[:action]
       end
 
       def internal?
diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index afbaa45d20..92205a89f6 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -137,6 +137,10 @@ module ActionDispatch
           @conditions = Hash[conditions]
           @defaults = formats[:defaults].merge(@defaults).merge(normalize_defaults(options))
 
+          if path_params.include?(:action) && !@requirements.key?(:action)
+            @defaults[:action] ||= 'index'
+          end
+
           @required_defaults = (split_options[:required_defaults] || []).map(&:first)
         end
 
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 310e98f584..de291d249b 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -533,12 +533,10 @@ module ActionDispatch
           @recall      = recall
           @set         = set
 
-          normalize_recall!
           normalize_options!
           normalize_controller_action_id!
           use_relative_controller!
           normalize_controller!
-          normalize_action!
         end
 
         def controller
@@ -557,11 +555,6 @@ module ActionDispatch
           end
         end
 
-        # Set 'index' as default action for recall
-        def normalize_recall!
-          @recall[:action] ||= 'index'
-        end
-
         def normalize_options!
           # If an explicit :controller was given, always make :action explicit
           # too, so that action expiry works as expected for things like
@@ -615,13 +608,6 @@ module ActionDispatch
           end
         end
 
-        # Move 'index' action from options to recall
-        def normalize_action!
-          if @options[:action] == 'index'.freeze
-            @recall[:action] = @options.delete(:action)
-          end
-        end
-
         # Generates a path from routes, returns [path, params].
         # If no route is generated the formatter will raise ActionController::UrlGenerationError
         def generate
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index a39fede5b9..9dabd58d69 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -1964,11 +1964,11 @@ class RackMountIntegrationTests < ActiveSupport::TestCase
   def test_extras
     params = {:controller => 'people'}
     assert_equal [], @routes.extra_keys(params)
-    assert_equal({:controller => 'people'}, params)
+    assert_equal({:controller => 'people', :action => 'index'}, params)
 
     params = {:controller => 'people', :foo => 'bar'}
     assert_equal [:foo], @routes.extra_keys(params)
-    assert_equal({:controller => 'people', :foo => 'bar'}, params)
+    assert_equal({:controller => 'people', :action => 'index', :foo => 'bar'}, params)
 
     params = {:controller => 'people', :action => 'create', :person => { :name => 'Josh'}}
     assert_equal [:person], @routes.extra_keys(params)
diff --git a/actionpack/test/dispatch/routing/inspector_test.rb b/actionpack/test/dispatch/routing/inspector_test.rb
index f72a87b994..642769681b 100644
--- a/actionpack/test/dispatch/routing/inspector_test.rb
+++ b/actionpack/test/dispatch/routing/inspector_test.rb
@@ -339,7 +339,7 @@ module ActionDispatch
         end
 
         assert_equal ["Prefix Verb URI Pattern            Controller#Action",
-                      "       GET  /:controller(/:action) (?-mix:api\\/[^\\/]+)#:action"], output
+                      "       GET  /:controller(/:action) :controller#:action"], output
       end
 
       def test_inspect_routes_shows_resources_route_when_assets_disabled
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 5ead9357ae..02cd640fa5 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -3964,16 +3964,6 @@ class TestUnicodePaths < ActionDispatch::IntegrationTest
 end
 
 class TestMultipleNestedController < ActionDispatch::IntegrationTest
-  module ::Foo
-    module Bar
-      class BazController < ActionController::Base
-        def index
-          render :inline => "<%= url_for :controller => '/pooh', :action => 'index' %>"
-        end
-      end
-    end
-  end
-
   Routes = ActionDispatch::Routing::RouteSet.new.tap do |app|
     app.draw do
       namespace :foo do
@@ -3985,7 +3975,18 @@ class TestMultipleNestedController < ActionDispatch::IntegrationTest
     end
   end
 
-  include Routes.url_helpers
+  module ::Foo
+    module Bar
+      class BazController < ActionController::Base
+        include Routes.url_helpers
+
+        def index
+          render :inline => "<%= url_for :controller => '/pooh', :action => 'index' %>"
+        end
+      end
+    end
+  end
+
   APP = build_app Routes
   def app; APP end
 
@@ -4717,3 +4718,42 @@ class TestPartialDynamicPathSegments < ActionDispatch::IntegrationTest
     assert_equal(params, request.path_parameters)
   end
 end
+
+class TestPathParameters < ActionDispatch::IntegrationTest
+  Routes = ActionDispatch::Routing::RouteSet.new.tap do |app|
+    app.draw do
+      scope module: 'test_path_parameters' do
+        scope ':locale', locale: /en|ar/ do
+          root to: 'home#index'
+          get '/about', to: 'pages#about'
+        end
+      end
+
+      get ':controller(/:action/(:id))'
+    end
+  end
+
+  class HomeController < ActionController::Base
+    include Routes.url_helpers
+
+    def index
+      render inline: "<%= root_path %>"
+    end
+  end
+
+  class PagesController < ActionController::Base
+    include Routes.url_helpers
+
+    def about
+      render inline: "<%= root_path(locale: :ar) %> | <%= url_for(locale: :ar) %>"
+    end
+  end
+
+  APP = build_app Routes
+  def app; APP end
+
+  def test_path_parameters_are_not_mutated
+    get '/en/about'
+    assert_equal "/ar | /ar/about", @response.body
+  end
+end