aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJeremy Kemper <jeremy@bitsweat.net>2007-05-26 20:07:34 +0000
committerJeremy Kemper <jeremy@bitsweat.net>2007-05-26 20:07:34 +0000
commitdcaa074abf5691a933b9c55159cc7d98a02b3b2f (patch)
tree4d58fee286eaa957e141939dcbd898055cab2325
parent42ebf559cc6ed80fa3bf55ecc1dea7c9e593f45e (diff)
downloadrails-dcaa074abf5691a933b9c55159cc7d98a02b3b2f.tar.gz
rails-dcaa074abf5691a933b9c55159cc7d98a02b3b2f.tar.bz2
rails-dcaa074abf5691a933b9c55159cc7d98a02b3b2f.zip
Routing: respond with 405 Method Not Allowed status when the route path matches but the HTTP method does not. Closes #6953.
git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@6862 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
-rw-r--r--actionpack/CHANGELOG2
-rwxr-xr-xactionpack/lib/action_controller/base.rb18
-rw-r--r--actionpack/lib/action_controller/rescue.rb20
-rw-r--r--actionpack/lib/action_controller/routing.rb13
-rw-r--r--actionpack/test/controller/rescue_test.rb26
-rw-r--r--actionpack/test/controller/resources_test.rb6
-rw-r--r--actionpack/test/controller/routing_test.rb22
7 files changed, 91 insertions, 16 deletions
diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 0258d02b54..356c88a42a 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
*SVN*
+* Routing: respond with 405 Method Not Allowed status when the route path matches but the HTTP method does not. #6953 [Josh Peek, defeated, Dan Kubb, Coda Hale]
+
* Add support for assert_select_rjs with :show and :hide. #7780 [dchelimsky]
* Make assert_select's failure messages clearer about what failed. #7779 [dchelimsky]
diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb
index dc5a1d18cd..8bff41d38d 100755
--- a/actionpack/lib/action_controller/base.rb
+++ b/actionpack/lib/action_controller/base.rb
@@ -22,6 +22,24 @@ module ActionController #:nodoc:
@failures = failures
end
end
+ class MethodNotAllowed < ActionControllerError #:nodoc:
+ attr_reader :allowed_methods
+
+ def initialize(*allowed_methods)
+ super("Only #{allowed_methods.to_sentence} requests are allowed.")
+ @allowed_methods = allowed_methods
+ end
+
+ def allowed_methods_header
+ allowed_methods.map { |method_symbol| method_symbol.to_s.upcase } * ', '
+ end
+
+ def handle_response!(response)
+ response.headers['Allow'] ||= allowed_methods_header
+ end
+ end
+ class NotImplemented < MethodNotAllowed #:nodoc:
+ end
class UnknownController < ActionControllerError #:nodoc:
end
class UnknownAction < ActionControllerError #:nodoc:
diff --git a/actionpack/lib/action_controller/rescue.rb b/actionpack/lib/action_controller/rescue.rb
index 225acd28f3..3ab10107bf 100644
--- a/actionpack/lib/action_controller/rescue.rb
+++ b/actionpack/lib/action_controller/rescue.rb
@@ -13,12 +13,14 @@ module ActionController #:nodoc:
DEFAULT_RESCUE_RESPONSE = :internal_server_error
DEFAULT_RESCUE_RESPONSES = {
- 'ActionController::RoutingError' => :not_found,
- 'ActionController::UnknownAction' => :not_found,
- 'ActiveRecord::RecordNotFound' => :not_found,
- 'ActiveRecord::StaleObjectError' => :conflict,
- 'ActiveRecord::RecordInvalid' => :unprocessable_entity,
- 'ActiveRecord::RecordNotSaved' => :unprocessable_entity
+ 'ActionController::RoutingError' => :not_found,
+ 'ActionController::UnknownAction' => :not_found,
+ 'ActiveRecord::RecordNotFound' => :not_found,
+ 'ActiveRecord::StaleObjectError' => :conflict,
+ 'ActiveRecord::RecordInvalid' => :unprocessable_entity,
+ 'ActiveRecord::RecordNotSaved' => :unprocessable_entity,
+ 'ActionController::MethodNotAllowed' => :method_not_allowed,
+ 'ActionController::NotImplemented' => :not_implemented
}
DEFAULT_RESCUE_TEMPLATE = 'diagnostics'
@@ -56,6 +58,12 @@ module ActionController #:nodoc:
log_error(exception) if logger
erase_results if performed?
+ # Let the exception alter the response if it wants.
+ # For example, MethodNotAllowed sets the Allow header.
+ if exception.respond_to?(:handle_response!)
+ exception.handle_response!(response)
+ end
+
if consider_all_requests_local || local_request?
rescue_action_locally(exception)
else
diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index 00d5050b7e..4a1eda82bf 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -251,6 +251,8 @@ module ActionController
# TODO: , (comma) should be an allowed path character.
SEPARATORS = %w( / ; . , ? )
+ HTTP_METHODS = [:get, :head, :post, :put, :delete]
+
# The root paths which may contain controller files
mattr_accessor :controller_paths
self.controller_paths = []
@@ -1345,7 +1347,16 @@ module ActionController
routes.each do |route|
result = route.recognize(path, environment) and return result
end
- raise RoutingError, "no route found to match #{path.inspect} with #{environment.inspect}"
+
+ allows = HTTP_METHODS.select { |verb| routes.find { |r| r.recognize(path, :method => verb) } }
+
+ if environment[:method] && !HTTP_METHODS.include?(environment[:method])
+ raise NotImplemented.new(*allows)
+ elsif !allows.empty?
+ raise MethodNotAllowed.new(*allows)
+ else
+ raise RoutingError, "No route matches #{path.inspect} with #{environment.inspect}"
+ end
end
def routes_by_controller
diff --git a/actionpack/test/controller/rescue_test.rb b/actionpack/test/controller/rescue_test.rb
index 885ac0b8c0..100fee8a6e 100644
--- a/actionpack/test/controller/rescue_test.rb
+++ b/actionpack/test/controller/rescue_test.rb
@@ -7,6 +7,14 @@ class RescueController < ActionController::Base
render :text => 'already rendered'
raise "don't panic!"
end
+
+ def method_not_allowed
+ raise ActionController::MethodNotAllowed.new(:get, :head, :put)
+ end
+
+ def not_implemented
+ raise ActionController::NotImplemented.new(:get, :put)
+ end
end
@@ -144,6 +152,8 @@ class RescueTest < Test::Unit::TestCase
assert_equal :conflict, responses['ActiveRecord::StaleObjectError']
assert_equal :unprocessable_entity, responses['ActiveRecord::RecordInvalid']
assert_equal :unprocessable_entity, responses['ActiveRecord::RecordNotSaved']
+ assert_equal :method_not_allowed, responses['ActionController::MethodNotAllowed']
+ assert_equal :not_implemented, responses['ActionController::NotImplemented']
end
def test_rescue_templates
@@ -176,6 +186,22 @@ class RescueTest < Test::Unit::TestCase
assert_nil @controller.send(:clean_backtrace, Exception.new)
end
end
+
+ def test_not_implemented
+ with_all_requests_local false do
+ head :not_implemented
+ end
+ assert_response :not_implemented
+ assert_equal "GET, PUT", @response.headers['Allow']
+ end
+
+ def test_method_not_allowed
+ with_all_requests_local false do
+ get :method_not_allowed
+ end
+ assert_response :method_not_allowed
+ assert_equal "GET, HEAD, PUT", @response.headers['Allow']
+ end
protected
def with_all_requests_local(local = true)
diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb
index 60d94120af..8149c02a9d 100644
--- a/actionpack/test/controller/resources_test.rb
+++ b/actionpack/test/controller/resources_test.rb
@@ -188,7 +188,7 @@ class ResourcesTest < Test::Unit::TestCase
with_restful_routing :messages do
assert_restful_routes_for :messages do |options|
assert_recognizes(options.merge(:action => "new"), :path => "/messages/new", :method => :get)
- assert_raises(ActionController::RoutingError) do
+ assert_raises(ActionController::MethodNotAllowed) do
ActionController::Routing::Routes.recognize_path("/messages/new", :method => :post)
end
end
@@ -384,11 +384,11 @@ class ResourcesTest < Test::Unit::TestCase
options = { :controller => controller_name.to_s }
collection_path = "/#{controller_name}"
- assert_raises(ActionController::RoutingError) do
+ assert_raises(ActionController::MethodNotAllowed) do
assert_recognizes(options.merge(:action => 'update'), :path => collection_path, :method => :put)
end
- assert_raises(ActionController::RoutingError) do
+ assert_raises(ActionController::MethodNotAllowed) do
assert_recognizes(options.merge(:action => 'destroy'), :path => collection_path, :method => :delete)
end
end
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index 2abccd7402..7efb6b8a6e 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -1590,8 +1590,13 @@ class RouteSetTest < Test::Unit::TestCase
assert_nothing_raised { set.recognize(request) }
assert_equal("update", request.path_parameters[:action])
- request.method = :update
- assert_raises(ActionController::RoutingError) { set.recognize(request) }
+ begin
+ request.method = :bacon
+ set.recognize(request)
+ flunk 'Should have raised NotImplemented'
+ rescue ActionController::NotImplemented => e
+ assert_equal [:get, :post, :put, :delete], e.allowed_methods
+ end
request.path = "/people/5"
request.method = :get
@@ -1608,10 +1613,15 @@ class RouteSetTest < Test::Unit::TestCase
assert_nothing_raised { set.recognize(request) }
assert_equal("destroy", request.path_parameters[:action])
assert_equal("5", request.path_parameters[:id])
-
- request.method = :post
- assert_raises(ActionController::RoutingError) { set.recognize(request) }
-
+
+ begin
+ request.method = :post
+ set.recognize(request)
+ flunk 'Should have raised MethodNotAllowed'
+ rescue ActionController::MethodNotAllowed => e
+ assert_equal [:get, :put, :delete], e.allowed_methods
+ end
+
ensure
Object.send(:remove_const, :PeopleController)
end