0001-prevent-namespace-with-dynamic-controller.diff

From 210b89938aea9bd514379f2a509049f173c1a56f Mon Sep 17 00:00:00 2001
From: Andrew White <[email protected]>
Date: Tue, 6 Jul 2010 22:20:06 +0100
Subject: [PATCH] When a dynamic :controller segment is present in the path add a
 Regexp constraint that allow matching on multiple path segments.

Using a namespace block isn't compatible with dynamic routes so we
raise an ArgumentError if we detect a :module present in the scope.

[#5052 state:resolved]
---
 actionpack/lib/action_dispatch/routing/mapper.rb   |   25 ++++++++++---
 .../lib/action_dispatch/routing/route_set.rb       |    6 +--
 actionpack/test/dispatch/routing_test.rb           |   36 ++++++++++++++------
 3 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index aab272f..430f6fc 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -65,6 +65,16 @@ module ActionDispatch
               end
             end
 
+            if path.match(':controller')
+              raise ArgumentError, ":controller segment is not allowed within a namespace block" if @scope[:module]
+
+              # Add a default constraint for :controller path segments that matches namespaced
+              # controllers with default routes like :controller/:action/:id(.:format), e.g:
+              # GET /admin/products/show/1
+              # => { :controller => 'admin/products', :action => 'show', :id => '1' }
+              options.reverse_merge!(:controller => /.+?/)
+            end
+
             path = normalize_path(path)
             path_without_format = path.sub(/\(\.:format\)$/, '')
 
@@ -93,7 +103,7 @@ module ActionDispatch
 
           def app
             Constraints.new(
-              to.respond_to?(:call) ? to : Routing::RouteSet::Dispatcher.new(:defaults => defaults, :module => @scope[:module]),
+              to.respond_to?(:call) ? to : Routing::RouteSet::Dispatcher.new(:defaults => defaults),
               blocks,
               @set.request_class
             )
@@ -135,8 +145,11 @@ module ActionDispatch
               defaults[:controller] ||= default_controller
               defaults[:action]     ||= default_action
 
-              defaults.delete(:controller) if defaults[:controller].blank?
-              defaults.delete(:action)     if defaults[:action].blank?
+              defaults.delete(:controller) if defaults[:controller].blank? || defaults[:controller].is_a?(Regexp)
+              defaults.delete(:action)     if defaults[:action].blank? || defaults[:action].is_a?(Regexp)
+
+              defaults[:controller] = defaults[:controller].to_s if defaults.key?(:controller)
+              defaults[:action] = defaults[:action].to_s if defaults.key?(:action)
 
               if defaults[:controller].blank? && segment_keys.exclude?("controller")
                 raise ArgumentError, "missing :controller"
@@ -185,15 +198,15 @@ module ActionDispatch
 
           def default_controller
             if @options[:controller]
-              @options[:controller].to_s
+              @options[:controller]
             elsif @scope[:controller]
-              @scope[:controller].to_s
+              @scope[:controller]
             end
           end
 
           def default_action
             if @options[:action]
-              @options[:action].to_s
+              @options[:action]
             end
           end
       end
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 01a068a..1b1a221 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -14,7 +14,6 @@ module ActionDispatch
         def initialize(options={})
           @defaults = options[:defaults]
           @glob_param = options.delete(:glob)
-          @module = options.delete(:module)
           @controllers = {}
         end
 
@@ -39,12 +38,11 @@ module ActionDispatch
         # 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 
+        # 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 = @module && !default_controller ?
-              "#{@module}/#{params[:controller]}" : params[:controller]
+            controller_param = params[:controller]
             controller_reference(controller_param)
           end
         rescue NameError => e
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 90d05d4..2a014bf 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -323,7 +323,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
         end
       end
 
-      match "whatever/:controller(/:action(/:id))"
+      match "whatever/:controller(/:action(/:id))", :id => /\d+/
 
       resource :profile do
         get :settings
@@ -349,7 +349,6 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
       namespace :private do
         root :to => redirect('/private/index')
         match "index", :to => 'private#index'
-        match ":controller(/:action(/:id))"
       end
 
       scope :only => [:index, :show] do
@@ -527,15 +526,14 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
   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
+    assert_raise(ArgumentError) do
+      self.class.stub_controllers do |routes|
+        routes.draw do
+          namespace :admin do
+            match '/:controller(/:action(/:id(.:format)))'
+          end
+        end
+      end
     end
   end
 
@@ -1354,6 +1352,22 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     end
   end
 
+  def test_url_generator_for_namespaced_generic_route
+    with_test_routes do
+      get 'whatever/foo/bar/show'
+      assert_equal 'foo/bar#show', @response.body
+
+      get 'whatever/foo/bar/show/1'
+      assert_equal 'foo/bar#show', @response.body
+
+      assert_equal 'http://www.example.com/whatever/foo/bar/show',
+        url_for(:controller => "foo/bar", :action => "show")
+
+      assert_equal 'http://www.example.com/whatever/foo/bar/show/1',
+        url_for(:controller => "foo/bar", :action => "show", :id => '1')
+    end
+  end
+
   def test_assert_recognizes_account_overview
     with_test_routes do
       assert_recognizes({:controller => "account", :action => "overview"}, "/account/overview")
-- 
1.7.1