Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add Missing Keys from Journey on Failed URL Format #7230

Merged
merged 2 commits into from

6 participants

Richard Schneeman Rafael Mendonça França Steve Klabnik Andrew White Carlos Antonio da Silva Jon Leighton
Richard Schneeman
Collaborator

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

<%= link_to 'user', user_path %>

An error will be raised like this:

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

Since Journey know's 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.

The current error message is misleading and confuses most developers. The important part isn't what's in the options, the important part is that we are missing keys. 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. Opening both issues at the same time to start a dialog.

Example Development Error in Rails: http://cl.ly/image/3S0T0n1T3421

cc/ @pixeltrix

Rafael Mendonça França
Owner

Cool. :+1:

Steve Klabnik
Collaborator

I like it! :+1:

Andrew White
Owner

@schneems can you add a test to make sure that raising an ActionController::UrlGenerationError returns a 500 response code and not a 404 - thanks!

Richard Schneeman
Collaborator

Added a test, let me know if there is a better place, or if you want more: https://github.com/rails/rails/pull/7230/files#L3R118

Carlos Antonio da Silva

Looks nice! Thanks @schneems.

Rafael Mendonça França

Could you rebase this one, add a CHANGELOG and update all the related docs? I'm going to merge it since rails/journey#44 was accepted.

actionpack/lib/action_dispatch/routing/route_set.rb
((8 lines not shown))
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}"
Rafael Mendonça França Owner

This is something that bother me, but if we don't have message we still have a trailing space after the options.

Andrew White Owner

Is raise_routing_error ever called without a message? Does the message argument need to be optional?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Richard Schneeman
Collaborator

@pixeltrix @rafaelfranca I also refactored some of the code so we can get rid of the error method. This gets rid of the whitespace problem and cleans up the interface to the generator class :) If you're not fans, I can revert. schneems@5e55f91 ATP actionpack & railties.

I checked the guides and greped around, didn't see any docs containing this error message. Added a changelog entry.

Rafael Mendonça França

Seems good. Now it needs a rebase.

schneems added some commits
Richard Schneeman schneems 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
0b6175a
Richard Schneeman schneems refactor route_set `generate_extras` functionality
The result of Generator with or without the @extras instance variable set contains the desired information. Rather than preserving state when initializing the original object, we can simply extract the keys from the resultant parameters.

ATP Actionpack, railties
cc57b2a
Richard Schneeman
Collaborator

Code has been rebased, ready for merge. Let me know if I need to do anything else.

Andrew White pixeltrix merged commit 46d8543 into from
Andrew White
Owner

Thanks for your work on this @schneems - hopefully it should reduce the number of false reports we get about routes not matching.

Jon Leighton
Collaborator

great job :+1:

Richard Schneeman
Collaborator

Thanks for all the eyes and all the help, you all rock!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 28, 2012
  1. Richard Schneeman

    Add Missing Keys from Journey on failed URL format

    schneems authored
    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
  2. Richard Schneeman

    refactor route_set `generate_extras` functionality

    schneems authored
    The result of Generator with or without the @extras instance variable set contains the desired information. Rather than preserving state when initializing the original object, we can simply extract the keys from the resultant parameters.
    
    ATP Actionpack, railties
This page is out of date. Refresh to see the latest.
5 actionpack/CHANGELOG.md
View
@@ -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
3  actionpack/lib/action_controller/metal/exceptions.rb
View
@@ -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.")
28 actionpack/lib/action_dispatch/routing/route_set.rb
View
@@ -438,12 +438,11 @@ class Generator #:nodoc:
attr_reader :options, :recall, :set, :named_route
- def initialize(options, recall, set, extras = false)
+ def initialize(options, recall, set)
@named_route = options.delete(:use_route)
@options = options.dup
@recall = recall.dup
@set = set
- @extras = extras
normalize_options!
normalize_controller_action_id!
@@ -526,20 +525,12 @@ def handle_nil_action!
recall[:action] = options.delete(:action) if options[:action] == 'index'
end
+ # Generates a path from routes, returns [path, params]
+ # if no path is returned the formatter will raise Journey::Router::RoutingError
def generate
- path, params = @set.formatter.generate(:path_info, named_route, options, recall, PARAMETERIZE)
-
- raise_routing_error unless path
-
- return [path, params.keys] if @extras
-
- [path, params]
- rescue Journey::Router::RoutingError
- raise_routing_error
- end
-
- def raise_routing_error
- raise ActionController::RoutingError, "No route matches #{options.inspect}"
+ @set.formatter.generate(:path_info, named_route, options, recall, PARAMETERIZE)
+ rescue Journey::Router::RoutingError => e
+ raise ActionController::UrlGenerationError, "No route matches #{options.inspect} #{e.message}"
end
def different_controller?
@@ -564,11 +555,12 @@ def extra_keys(options, recall={})
end
def generate_extras(options, recall={})
- generate(options, recall, true)
+ path, params = generate(options, recall)
+ return path, params.keys
end
- def generate(options, recall = {}, extras = false)
- Generator.new(options, recall, self, extras).generate
+ def generate(options, recall = {})
+ Generator.new(options, recall, self).generate
end
RESERVED_OPTIONS = [:host, :protocol, :port, :subdomain, :domain, :tld_length,
12 actionpack/test/controller/routing_test.rb
View
@@ -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' })
11 actionpack/test/dispatch/debug_exceptions_test.rb
View
@@ -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", {}, {
12 actionpack/test/dispatch/routing_test.rb
View
@@ -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
Something went wrong with that request. Please try again.