Raise an ArgumentError when a clashing named route is defined

4 files changed, 33 insertions, 35 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..38e9288572 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -403,11 +403,18 @@ 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" \
+            "This conflict might arise if your routes contain `resources :#{name.pluralize}`. " \
+            "If so, 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
diff --git a/guides/source/upgrading_ruby_on_rails.md b/guides/source/upgrading_ruby_on_rails.md
index 0941bc7e58..af223873ec 100644
--- a/guides/source/upgrading_ruby_on_rails.md
+++ b/guides/source/upgrading_ruby_on_rails.md
@@ -103,32 +103,20 @@ Rails 4.0 extracted Active Resource to its own gem. If you still need the featur
 
 * Rails 4.0 changed how `assert_generates`, `assert_recognizes`, and `assert_routing` work. Now all these assertions raise `Assertion` instead of `ActionController::RoutingError`.
 
-* Rails 4.0 correctly prefers the first named route defined in `config/routes.rb` if a clashing route is found later. Check the output of `rake routes` before upgrading and remove unused named routes to avoid issues.
+* Rails 4.0 raises an `ArgumentError` if clashing named routes are defined. This can be triggered by explicitly defined named routes or by the `resources` method. Here are two examples that clash with routes named `example_path`:
 
 ```ruby
-  # config/routes.rb
   get 'one' => 'test#example', as: :example
   get 'two' => 'test#example', as: :example
-
-  # Rails 3
-  <%= example_path %> # => '/two'
-
-  # Rails 4
-  <%= example_path %> # => '/one'
 ```
 
 ```ruby
-  # config/routes.rb
   resources :examples
   get 'clashing/:id' => 'test#example', as: :example
-
-  # Rails 3
-  <%= example_path(1) %> # => '/clashing/1'
-
-  # Rails 4
-  <%= example_path(1) %> # => '/examples/1'
 ```
 
+In the first case, you can simply avoid using the same name for multiple routes. In the second, you can use the `only` or `except` options provided by the `resources` method to restrict the routes created as detailed in the [Routing Guide](http://guides.rubyonrails.org/routing.html#restricting-the-routes-created).
+
 * Rails 4.0 also changed the way unicode character routes are drawn. Now you can draw unicode character routes directly. If you already draw such routes, you must change them, for example:
 
 ```ruby