Fix routes with :controller segment when namespaced [#5034 state:resolved]

4 files changed, 63 insertions, 30 deletions

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 0d760ed23a..d71005a852 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -91,7 +91,7 @@ module ActionDispatch
 
           def app
             Constraints.new(
-              to.respond_to?(:call) ? to : Routing::RouteSet::Dispatcher.new(:defaults => defaults),
+              to.respond_to?(:call) ? to : Routing::RouteSet::Dispatcher.new(:defaults => defaults, :module => @scope[:module]),
               blocks,
               @set.request_class
             )
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 7248ed03f6..afa312889f 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -14,6 +14,7 @@ module ActionDispatch
         def initialize(options={})
           @defaults = options[:defaults]
           @glob_param = options.delete(:glob)
+          @module = options.delete(:module)
           @controllers = {}
         end
 
@@ -26,7 +27,7 @@ module ActionDispatch
             return [404, {'X-Cascade' => 'pass'}, []]
           end
 
-          controller.action(params[:action]).call(env)
+          dispatch(controller, params[:action], env)
         end
 
         def prepare_params!(params)
@@ -34,29 +35,44 @@ module ActionDispatch
           split_glob_param!(params) if @glob_param
         end
 
-        def controller(params, raise_error=true)
+        # If this is a default_controller (i.e. a controller specified by the user)
+        # we should raise an error in case it's not found, because it usually means
+        # an user error. However, if the controller was retrieved through a dynamic
+        # segment, as in :controller(/:action), we should simply return nil and
+        # delegate the control back to Rack cascade. Besides, if this is not a default 
+        # controller, it means we should respect the @scope[:module] parameter.
+        def controller(params, default_controller=true)
           if params && params.key?(:controller)
-            controller_param = params[:controller]
-            unless controller = @controllers[controller_param]
-              controller_name = "#{controller_param.camelize}Controller"
-              controller = @controllers[controller_param] =
-                ActiveSupport::Dependencies.ref(controller_name)
-            end
-
-            controller.get
+            controller_param = @module && !default_controller ?
+              "#{@module}/#{params[:controller]}" : params[:controller]
+            controller_reference(controller_param)
           end
         rescue NameError => e
-          raise ActionController::RoutingError, e.message, e.backtrace if raise_error
+          raise ActionController::RoutingError, e.message, e.backtrace if default_controller
         end
 
-        private
-          def merge_default_action!(params)
-            params[:action] ||= 'index'
-          end
+      private
 
-          def split_glob_param!(params)
-            params[@glob_param] = params[@glob_param].split('/').map { |v| URI.unescape(v) }
+        def controller_reference(controller_param)
+          unless controller = @controllers[controller_param]
+            controller_name = "#{controller_param.camelize}Controller"
+            controller = @controllers[controller_param] =
+              ActiveSupport::Dependencies.ref(controller_name)
           end
+          controller.get
+        end
+
+        def dispatch(controller, action, env)
+          controller.action(action).call(env)
+        end
+
+        def merge_default_action!(params)
+          params[:action] ||= 'index'
+        end
+
+        def split_glob_param!(params)
+          params[@glob_param] = params[@glob_param].split('/').map { |v| URI.unescape(v) }
+        end
       end
 
       # A NamedRouteCollection instance is a collection of named routes, and also
diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb
index 89c4cb46a1..5be47f7c96 100644
--- a/actionpack/test/abstract_unit.rb
+++ b/actionpack/test/abstract_unit.rb
@@ -182,13 +182,16 @@ class ActionController::IntegrationTest < ActiveSupport::TestCase
 
   self.app = build_app
 
-  class StubDispatcher
-    def self.new(*args)
-      lambda { |env|
-        params = env['action_dispatch.request.path_parameters']
-        controller, action = params[:controller], params[:action]
-        [200, {'Content-Type' => 'text/html'}, ["#{controller}##{action}"]]
-      }
+  # Stub Rails dispatcher so it does not get controller references and
+  # simply return the controller#action as Rack::Body.
+  class StubDispatcher < ::ActionDispatch::Routing::RouteSet::Dispatcher
+    protected
+    def controller_reference(controller_param)
+      controller_param
+    end
+
+    def dispatch(controller, action, env)
+      [200, {'Content-Type' => 'text/html'}, ["#{controller}##{action}"]]
     end
   end
 
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 26bd641cd6..1bfc92aa3d 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -310,11 +310,6 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
         match '/' => 'mes#index'
       end
 
-      namespace :private do
-        root :to => redirect('/private/index')
-        match "index", :to => 'private#index'
-      end
-
       get "(/:username)/followers" => "followers#index"
       get "/groups(/user/:username)" => "groups#index"
       get "(/user/:username)/photos" => "photos#index"
@@ -348,6 +343,12 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
         end
       end
 
+      namespace :private do
+        root :to => redirect('/private/index')
+        match "index", :to => 'private#index'
+        match ":controller(/:action(/:id))"
+      end
+
       match '/:locale/*file.:format', :to => 'files#show', :file => /path\/to\/existing\/file/
     end
   end
@@ -470,6 +471,19 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     end
   end
 
+  def test_namespace_with_controller_segment
+    with_test_routes do
+      get '/private/foo'
+      assert_equal 'private/foo#index', @response.body
+
+      get '/private/foo/bar'
+      assert_equal 'private/foo#bar', @response.body
+
+      get '/private/foo/bar/1'
+      assert_equal 'private/foo#bar', @response.body
+    end
+  end
+
   def test_session_singleton_resource
     with_test_routes do
       get '/session'