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 https://github.com/rails/journey/pull/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

6 files changed, 35 insertions, 16 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 2752c95520..4fcce72680 100644
--- a/actionpack/CHANGELOG.md
+++ b/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
diff --git a/actionpack/lib/action_controller/metal/exceptions.rb b/actionpack/lib/action_controller/metal/exceptions.rb
index 8fd8f4797c..3c9d0c86a7 100644
--- a/actionpack/lib/action_controller/metal/exceptions.rb
+++ b/actionpack/lib/action_controller/metal/exceptions.rb
@@ -16,6 +16,9 @@ module ActionController
     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.")
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 32d267d1d6..bb1323c74e 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -534,12 +534,12 @@ module ActionDispatch
           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?
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index 57ab325683..f0430e516f 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -275,7 +275,7 @@ class LegacyRouteSetTests < ActiveSupport::TestCase
       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 @@ class LegacyRouteSetTests < ActiveSupport::TestCase
     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 @@ class LegacyRouteSetTests < ActiveSupport::TestCase
 
     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 @@ class LegacyRouteSetTests < ActiveSupport::TestCase
       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 @@ class RouteSetTest < ActiveSupport::TestCase
     set.draw do
       get    "/people" => "missing#index"
     end
-    
+
     assert_raise(ActionController::RoutingError) {
       set.recognize_path("/people", :method => :get)
     }
@@ -1459,7 +1459,7 @@ class RouteSetTest < ActiveSupport::TestCase
 
     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' })
diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb
index 6ff651ad52..d236b14e02 100644
--- a/actionpack/test/dispatch/debug_exceptions_test.rb
+++ b/actionpack/test/dispatch/debug_exceptions_test.rb
@@ -37,6 +37,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
         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 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
     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", {}, {
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index b029131ad8..856248e2ac 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -1799,7 +1799,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
 
     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 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
 
     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 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
 
     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 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
 
     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 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
 
     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 @@ class TestNamedRouteUrlHelpers < ActionDispatch::IntegrationTest
 
     get "/categories/1"
     assert_response :success
-    assert_raises(ActionController::RoutingError) { product_path(nil) }
+    assert_raises(ActionController::UrlGenerationError) { product_path(nil) }
   end
 end