Merge pull request #7230 from schneems/schneems/expose_required_keys

Add Missing Keys from Journey on Failed URL Format

6 files changed, 41 insertions, 30 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index ae663deed8..e9738f6a86 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..1fe40d8458 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -438,12 +438,11 @@ module ActionDispatch
 
         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 @@ module ActionDispatch
           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 @@ module ActionDispatch
       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,
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