diff options
| -rw-r--r-- | actionpack/CHANGELOG | 2 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/routing.rb | 35 | ||||
| -rw-r--r-- | actionpack/test/controller/routing_test.rb | 62 |
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 |