Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fix routing to respect user provided requirements and defaults when a…

…ssigning 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
  • Loading branch information...
commit 4ae3db83663ad2f12d732da333c664a6452df674 1 parent 54c393f
@seckar seckar authored
View
2  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:
View
35 actionpack/lib/action_controller/routing.rb
@@ -820,8 +820,6 @@ def segment_for(string)
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 @@ def divide_route_options(segments, options)
# 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 @@ def assign_route_options(segments, defaults, requirements)
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 @@ def ensure_required_segments(segments)
end
end
end
-
+
# Construct and return a route with the given path and options.
def build(path, options)
# Wrap the path with slashes
View
62 actionpack/test/controller/routing_test.rb
@@ -311,7 +311,19 @@ def test_recognition_with_uppercase_controller_name
#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 @@ def test_segments_for
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 @@ def test_segmentation_of_dynamic_dot_path
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
Please sign in to comment.
Something went wrong with that request. Please try again.