Resource member routes require :id, eliminating the ambiguous overlap with collection routes. Closes #7229.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@6062 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

3 files changed, 142 insertions, 72 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index c9ab508ff4..8d55653916 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Resource member routes require :id, eliminating the ambiguous overlap with collection routes.  #7229 [dkubb]
+
 * Remove deprecated assertions.  [Jeremy Kemper]
 
 * Change session restoration to allow namespaced models to be autoloaded. Closes #6348. [Nicholas Seckar]
diff --git a/actionpack/lib/action_controller/resources.rb b/actionpack/lib/action_controller/resources.rb
index f23ebfb813..5e15bc8f3a 100644
--- a/actionpack/lib/action_controller/resources.rb
+++ b/actionpack/lib/action_controller/resources.rb
@@ -322,74 +322,82 @@ module ActionController
 
       def map_collection_actions(map, resource)
         resource.collection_methods.each do |method, actions|
-          route_options = requirements_for(method)
-
           actions.each do |action|
-            map.named_route(
-              "#{resource.name_prefix}#{action}_#{resource.plural}", 
-              "#{resource.path};#{action}", 
-              route_options.merge(:action => action.to_s)
-            )
-
-            map.named_route(
-              "formatted_#{resource.name_prefix}#{action}_#{resource.plural}",
-              "#{resource.path}.:format;#{action}",
-              route_options.merge(:action => action.to_s)
-            )
+            action_options = action_options_for(action, resource, method)
+            map.named_route("#{resource.name_prefix}#{action}_#{resource.plural}", "#{resource.path};#{action}", action_options)
+            map.named_route("formatted_#{resource.name_prefix}#{action}_#{resource.plural}", "#{resource.path}.:format;#{action}", action_options)
           end
         end
       end
 
       def map_default_collection_actions(map, resource)
-        map.named_route("#{resource.name_prefix}#{resource.plural}", resource.path, :action => "index", :conditions => { :method => :get })
-        map.named_route("formatted_#{resource.name_prefix}#{resource.plural}", "#{resource.path}.:format", :action => "index", :conditions => { :method => :get })
+        index_action_options = action_options_for("index", resource)
+        map.named_route("#{resource.name_prefix}#{resource.plural}", resource.path, index_action_options)
+        map.named_route("formatted_#{resource.name_prefix}#{resource.plural}", "#{resource.path}.:format", index_action_options)
 
-        map.connect(resource.path, :action => "create", :conditions => { :method => :post })
-        map.connect("#{resource.path}.:format", :action => "create", :conditions => { :method => :post })
+        create_action_options = action_options_for("create", resource)
+        map.connect(resource.path, create_action_options)
+        map.connect("#{resource.path}.:format", create_action_options)
       end
 
       def map_default_singleton_actions(map, resource)
-        map.connect(resource.path, :action => "create", :conditions => { :method => :post })
-        map.connect("#{resource.path}.:format", :action => "create", :conditions => { :method => :post })
+        create_action_options = action_options_for("create", resource)
+        map.connect(resource.path, create_action_options)
+        map.connect("#{resource.path}.:format", create_action_options)
       end
 
       def map_new_actions(map, resource)
         resource.new_methods.each do |method, actions|
-          route_options = requirements_for(method)
           actions.each do |action|
+            action_options = action_options_for(action, resource, method)
             if action == :new
-              map.named_route("#{resource.name_prefix}new_#{resource.singular}", resource.new_path, route_options.merge(:action => "new"))
-              map.named_route("formatted_#{resource.name_prefix}new_#{resource.singular}", "#{resource.new_path}.:format", route_options.merge(:action => "new"))
+              map.named_route("#{resource.name_prefix}new_#{resource.singular}", resource.new_path, action_options)
+              map.named_route("formatted_#{resource.name_prefix}new_#{resource.singular}", "#{resource.new_path}.:format", action_options)
             else
-              map.named_route("#{resource.name_prefix}#{action}_new_#{resource.singular}", "#{resource.new_path};#{action}", route_options.merge(:action => action.to_s))
-              map.named_route("formatted_#{resource.name_prefix}#{action}_new_#{resource.singular}", "#{resource.new_path}.:format;#{action}", route_options.merge(:action => action.to_s))
+              map.named_route("#{resource.name_prefix}#{action}_new_#{resource.singular}", "#{resource.new_path};#{action}", action_options)
+              map.named_route("formatted_#{resource.name_prefix}#{action}_new_#{resource.singular}", "#{resource.new_path}.:format;#{action}", action_options)
             end
           end
         end
       end
-      
+
       def map_member_actions(map, resource)
         resource.member_methods.each do |method, actions|
-          route_options = requirements_for(method)
-
           actions.each do |action|
-            map.named_route("#{resource.name_prefix}#{action}_#{resource.singular}", "#{resource.member_path};#{action}", route_options.merge(:action => action.to_s))
-            map.named_route("formatted_#{resource.name_prefix}#{action}_#{resource.singular}", "#{resource.member_path}.:format;#{action}", route_options.merge(:action => action.to_s))
+            action_options = action_options_for(action, resource, method)
+            map.named_route("#{resource.name_prefix}#{action}_#{resource.singular}", "#{resource.member_path};#{action}", action_options)
+            map.named_route("formatted_#{resource.name_prefix}#{action}_#{resource.singular}", "#{resource.member_path}.:format;#{action}",action_options)
           end
         end
 
-        map.named_route("#{resource.name_prefix}#{resource.singular}", resource.member_path, :action => "show", :conditions => { :method => :get })
-        map.named_route("formatted_#{resource.name_prefix}#{resource.singular}", "#{resource.member_path}.:format", :action => "show", :conditions => { :method => :get })
+        show_action_options = action_options_for("show", resource)
+        map.named_route("#{resource.name_prefix}#{resource.singular}", resource.member_path, show_action_options)
+        map.named_route("formatted_#{resource.name_prefix}#{resource.singular}", "#{resource.member_path}.:format", show_action_options)
+
+        update_action_options = action_options_for("update", resource)
+        map.connect(resource.member_path, update_action_options)
+        map.connect("#{resource.member_path}.:format", update_action_options)
 
-        map.connect(resource.member_path, :action => "update", :conditions => { :method => :put })
-        map.connect("#{resource.member_path}.:format", :action => "update", :conditions => { :method => :put })
+        destroy_action_options = action_options_for("destroy", resource)
+        map.connect(resource.member_path, destroy_action_options)
+        map.connect("#{resource.member_path}.:format", destroy_action_options)
+      end
 
-        map.connect(resource.member_path, :action => "destroy", :conditions => { :method => :delete })
-        map.connect("#{resource.member_path}.:format", :action => "destroy", :conditions => { :method => :delete })
+      def conditions_for(method)
+        { :conditions => method == :any ? {} : { :method => method } }
       end
 
-      def requirements_for(method)
-        method == :any ? {} : { :conditions => { :method => method } }
+      def action_options_for(action, resource, method = nil)
+        default_options = { :action => action.to_s }
+        require_id = resource.kind_of?(SingletonResource) ? {} : { :requirements => { :id => Regexp.new("[^#{Routing::SEPARATORS.join}]+") } }
+        case default_options[:action]
+          when "index", "new" : default_options.merge(conditions_for(method || :get))
+          when "create"       : default_options.merge(conditions_for(method || :post))
+          when "show", "edit" : default_options.merge(conditions_for(method || :get)).merge(require_id)
+          when "update"       : default_options.merge(conditions_for(method || :put)).merge(require_id)
+          when "destroy"      : default_options.merge(conditions_for(method || :delete)).merge(require_id)
+          else                  default_options.merge(conditions_for(method))
+        end
       end
   end
 end
diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb
index e424df3514..e5605dff62 100644
--- a/actionpack/test/controller/resources_test.rb
+++ b/actionpack/test/controller/resources_test.rb
@@ -27,6 +27,23 @@ class ResourcesTest < Test::Unit::TestCase
     assert_resource_methods [:new, :preview, :draft], resource, :new,        :get
   end
 
+  def test_should_resource_controller_name_equal_resource_name_by_default
+    resource = ActionController::Resources::Resource.new(:messages, {})
+    assert_equal 'messages', resource.controller
+  end
+
+  def test_should_resource_controller_name_equal_controller_option
+    resource = ActionController::Resources::Resource.new(:messages, :controller => 'posts')
+    assert_equal 'posts', resource.controller
+  end
+
+  def test_should_all_singleton_paths_be_the_same
+    [ :path, :nesting_path_prefix, :member_path ].each do |method|
+      resource = ActionController::Resources::SingletonResource.new(:messages, :path_prefix => 'admin')
+      assert_equal 'admin/messages', resource.send(method)
+    end
+  end
+
   def test_default_restful_routes
     with_restful_routing :messages do
       assert_simply_restful_for :messages
@@ -116,7 +133,6 @@ class ResourcesTest < Test::Unit::TestCase
     end
   end
 
-
   def test_with_new_action
     with_restful_routing :messages, :new => { :preview => :post } do
       preview_options = {:action => 'preview'}
@@ -280,6 +296,22 @@ class ResourcesTest < Test::Unit::TestCase
     end
   end
 
+  def test_should_not_allow_delete_or_put_on_collection_path
+    controller_name = :messages
+    with_restful_routing controller_name do
+      options = { :controller => controller_name.to_s }
+      collection_path = "/#{controller_name}"
+
+      assert_raises(ActionController::RoutingError) do
+        assert_recognizes(options.merge(:action => 'update'), :path => collection_path, :method => :put)
+      end
+
+      assert_raises(ActionController::RoutingError) do
+        assert_recognizes(options.merge(:action => 'destroy'), :path => collection_path, :method => :delete)
+      end
+    end
+  end
+
   protected
     def with_restful_routing(*args)
       with_routing do |set|
@@ -309,30 +341,38 @@ class ResourcesTest < Test::Unit::TestCase
     def assert_restful_routes_for(controller_name, options = {})
       (options[:options] ||= {})[:controller] = controller_name.to_s
 
-      collection_path = "/#{options[:path_prefix]}#{controller_name}"
-      member_path = "#{collection_path}/1"
-      new_path = "#{collection_path}/new"
+      collection_path            = "/#{options[:path_prefix]}#{controller_name}"
+      member_path                = "#{collection_path}/1"
+      new_path                   = "#{collection_path}/new"
+      edit_member_path           = "#{member_path};edit"
+      formatted_edit_member_path = "#{member_path}.xml;edit"
 
       with_options(options[:options]) do |controller|
         controller.assert_routing collection_path,            :action => 'index'
-        controller.assert_routing "#{collection_path}.xml" ,  :action => 'index', :format => 'xml'
         controller.assert_routing new_path,                   :action => 'new'
         controller.assert_routing member_path,                :action => 'show', :id => '1'
-        controller.assert_routing "#{member_path};edit",      :action => 'edit', :id => '1'
+        controller.assert_routing edit_member_path,           :action => 'edit', :id => '1'
+        controller.assert_routing "#{collection_path}.xml",   :action => 'index',            :format => 'xml'
+        controller.assert_routing "#{new_path}.xml",          :action => 'new',              :format => 'xml'
         controller.assert_routing "#{member_path}.xml",       :action => 'show', :id => '1', :format => 'xml'
+        controller.assert_routing formatted_edit_member_path, :action => 'edit', :id => '1', :format => 'xml'
       end
 
-      assert_recognizes(
-        options[:options].merge(:action => 'create'),
-        :path => collection_path, :method => :post)
-
-      assert_recognizes(
-        options[:options].merge(:action => 'update', :id => '1'),
-        :path => member_path, :method => :put)
-
-      assert_recognizes(
-        options[:options].merge(:action => 'destroy', :id => '1'),
-        :path => member_path, :method => :delete)
+      assert_recognizes(options[:options].merge(:action => 'index'),               :path => collection_path,  :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'new'),                 :path => new_path,         :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'create'),              :path => collection_path,  :method => :post)
+      assert_recognizes(options[:options].merge(:action => 'show',    :id => '1'), :path => member_path,      :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'edit',    :id => '1'), :path => edit_member_path, :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'update',  :id => '1'), :path => member_path,      :method => :put)
+      assert_recognizes(options[:options].merge(:action => 'destroy', :id => '1'), :path => member_path,      :method => :delete)
+
+      assert_recognizes(options[:options].merge(:action => 'index',               :format => 'xml'), :path => "#{collection_path}.xml",   :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'new',                 :format => 'xml'), :path => "#{new_path}.xml",          :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'create',              :format => 'xml'), :path => "#{collection_path}.xml",   :method => :post)
+      assert_recognizes(options[:options].merge(:action => 'show',    :id => '1', :format => 'xml'), :path => "#{member_path}.xml",       :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'edit',    :id => '1', :format => 'xml'), :path => formatted_edit_member_path, :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'update',  :id => '1', :format => 'xml'), :path => "#{member_path}.xml",       :method => :put)
+      assert_recognizes(options[:options].merge(:action => 'destroy', :id => '1', :format => 'xml'), :path => "#{member_path}.xml",       :method => :delete)
 
       yield options[:options] if block_given?
     end
@@ -354,30 +394,48 @@ class ResourcesTest < Test::Unit::TestCase
       full_prefix = "/#{options[:path_prefix]}#{controller_name}"
       name_prefix = options[:name_prefix]
 
-      assert_named_route "#{full_prefix}",        "#{name_prefix}#{controller_name}_path",           options[:options]
-      assert_named_route "#{full_prefix}.xml",    "formatted_#{name_prefix}#{controller_name}_path", options[:options].merge(:format => 'xml')
-      assert_named_route "#{full_prefix}/new",    "#{name_prefix}new_#{singular_name}_path",         options[:options]
-      assert_named_route "#{full_prefix}/1",      "#{name_prefix}#{singular_name}_path",             options[:options].merge(:id => '1')
-      assert_named_route "#{full_prefix}/1;edit", "#{name_prefix}edit_#{singular_name}_path",        options[:options].merge(:id => '1')
-      assert_named_route "#{full_prefix}/1.xml",  "formatted_#{name_prefix}#{singular_name}_path",   options[:options].merge(:format => 'xml', :id => '1')
+      assert_named_route "#{full_prefix}",            "#{name_prefix}#{controller_name}_path",              options[:options]
+      assert_named_route "#{full_prefix}/new",        "#{name_prefix}new_#{singular_name}_path",            options[:options]
+      assert_named_route "#{full_prefix}/1",          "#{name_prefix}#{singular_name}_path",                options[:options].merge(:id => '1')
+      assert_named_route "#{full_prefix}/1;edit",     "#{name_prefix}edit_#{singular_name}_path",           options[:options].merge(:id => '1')
+      assert_named_route "#{full_prefix}.xml",        "formatted_#{name_prefix}#{controller_name}_path",    options[:options].merge(            :format => 'xml')
+      assert_named_route "#{full_prefix}/new.xml",    "formatted_#{name_prefix}new_#{singular_name}_path",  options[:options].merge(            :format => 'xml')
+      assert_named_route "#{full_prefix}/1.xml",      "formatted_#{name_prefix}#{singular_name}_path",      options[:options].merge(:id => '1', :format => 'xml')
+      assert_named_route "#{full_prefix}/1.xml;edit", "formatted_#{name_prefix}edit_#{singular_name}_path", options[:options].merge(:id => '1', :format => 'xml')
       yield options[:options] if block_given?
     end
 
     def assert_singleton_routes_for(singleton_name, options = {})
       (options[:options] ||= {})[:controller] ||= singleton_name.to_s
-      
-      full_path = "/#{options[:path_prefix]}#{singleton_name}"
+
+      full_path           = "/#{options[:path_prefix]}#{singleton_name}"
+      new_path            = "#{full_path}/new"
+      edit_path           = "#{full_path};edit"
+      formatted_edit_path = "#{full_path}.xml;edit"
+
       with_options options[:options] do |controller|
-        controller.assert_routing full_path,          :action => 'show'
-        controller.assert_routing "#{full_path}.xml", :action => 'show', :format => 'xml'
-        controller.assert_routing "#{full_path}/new", :action => 'new'
-        controller.assert_routing "#{full_path};edit", :action => 'edit'
+        controller.assert_routing full_path,           :action => 'show'
+        controller.assert_routing new_path,            :action => 'new'
+        controller.assert_routing edit_path,           :action => 'edit'
+        controller.assert_routing "#{full_path}.xml",  :action => 'show', :format => 'xml'
+        controller.assert_routing "#{new_path}.xml",   :action => 'new',  :format => 'xml'
+        controller.assert_routing formatted_edit_path, :action => 'edit', :format => 'xml'
       end
 
+      assert_recognizes(options[:options].merge(:action => 'show'),    :path => full_path, :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'new'),     :path => new_path,  :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'edit'),    :path => edit_path, :method => :get)
       assert_recognizes(options[:options].merge(:action => 'create'),  :path => full_path, :method => :post)
       assert_recognizes(options[:options].merge(:action => 'update'),  :path => full_path, :method => :put)
       assert_recognizes(options[:options].merge(:action => 'destroy'), :path => full_path, :method => :delete)
 
+      assert_recognizes(options[:options].merge(:action => 'show',    :format => 'xml'), :path => "#{full_path}.xml",  :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'new',     :format => 'xml'), :path => "#{new_path}.xml",   :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'edit',    :format => 'xml'), :path => formatted_edit_path, :method => :get)
+      assert_recognizes(options[:options].merge(:action => 'create',  :format => 'xml'), :path => "#{full_path}.xml",  :method => :post)
+      assert_recognizes(options[:options].merge(:action => 'update',  :format => 'xml'), :path => "#{full_path}.xml",  :method => :put)
+      assert_recognizes(options[:options].merge(:action => 'destroy', :format => 'xml'), :path => "#{full_path}.xml",  :method => :delete)
+
       yield options[:options] if block_given?
     end
 
@@ -388,13 +446,15 @@ class ResourcesTest < Test::Unit::TestCase
       @response   = ActionController::TestResponse.new
       get :show, options[:options]
       options[:options].delete :action
-      
+
       full_path = "/#{options[:path_prefix]}#{singleton_name}"
-      
-      assert_named_route "#{full_path}",    "#{singleton_name}_path",            options[:options]
-      assert_named_route "#{full_path}.xml", "formatted_#{singleton_name}_path", options[:options].merge(:format => 'xml')
-      assert_named_route "#{full_path}/new", "new_#{singleton_name}_path", options[:options]
-      assert_named_route "#{full_path};edit", "edit_#{singleton_name}_path", options[:options]
+
+      assert_named_route "#{full_path}",          "#{singleton_name}_path",                options[:options]
+      assert_named_route "#{full_path}/new",      "new_#{singleton_name}_path",            options[:options]
+      assert_named_route "#{full_path};edit",     "edit_#{singleton_name}_path",           options[:options]
+      assert_named_route "#{full_path}.xml",      "formatted_#{singleton_name}_path",      options[:options].merge(:format => 'xml')
+      assert_named_route "#{full_path}/new.xml",  "formatted_new_#{singleton_name}_path",  options[:options].merge(:format => 'xml')
+      assert_named_route "#{full_path}.xml;edit", "formatted_edit_#{singleton_name}_path", options[:options].merge(:format => 'xml')
     end
 
     def assert_named_route(expected, route, options)