Skip to content
Browse files

Merge pull request #7230 from schneems/schneems/expose_required_keys

Add Missing Keys from Journey on Failed URL Format
  • Loading branch information...
2 parents 3b2c42c + cc57b2a commit 46d85438fed2a44cb6e9882c532d33ca25211c57 @pixeltrix pixeltrix committed Aug 29, 2012
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
28 actionpack/lib/action_dispatch/routing/route_set.rb
@@ -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,
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,31 +1799,31 @@ 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
assert_equal '/movies/0001/reviews', movie_reviews_path(:movie_id => '0001')
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
assert_equal '/movies/0001/reviews/0001', movie_review_path(:movie_id => '0001', :id => '0001')
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
assert_equal '/movies/0001/trailer', movie_trailer_path(:movie_id => '0001')
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

0 comments on commit 46d8543

Please sign in to comment.
Something went wrong with that request. Please try again.