Merge pull request #23103 from rails/refactor-handling-of-action-default

Refactor handling of :action default in routing

8 files changed, 73 insertions, 33 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 370e3a1958..5c856f1b36 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,6 +1,11 @@
+*   Routing: Refactor `:action` default handling to ensure that path
+    parameters are not mutated during route generation.
+
+    *Andrew White*
+
 *   Add extension synonyms `yml` and `yaml` for MIME type `application/x-yaml`.
 
-   *bogdanvlviv*
+    *bogdanvlviv*
 
 *   Adds support for including ActionController::Cookies in API controllers.
     Previously, including the module would raise when trying to define
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 5d30a545a2..2459a45827 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 ffd5b83ad3..faa93ecc17 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 16237bd564..c65dafe9e6 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -548,12 +548,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
@@ -572,11 +570,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
@@ -630,13 +623,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 c477b4156c..168677829a 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -2064,11 +2064,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 9d0d23d6de..5aafcb23c2 100644
--- a/actionpack/test/dispatch/routing/inspector_test.rb
+++ b/actionpack/test/dispatch/routing/inspector_test.rb
@@ -347,7 +347,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 09830c0c46..ade4b0c381 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -3991,16 +3991,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
@@ -4012,7 +4002,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
 
@@ -4755,3 +4756,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