Skip to content
Browse files

Ensure that named routes do not overwrite previously defined routes.

  • Loading branch information...
1 parent 9ddda10 commit 1a2b28c9d9deb7958124917195529f1b96b6bd82 @josevalim josevalim committed Sep 29, 2010
Showing with 31 additions and 10 deletions.
  1. +13 −10 actionpack/lib/action_dispatch/routing/mapper.rb
  2. +18 −0 actionpack/test/dispatch/routing_test.rb
View
23 actionpack/lib/action_dispatch/routing/mapper.rb
@@ -62,7 +62,6 @@ def normalize_options!
if using_match_shorthand?(path_without_format, @options)
to_shorthand = @options[:to].blank?
@options[:to] ||= path_without_format[1..-1].sub(%r{/([^/]*)$}, '#\1')
- @options[:as] ||= Mapper.normalize_name(path_without_format)
end
@options.merge!(default_controller_and_action(to_shorthand))
@@ -881,9 +880,14 @@ def match(*args)
if action.to_s =~ /^[\w\/]+$/
options[:action] ||= action unless action.to_s.include?("/")
- options[:as] = name_for_action(action, options[:as])
else
- options[:as] = name_for_action(options[:as])
+ action = nil
+ end
+
+ if options.key?(:as) && !options[:as]
+ options.delete(:as)
+ else
+ options[:as] = name_for_action(options[:as], action)
end
super(path, options)
@@ -1049,18 +1053,16 @@ def action_path(name, path = nil)
path || @scope[:path_names][name.to_sym] || name.to_s
end
- def prefix_name_for_action(action, as)
- if as.present?
+ def prefix_name_for_action(as, action)
+ if as
as.to_s
- elsif as
- nil
elsif !canonical_action?(action, @scope[:scope_level])
action.to_s
end
end
- def name_for_action(action, as=nil)
- prefix = prefix_name_for_action(action, as)
+ def name_for_action(as, action)
+ prefix = prefix_name_for_action(as, action)
prefix = Mapper.normalize_name(prefix) if prefix
name_prefix = @scope[:as]
@@ -1084,7 +1086,8 @@ def name_for_action(action, as=nil)
[name_prefix, member_name, prefix]
end
- name.select(&:present?).join("_").presence
+ candidate = name.select(&:present?).join("_").presence
+ candidate unless as.nil? && @set.routes.map(&:name).include?(candidate)
end
end
View
18 actionpack/test/dispatch/routing_test.rb
@@ -442,6 +442,15 @@ def self.matches?(request)
get :preview, :on => :member
end
+ scope :as => "routes" do
+ get "/c/:id", :as => :collision, :to => "collision#show"
+ get "/collision", :to => "collision#show"
+ get "/no_collision", :to => "collision#show", :as => nil
+
+ get "/fc/:id", :as => :forced_collision, :to => "forced_collision#show"
+ get "/forced_collision", :as => :forced_collision, :to => "forced_collision#show"
+ end
+
match '/purchases/:token/:filename',
:to => 'purchases#fetch',
:token => /[[:alnum:]]{10}/,
@@ -2128,6 +2137,15 @@ def test_nested_resource_constraints
assert_raises(ActionController::RoutingError){ list_todo_path(:list_id => '2', :id => '1') }
end
+ def test_named_routes_collision_is_avoided_unless_explicitly_given_as
+ assert_equal "/c/1", routes_collision_path(1)
+ assert_equal "/forced_collision", routes_forced_collision_path
+ end
+
+ def test_explicitly_avoiding_the_named_route
+ assert !respond_to?(:routes_no_collision_path)
+ end
+
def test_controller_name_with_leading_slash_raise_error
assert_raise(ArgumentError) do
self.class.stub_controllers do |routes|

0 comments on commit 1a2b28c

Please sign in to comment.
Something went wrong with that request. Please try again.