Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Add Missing Keys from Journey on failed URL format

Many named routes have keys that are required to successfully resolve. If a key is left off like this:

    <%= link_to 'user', user_path %>

This will produce an error like this:

    No route matches {:action=>"show", :controller=>"users"}

Since we know that the :id is missing, we can add extra debugging information to the error message.

    No route matches {:action=>"show", :controller=>"users"} missing required keys: [:id]


This will help new and seasoned developers look closer at their parameters. I've also subclassed the routing error to be clear that this error is a result of attempting to generate a url and not because the user is trying to visit a bad url. 

While this may sound trivial this error message is misleading and confuses most developers. The important part isn't what's in the options its's what's missing. Adding this information to the error message will make debugging much more obvious. 

This is the sister pull request of rails/journey#44 which will be required to get they missing keys into the correct error message. 

Example Development Error in Rails: http://cl.ly/image/3S0T0n1T3421
  • Loading branch information...
commit 0b6175ac2df96ebfca1baac89c20deaa13e61142 1 parent db8ff15
@schneems schneems authored
View
5 actionpack/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* When building a URL fails, add missing keys provided by Journey. Failed URL
+ generation now returns a 500 status instead of a 404.
+
+ *Richard Schneeman*
+
* Deprecate availbility of ActionView::RecordIdentifier in controllers by default.
It's view specific and can be easily included in controller manually if someone
really needs it. RecordIdentifier will be removed from ActionController::Base
View
3  actionpack/lib/action_controller/metal/exceptions.rb
@@ -16,6 +16,9 @@ def initialize(message, failures=[])
end
end
+ class ActionController::UrlGenerationError < RoutingError #:nodoc:
+ end
+
class MethodNotAllowed < ActionControllerError #:nodoc:
def initialize(*allowed_methods)
super("Only #{allowed_methods.to_sentence(:locale => :en)} requests are allowed.")
View
8 actionpack/lib/action_dispatch/routing/route_set.rb
@@ -534,12 +534,12 @@ def generate
return [path, params.keys] if @extras
[path, params]
- rescue Journey::Router::RoutingError
- raise_routing_error
+ rescue Journey::Router::RoutingError => e
+ raise_routing_error(e.message)
end
- def raise_routing_error
- raise ActionController::RoutingError, "No route matches #{options.inspect}"
+ def raise_routing_error(message)
+ raise ActionController::UrlGenerationError, "No route matches #{options.inspect} #{message}"
end
def different_controller?
View
12 actionpack/test/controller/routing_test.rb
@@ -275,7 +275,7 @@ def test_specific_controller_action_failure
mount lambda {} => "/foo"
end
- assert_raises(ActionController::RoutingError) do
+ assert_raises(ActionController::UrlGenerationError) do
url_for(rs, :controller => "omg", :action => "lol")
end
end
@@ -514,7 +514,7 @@ def test_should_list_options_diff_when_routing_constraints_dont_match
rs.draw do
get 'post/:id' => 'post#show', :constraints => { :id => /\d+/ }, :as => 'post'
end
- assert_raise(ActionController::RoutingError) do
+ assert_raise(ActionController::UrlGenerationError) do
url_for(rs, { :controller => 'post', :action => 'show', :bad_param => "foo", :use_route => "post" })
end
end
@@ -594,7 +594,7 @@ def test_requirement_should_prevent_optional_id
assert_equal '/post/10', url_for(rs, { :controller => 'post', :action => 'show', :id => 10 })
- assert_raise ActionController::RoutingError do
+ assert_raise(ActionController::UrlGenerationError) do
url_for(rs, { :controller => 'post', :action => 'show' })
end
end
@@ -760,7 +760,7 @@ def test_failed_constraints_raises_exception_with_violated_constraints
get 'foos/:id' => 'foos#show', :as => 'foo_with_requirement', :constraints => { :id => /\d+/ }
end
- assert_raise(ActionController::RoutingError) do
+ assert_raise(ActionController::UrlGenerationError) do
setup_for_named_route.send(:foo_with_requirement_url, "I am Against the constraints")
end
end
@@ -1051,7 +1051,7 @@ def test_route_error_with_missing_controller
set.draw do
get "/people" => "missing#index"
end
-
+
assert_raise(ActionController::RoutingError) {
set.recognize_path("/people", :method => :get)
}
@@ -1459,7 +1459,7 @@ def test_route_requirement_generate_with_ignore_case
url = url_for(set, { :controller => 'pages', :action => 'show', :name => 'david' })
assert_equal "/page/david", url
- assert_raise ActionController::RoutingError do
+ assert_raise(ActionController::UrlGenerationError) do
url_for(set, { :controller => 'pages', :action => 'show', :name => 'davidjamis' })
end
url = url_for(set, { :controller => 'pages', :action => 'show', :name => 'JAMIS' })
View
11 actionpack/test/dispatch/debug_exceptions_test.rb
@@ -37,6 +37,8 @@ def call(env)
raise ActionView::Template::Error.new('template', AbstractController::ActionNotFound.new)
when "/bad_request"
raise ActionController::BadRequest
+ when "/missing_keys"
+ raise ActionController::UrlGenerationError, "No route matches"
else
raise "puke!"
end
@@ -113,6 +115,15 @@ def call(env)
assert_match(/AbstractController::ActionNotFound/, body)
end
+ test "named urls missing keys raise 500 level error" do
+ @app = DevelopmentApp
+
+ get "/missing_keys", {}, {'action_dispatch.show_exceptions' => true}
+ assert_response 500
+
+ assert_match(/ActionController::UrlGenerationError/, body)
+ end
+
test "show the controller name in the diagnostics template when controller name is present" do
@app = DevelopmentApp
get("/runtime_error", {}, {
View
12 actionpack/test/dispatch/routing_test.rb
@@ -1799,7 +1799,7 @@ def test_constraints_are_merged_from_scope
get '/movies/00001'
assert_equal 'Not Found', @response.body
- assert_raises(ActionController::RoutingError){ movie_path(:id => '00001') }
+ assert_raises(ActionController::UrlGenerationError){ movie_path(:id => '00001') }
get '/movies/0001/reviews'
assert_equal 'reviews#index', @response.body
@@ -1807,7 +1807,7 @@ def test_constraints_are_merged_from_scope
get '/movies/00001/reviews'
assert_equal 'Not Found', @response.body
- assert_raises(ActionController::RoutingError){ movie_reviews_path(:movie_id => '00001') }
+ assert_raises(ActionController::UrlGenerationError){ movie_reviews_path(:movie_id => '00001') }
get '/movies/0001/reviews/0001'
assert_equal 'reviews#show', @response.body
@@ -1815,7 +1815,7 @@ def test_constraints_are_merged_from_scope
get '/movies/00001/reviews/0001'
assert_equal 'Not Found', @response.body
- assert_raises(ActionController::RoutingError){ movie_path(:movie_id => '00001', :id => '00001') }
+ assert_raises(ActionController::UrlGenerationError){ movie_path(:movie_id => '00001', :id => '00001') }
get '/movies/0001/trailer'
assert_equal 'trailers#show', @response.body
@@ -1823,7 +1823,7 @@ def test_constraints_are_merged_from_scope
get '/movies/00001/trailer'
assert_equal 'Not Found', @response.body
- assert_raises(ActionController::RoutingError){ movie_trailer_path(:movie_id => '00001') }
+ assert_raises(ActionController::UrlGenerationError){ movie_trailer_path(:movie_id => '00001') }
end
def test_only_should_be_read_from_scope
@@ -2142,7 +2142,7 @@ def test_nested_resource_constraints
get '/lists/2/todos/1'
assert_equal 'Not Found', @response.body
- assert_raises(ActionController::RoutingError){ list_todo_path(:list_id => '2', :id => '1') }
+ assert_raises(ActionController::UrlGenerationError){ list_todo_path(:list_id => '2', :id => '1') }
end
def test_named_routes_collision_is_avoided_unless_explicitly_given_as
@@ -2625,7 +2625,7 @@ def app; Routes end
get "/categories/1"
assert_response :success
- assert_raises(ActionController::RoutingError) { product_path(nil) }
+ assert_raises(ActionController::UrlGenerationError) { product_path(nil) }
end
end
Please sign in to comment.
Something went wrong with that request. Please try again.