Merge pull request #9704 from trevorturk/warn-about-skipped-routes

Raise an ArgumentError when a clashing named route is defined

3 files changed, 31 insertions, 20 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 0c6973f9b6..8eac446603 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,5 +1,9 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Raise an `ArgumentError` when a clashing named route is defined.
+
+    *Trevor Turk*
+
 *   Allow default url options to accept host with protocol such as `http://`
 
         config.action_mailer.default_url_options = { host: "http://mydomain.com" }
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 619dd22ec1..7fb4719fa0 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -403,11 +403,19 @@ module ActionDispatch
       def add_route(app, conditions = {}, requirements = {}, defaults = {}, name = nil, anchor = true)
         raise ArgumentError, "Invalid route name: '#{name}'" unless name.blank? || name.to_s.match(/^[_a-z]\w*$/i)
 
+        if name && named_routes[name]
+          raise ArgumentError, "Invalid route name, already in use: '#{name}' \n" \
+            "You may have defined two routes with the same name using the `:as` option, or "
+            "you may be overriding a route already defined by a resource with the same naming. " \
+            "For the latter, you can restrict the routes created with `resources` as explained here: \n" \
+            "http://guides.rubyonrails.org/routing.html#restricting-the-routes-created"
+        end
+
         path = build_path(conditions.delete(:path_info), requirements, SEPARATORS, anchor)
         conditions = build_conditions(conditions, path.names.map { |x| x.to_sym })
 
         route = @set.add_route(app, path, conditions, defaults, name)
-        named_routes[name] = route if name && !named_routes[name]
+        named_routes[name] = route if name
         route
       end
 
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 2bf7056ff7..df359ba77d 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -2577,22 +2577,6 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     assert_raises(ActionController::UrlGenerationError){ list_todo_path(:list_id => '2', :id => '1') }
   end
 
-  def test_named_routes_collision_is_avoided_unless_explicitly_given_as
-    draw do
-      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
-    end
-
-    assert_equal "/c/1", routes_collision_path(1)
-    assert_equal "/fc/1", routes_forced_collision_path(1)
-  end
-
   def test_redirect_argument_error
     routes = Class.new { include ActionDispatch::Routing::Redirection }.new
     assert_raises(ArgumentError) { routes.redirect Object.new }
@@ -2604,9 +2588,6 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
         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
     end
 
@@ -2657,6 +2638,24 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     end
   end
 
+  def test_duplicate_route_name_raises_error
+    assert_raise(ArgumentError) do
+      draw do
+        get '/collision', :to => 'collision#show', :as => 'collision'
+        get '/duplicate', :to => 'duplicate#show', :as => 'collision'
+      end
+    end
+  end
+
+  def test_duplicate_route_name_via_resources_raises_error
+    assert_raise(ArgumentError) do
+      draw do
+        resources :collisions
+        get '/collision', :to => 'collision#show', :as => 'collision'
+      end
+    end
+  end
+
   def test_nested_route_in_nested_resource
     draw do
       resources :posts, :only => [:index, :show] do