Fix routing to respect user provided requirements and defaults when assigning default routing options (such as :action => 'index'). Closes #5950.

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

3 files changed, 90 insertions, 9 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 765ef51029..eb4c5f4088 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Fix routing to respect user provided requirements and defaults when assigning default routing options (such as :action => 'index'). Closes #5950. [Nicholas Seckar]
+
 * Rescue Errno::ECONNRESET to handle an unexpectedly closed socket connection. Improves SCGI reliability.  #3368, #6226 [sdsykes, fhanshaw@vesaria.com]
 
 * Added that respond_to blocks will automatically set the content type to be the same as is requested [DHH]. Examples:
diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index 1650c86b2a..c309878dbd 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -820,8 +820,6 @@ module ActionController
           when /\A:(\w+)/
             key = $1.to_sym
             case key
-              when :action then DynamicSegment.new(key, :default => 'index')
-              when :id then DynamicSegment.new(key, :optional => true)
               when :controller then ControllerSegment.new(key)
               else DynamicSegment.new key
             end
@@ -862,11 +860,11 @@ module ActionController
       # are returned as a hash.
       def assign_route_options(segments, defaults, requirements)
         route_requirements = {} # Requirements that do not belong to a segment
-    
+        
         segment_named = Proc.new do |key|
           segments.detect { |segment| segment.key == key if segment.respond_to?(:key) }
         end
-    
+        
         requirements.each do |key, requirement|
           segment = segment_named[key]
           if segment
@@ -879,18 +877,39 @@ module ActionController
             route_requirements[key] = requirement
           end
         end
-    
+        
         defaults.each do |key, default|
           segment = segment_named[key]
           raise ArgumentError, "#{key}: No matching segment exists; cannot assign default" unless segment
           segment.is_optional = true
           segment.default = default.to_param if default
         end
-
+        
+        assign_default_route_options(segments)
         ensure_required_segments(segments)
         route_requirements
       end
-  
+      
+      # Assign default options, such as 'index' as a default for :action. This
+      # method must be run *after* user supplied requirements and defaults have
+      # been applied to the segments.
+      def assign_default_route_options(segments)
+        segments.each do |segment|
+          next unless segment.is_a? DynamicSegment
+          case segment.key
+            when :action
+              if segment.regexp.nil? || segment.regexp.match('index').to_s == 'index'
+                segment.default ||= 'index'
+                segment.is_optional = true
+              end
+            when :id
+              if segment.default.nil? && segment.regexp.nil? || segment.regexp =~ ''
+                segment.is_optional = true
+              end
+          end
+        end
+      end
+      
       # Makes sure that there are no optional segments that precede a required
       # segment. If any are found that precede a required segment, they are
       # made required.
@@ -909,7 +928,7 @@ module ActionController
           end
         end
       end
-
+      
       # Construct and return a route with the given path and options.
       def build(path, options)
         # Wrap the path with slashes
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index 3ce011c178..ee2ef63377 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -311,7 +311,19 @@ class LegacyRouteSetTests < Test::Unit::TestCase
     #assert_equal({'controller' => "admin/news_feed", 'action' => 'index'}, rs.recognize_path("Admin/NewsFeed"))
     #assert_equal({'controller' => "admin/news_feed", 'action' => 'index'}, rs.recognize_path("Admin/News_Feed"))
   end
+  
+  def test_requirement_should_prevent_optional_id
+    rs.draw do |map|
+      map.post 'post/:id', :controller=> 'post', :action=> 'show', :requirements => {:id => /\d+/}
+    end
 
+    assert_equal '/post/10', rs.generate(:controller => 'post', :action => 'show', :id => 10)
+    
+    assert_raises ActionController::RoutingError do
+      rs.generate(:controller => 'post', :action => 'show')
+    end
+  end
+  
   def test_both_requirement_and_optional
     rs.draw do |map|
       map.blog('test/:year', :controller => 'post', :action => 'show',
@@ -992,7 +1004,6 @@ class RouteBuilderTest < Test::Unit::TestCase
   def test_segment_for_action
     s, r = builder.segment_for(':action/something/else')
     assert_equal '/something/else', r
-    assert_equal 'index', s.default
     assert_equal :action, s.key
   end
   
@@ -1083,6 +1094,55 @@ class RouteBuilderTest < Test::Unit::TestCase
     assert_kind_of ROUTING::DynamicSegment, segments.last
   end
   
+  def test_assignment_of_default_options
+    segments = builder.segments_for_route_path '/:controller/:action/:id/'
+    action, id = segments[-4], segments[-2]
+    
+    assert_equal :action, action.key
+    assert_equal :id, id.key
+    assert ! action.optional?
+    assert ! id.optional?
+    
+    builder.assign_default_route_options(segments)
+    
+    assert_equal 'index', action.default
+    assert action.optional?
+    assert id.optional?
+  end
+  
+  def test_assignment_of_default_options_respects_existing_defaults
+    segments = builder.segments_for_route_path '/:controller/:action/:id/'
+    action, id = segments[-4], segments[-2]
+    
+    assert_equal :action, action.key
+    assert_equal :id, id.key
+    action.default = 'show'
+    action.is_optional = true
+    
+    id.default = 'Welcome'
+    id.is_optional = true
+    
+    builder.assign_default_route_options(segments)
+    
+    assert_equal 'show', action.default
+    assert action.optional?
+    assert_equal 'Welcome', id.default
+    assert id.optional?
+  end
+  
+  def test_assignment_of_default_options_respects_regexps
+    segments = builder.segments_for_route_path '/:controller/:action/:id/'
+    action = segments[-4]
+    
+    assert_equal :action, action.key
+    action.regexp = /show|in/ # Use 'in' to check partial matches
+    
+    builder.assign_default_route_options(segments)
+    
+    assert_equal nil, action.default
+    assert ! action.optional?
+  end
+  
   def test_assignment_of_is_optional_when_default
     segments = builder.segments_for_route_path '/books/:action.rss'
     assert_equal segments[3].key, :action