Skip to content
This repository
Browse code

Remove fancy method not allowed resource exceptions since they are

too much of a hack
  • Loading branch information...
commit 588225f8852c4b60bfba38f16d8797a41e175400 1 parent 2f90d70
Joshua Peek authored
11  actionpack/lib/action_controller/metal/exceptions.rb
@@ -18,18 +18,9 @@ class MethodNotAllowed < ActionControllerError #:nodoc:
18 18
 
19 19
     def initialize(*allowed_methods)
20 20
       super("Only #{allowed_methods.to_sentence(:locale => :en)} requests are allowed.")
21  
-      @allowed_methods = allowed_methods
22  
-    end
23  
-
24  
-    def allowed_methods_header
25  
-      allowed_methods.map { |method_symbol| method_symbol.to_s.upcase } * ', '
26  
-    end
27  
-
28  
-    def handle_response!(response)
29  
-      response.headers['Allow'] ||= allowed_methods_header
30 21
     end
31 22
   end
32  
-  
  23
+
33 24
   class NotImplemented < MethodNotAllowed #:nodoc:
34 25
   end
35 26
 
24  actionpack/lib/action_dispatch/routing/route_set.rb
@@ -456,30 +456,9 @@ def generate(options, recall = {}, method = :generate)
456 456
 
457 457
       def call(env)
458 458
         @set.call(env)
459  
-      rescue ActionController::RoutingError => e
460  
-        raise e if env['action_controller.rescue_error'] == false
461  
-
462  
-        method, path = env['REQUEST_METHOD'].downcase.to_sym, env['PATH_INFO']
463  
-
464  
-        # Route was not recognized. Try to find out why (maybe wrong verb).
465  
-        allows = HTTP_METHODS.select { |verb|
466  
-          begin
467  
-            recognize_path(path, {:method => verb}, false)
468  
-          rescue ActionController::RoutingError
469  
-            nil
470  
-          end
471  
-        }
472  
-
473  
-        if !HTTP_METHODS.include?(method)
474  
-          raise ActionController::NotImplemented.new(*allows)
475  
-        elsif !allows.empty?
476  
-          raise ActionController::MethodNotAllowed.new(*allows)
477  
-        else
478  
-          raise e
479  
-        end
480 459
       end
481 460
 
482  
-      def recognize_path(path, environment = {}, rescue_error = true)
  461
+      def recognize_path(path, environment = {})
483 462
         method = (environment[:method] || "GET").to_s.upcase
484 463
 
485 464
         begin
@@ -489,7 +468,6 @@ def recognize_path(path, environment = {}, rescue_error = true)
489 468
         end
490 469
 
491 470
         env['action_controller.recognize'] = true
492  
-        env['action_controller.rescue_error'] = rescue_error
493 471
         status, headers, body = call(env)
494 472
         body
495 473
       end
8  actionpack/test/controller/resources_test.rb
@@ -403,7 +403,7 @@ def test_override_new_method
403 403
     with_restful_routing :messages do
404 404
       assert_restful_routes_for :messages do |options|
405 405
         assert_recognizes(options.merge(:action => "new"), :path => "/messages/new", :method => :get)
406  
-        assert_raise(ActionController::MethodNotAllowed) do
  406
+        assert_raise(ActionController::RoutingError) do
407 407
           ActionController::Routing::Routes.recognize_path("/messages/new", :method => :post)
408 408
         end
409 409
       end
@@ -689,11 +689,11 @@ def test_should_not_allow_delete_or_put_on_collection_path
689 689
       options = { :controller => controller_name.to_s }
690 690
       collection_path = "/#{controller_name}"
691 691
 
692  
-      assert_raise(ActionController::MethodNotAllowed) do
  692
+      assert_raise(ActionController::RoutingError) do
693 693
         assert_recognizes(options.merge(:action => 'update'), :path => collection_path, :method => :put)
694 694
       end
695 695
 
696  
-      assert_raise(ActionController::MethodNotAllowed) do
  696
+      assert_raise(ActionController::RoutingError) do
697 697
         assert_recognizes(options.merge(:action => 'destroy'), :path => collection_path, :method => :delete)
698 698
       end
699 699
     end
@@ -1378,7 +1378,7 @@ def assert_whether_allowed(allowed, not_allowed, options, action, path, method)
1378 1378
     end
1379 1379
 
1380 1380
     def assert_not_recognizes(expected_options, path)
1381  
-      assert_raise ActionController::RoutingError, ActionController::MethodNotAllowed, Assertion do
  1381
+      assert_raise ActionController::RoutingError, Assertion do
1382 1382
         assert_recognizes(expected_options, path)
1383 1383
       end
1384 1384
     end
9  actionpack/test/controller/routing_test.rb
@@ -976,7 +976,7 @@ def test_recognize_with_conditions
976 976
     params = set.recognize_path("/people", :method => :put)
977 977
     assert_equal("update", params[:action])
978 978
 
979  
-    assert_raise(ActionController::NotImplemented) {
  979
+    assert_raise(ActionController::RoutingError) {
980 980
       set.recognize_path("/people", :method => :bacon)
981 981
     }
982 982
 
@@ -992,12 +992,9 @@ def test_recognize_with_conditions
992 992
     assert_equal("destroy", params[:action])
993 993
     assert_equal("5", params[:id])
994 994
 
995  
-    begin
  995
+    assert_raise(ActionController::RoutingError) {
996 996
       set.recognize_path("/people/5", :method => :post)
997  
-      flunk 'Should have raised MethodNotAllowed'
998  
-    rescue ActionController::MethodNotAllowed => e
999  
-      assert_equal [:get, :put, :delete], e.allowed_methods
1000  
-    end
  997
+    }
1001 998
   end
1002 999
 
1003 1000
   def test_recognize_with_alias_in_conditions

2 notes on commit 588225f

Andrew Briening

Isn't this a step backward from HTTP/1.1 compliance?
http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.2
"Servers SHOULD respond to invalid Request-URIs with an appropriate status code"

I get that this is a "SHOULD" and not a "MUST" but I have been using "MethodNotAllowed" exceptions in Rails 2 to send an appropriate 405 response along with the "Allow" header using "handle_response!".

While working on a Rails 3 project today, I found it surprising that a URI that is a valid route will raise RoutingError instead of MethodNotAllowed given the wrong HTTP method. I usually treat RoutingError as 404 which isn't really an appropriate HTTP/1.1 response.

Any thoughts on a workaround? Or should I look into a less "fancy/hacky" implementation?

A quick untested application based solution:
https://gist.github.com/1248839

Andrew Briening

My first stab a solution failed because the RouteSet returns a 404 with 'X-Cascade: pass', never even touches ActionController and gets handled by ActionDispatch::ShowExceptions. So I've added two catch-all routes ( thanks for the idea Brian A ) and an action for handling the 501 & 405 errors.

Current rails:
GET /users => 200 OK
OPTIONS /users => 404 Not Found ( or RoutingError in development )
PROPFIND /users => 404 Not Found ( or RoutingError in development )

Using this solution: https://gist.github.com/1255051
GET /users => 200 OK
OPTIONS /users => 405 Method Not Allowed
PROPFIND /users => 501 Not Implemented

It would be great for Rails to strive for HTTP/1.1 compliance where possible at the framework level, but it's fortunately flexible enough to allow handling at the application level.

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