From a2b7c0e69d671294067b625c749be6fdbec7b433 Mon Sep 17 00:00:00 2001 From: Trevor Turk Date: Tue, 19 Mar 2013 23:23:55 -0500 Subject: Raise an ArgumentError when a clashing named route is defined --- actionpack/CHANGELOG.md | 4 +++ .../lib/action_dispatch/routing/route_set.rb | 9 +++++- actionpack/test/dispatch/routing_test.rb | 37 +++++++++++----------- guides/source/upgrading_ruby_on_rails.md | 18 ++--------- 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 -- cgit v1.2.3 From 1a25ebf884206df34b30a0bac8345d75fbc6d13c Mon Sep 17 00:00:00 2001 From: Trevor Turk Date: Wed, 20 Mar 2013 12:48:35 -0500 Subject: Tweak exception message to avoid giving potentially misleading suggestions --- actionpack/lib/action_dispatch/routing/route_set.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 38e9288572..7fb4719fa0 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -405,8 +405,9 @@ module ActionDispatch 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" \ + "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 -- cgit v1.2.3