Skip to content

Commit

Permalink
Raise an ArgumentError when a clashing named route is defined
Browse files Browse the repository at this point in the history
  • Loading branch information
trevorturk committed Mar 20, 2013
1 parent e61cdf0 commit a2b7c0e
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 35 deletions.
4 changes: 4 additions & 0 deletions actionpack/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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" }
Expand Down
9 changes: 8 additions & 1 deletion actionpack/lib/action_dispatch/routing/route_set.rb
Original file line number Diff line number Diff line change
Expand Up @@ -403,11 +403,18 @@ def empty?
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

Expand Down
37 changes: 18 additions & 19 deletions actionpack/test/dispatch/routing_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2577,22 +2577,6 @@ def test_nested_resource_constraints
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 }
Expand All @@ -2604,9 +2588,6 @@ def test_explicitly_avoiding_the_named_route
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

Expand Down Expand Up @@ -2657,6 +2638,24 @@ def test_invalid_route_name_raises_error
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
Expand Down
18 changes: 3 additions & 15 deletions guides/source/upgrading_ruby_on_rails.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit a2b7c0e

Please sign in to comment.