Make rescue_from behave like rescue when dealing with subclasses. Closes #10079 [fxn]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@8081 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

2 files changed, 210 insertions, 6 deletions

diff --git a/actionpack/lib/action_controller/rescue.rb b/actionpack/lib/action_controller/rescue.rb
index fa847a8274..7f82e38f54 100644
--- a/actionpack/lib/action_controller/rescue.rb
+++ b/actionpack/lib/action_controller/rescue.rb
@@ -41,8 +41,8 @@ module ActionController #:nodoc:
       base.rescue_templates = Hash.new(DEFAULT_RESCUE_TEMPLATE)
       base.rescue_templates.update DEFAULT_RESCUE_TEMPLATES
 
-      base.class_inheritable_hash :rescue_handlers
-      base.rescue_handlers = {}
+      base.class_inheritable_array :rescue_handlers
+      base.rescue_handlers = []
 
       base.extend(ClassMethods)
       base.class_eval do
@@ -55,13 +55,27 @@ module ActionController #:nodoc:
         new.process(request, response, :rescue_action, exception)
       end
 
-      # Rescue exceptions raised in controller actions by passing at least one exception class and a :with option that contains the name of the method to be called to respond to the exception.
-      # Handler methods that take one argument will be called with the exception, so that the exception can be inspected when dealing with it.
+      # Rescue exceptions raised in controller actions.
+      #
+      # <tt>rescue_from</tt> receives a series of exception classes or class
+      # names, and a trailing :with option with the name of a method or a Proc
+      # object to be called to handle them. Alternatively a block can be given.
+      #
+      # Handlers that take one argument will be called with the exception, so
+      # that the exception can be inspected when dealing with it.
+      #
+      # Handlers are inherited. They are searched from right to left, from
+      # bottom to top, and up the hierarchy. The handler of the first class for
+      # which exception.is_a?(klass) holds true is the one invoked, if any.
       #
       # class ApplicationController < ActionController::Base
       #   rescue_from User::NotAuthorized, :with => :deny_access # self defined exception
       #   rescue_from ActiveRecord::RecordInvalid, :with => :show_errors
       #
+      #   rescue_from 'MyAppError::Base' do |exception|
+      #     render :xml => exception, :status => 500
+      #   end
+      #
       #   protected
       #     def deny_access
       #       ...
@@ -78,7 +92,17 @@ module ActionController #:nodoc:
         end
 
         klasses.each do |klass|
-          rescue_handlers[klass.name] = options[:with]
+          key = if klass.is_a?(Class) && klass <= Exception
+            klass.name
+          elsif klass.is_a?(String)
+            klass
+          else
+            raise(ArgumentError, "#{klass} is neither an Exception nor a String")
+          end
+
+          # Order is important, we put the pair at the end. When dealing with an
+          # exception we will follow the documented order going from right to left.
+          rescue_handlers << [key, options[:with]]
         end
       end
     end
@@ -192,7 +216,28 @@ module ActionController #:nodoc:
       end
 
       def handler_for_rescue(exception)
-        case handler = rescue_handlers[exception.class.name]
+        # We go from right to left because pairs are pushed onto rescue_handlers
+        # as rescue_from declarations are found.
+        _, handler = *rescue_handlers.reverse.detect do |klass_name, handler|
+          # The purpose of allowing strings in rescue_from is to support the
+          # declaration of handler associations for exception classes whose
+          # definition is yet unknown.
+          #
+          # Since this loop needs the constants it would be inconsistent to
+          # assume they should exist at this point. An early raised exception
+          # could trigger some other handler and the array could include
+          # precisely a string whose corresponding constant has not yet been
+          # seen. This is why we are tolerant to unkown constants.
+          #
+          # Note that this tolerance only matters if the exception was given as
+          # a string, otherwise a NameError will be raised by the interpreter
+          # itself when rescue_from CONSTANT is executed.
+          klass = self.class.const_get(klass_name) rescue nil
+          klass ||= klass_name.constantize rescue nil
+          exception.is_a?(klass) if klass
+        end
+
+        case handler
         when Symbol
           method(handler)
         when Proc
diff --git a/actionpack/test/controller/rescue_test.rb b/actionpack/test/controller/rescue_test.rb
index cf4dcac029..a63fd06005 100644
--- a/actionpack/test/controller/rescue_test.rb
+++ b/actionpack/test/controller/rescue_test.rb
@@ -5,35 +5,62 @@ uses_mocha 'rescue' do
 class RescueController < ActionController::Base
   class NotAuthorized < StandardError
   end
+  class NotAuthorizedToRescueAsString < StandardError
+  end
 
   class RecordInvalid < StandardError
   end
+  class RecordInvalidToRescueAsString < StandardError
+  end
 
   class NotAllowed < StandardError
   end
+  class NotAllowedToRescueAsString < StandardError
+  end
 
   class InvalidRequest < StandardError
   end
+  class InvalidRequestToRescueAsString < StandardError
+  end
 
   class BadGateway < StandardError
   end
+  class BadGatewayToRescueAsString < StandardError
+  end
 
   class ResourceUnavailable < StandardError
   end
+  class ResourceUnavailableToRescueAsString < StandardError
+  end
+
+  # We use a fully-qualified name in some strings, and a relative constant
+  # name in some other to test correct handling of both cases.
 
   rescue_from NotAuthorized, :with => :deny_access
+  rescue_from 'RescueController::NotAuthorizedToRescueAsString', :with => :deny_access
+
   rescue_from RecordInvalid, :with => :show_errors
+  rescue_from 'RescueController::RecordInvalidToRescueAsString', :with => :show_errors
 
   rescue_from NotAllowed, :with => proc { head :forbidden }
+  rescue_from 'RescueController::NotAllowedToRescueAsString', :with => proc { head :forbidden }
+
   rescue_from InvalidRequest, :with => proc { |exception| render :text => exception.message }
+  rescue_from 'InvalidRequestToRescueAsString', :with => proc { |exception| render :text => exception.message }
 
   rescue_from BadGateway do
     head :status => 502
   end
+  rescue_from 'BadGatewayToRescueAsString' do
+    head :status => 502
+  end
 
   rescue_from ResourceUnavailable do |exception|
     render :text => exception.message
   end
+  rescue_from 'ResourceUnavailableToRescueAsString' do |exception|
+    render :text => exception.message
+  end
 
   def raises
     render :text => 'already rendered'
@@ -51,26 +78,44 @@ class RescueController < ActionController::Base
   def not_authorized
     raise NotAuthorized
   end
+  def not_authorized_raise_as_string
+    raise NotAuthorizedToRescueAsString
+  end
 
   def not_allowed
     raise NotAllowed
   end
+  def not_allowed_raise_as_string
+    raise NotAllowedToRescueAsString
+  end
 
   def invalid_request
     raise InvalidRequest
   end
+  def invalid_request_raise_as_string
+    raise InvalidRequestToRescueAsString
+  end
 
   def record_invalid
     raise RecordInvalid
   end
+  def record_invalid_raise_as_string
+    raise RecordInvalidToRescueAsString
+  end
   
   def bad_gateway
     raise BadGateway
   end
+  def bad_gateway_raise_as_string
+    raise BadGatewayToRescueAsString
+  end
 
   def resource_unavailable
     raise ResourceUnavailable
   end
+  def resource_unavailable_raise_as_string
+    raise ResourceUnavailableToRescueAsString
+  end
 
   def missing_template
   end
@@ -278,32 +323,58 @@ class RescueTest < Test::Unit::TestCase
     get :not_authorized
     assert_response :forbidden
   end
+  def test_rescue_handler_string
+    get :not_authorized_raise_as_string
+    assert_response :forbidden
+  end
 
   def test_rescue_handler_with_argument
     @controller.expects(:show_errors).once.with { |e| e.is_a?(Exception) }
     get :record_invalid
   end
+  def test_rescue_handler_with_argument_as_string
+    @controller.expects(:show_errors).once.with { |e| e.is_a?(Exception) }
+    get :record_invalid_raise_as_string
+  end
 
   def test_proc_rescue_handler
     get :not_allowed
     assert_response :forbidden
   end
+  def test_proc_rescue_handler_as_string
+    get :not_allowed_raise_as_string
+    assert_response :forbidden
+  end
 
   def test_proc_rescue_handle_with_argument
     get :invalid_request
     assert_equal "RescueController::InvalidRequest", @response.body
   end
+  def test_proc_rescue_handle_with_argument_as_string
+    get :invalid_request_raise_as_string
+    assert_equal "RescueController::InvalidRequestToRescueAsString", @response.body
+  end
 
   def test_block_rescue_handler
     get :bad_gateway
     assert_response 502
   end
+  def test_block_rescue_handler_as_string
+    get :bad_gateway_raise_as_string
+    assert_response 502
+  end
 
   def test_block_rescue_handler_with_argument
     get :resource_unavailable
     assert_equal "RescueController::ResourceUnavailable", @response.body
   end
 
+  def test_block_rescue_handler_with_argument_as_string
+    get :resource_unavailable_raise_as_string
+    assert_equal "RescueController::ResourceUnavailableToRescueAsString", @response.body
+  end
+
+
   protected
     def with_all_requests_local(local = true)
       old_local, ActionController::Base.consider_all_requests_local =
@@ -339,4 +410,92 @@ class RescueTest < Test::Unit::TestCase
     end
 end
 
+class ExceptionInheritanceRescueController < ActionController::Base
+
+  class ParentException < StandardError
+  end
+
+  class ChildException < ParentException
+  end
+
+  class GrandchildException < ChildException
+  end
+
+  rescue_from ChildException,      :with => lambda { head :ok }
+  rescue_from ParentException,     :with => lambda { head :created }
+  rescue_from GrandchildException, :with => lambda { head :no_content }
+
+  def raise_parent_exception
+    raise ParentException
+  end
+
+  def raise_child_exception
+    raise ChildException
+  end
+
+  def raise_grandchild_exception
+    raise GrandchildException
+  end
+end
+
+class ExceptionInheritanceRescueTest < Test::Unit::TestCase
+
+  def setup
+    @controller = ExceptionInheritanceRescueController.new
+    @request    = ActionController::TestRequest.new
+    @response   = ActionController::TestResponse.new
+  end
+
+  def test_bottom_first
+    get :raise_grandchild_exception
+    assert_response :no_content
+  end
+
+  def test_inheritance_works
+    get :raise_child_exception
+    assert_response :created
+  end
+end
+
+class ControllerInheritanceRescueController < ExceptionInheritanceRescueController
+  class FirstExceptionInChildController < StandardError
+  end
+
+  class SecondExceptionInChildController < StandardError
+  end
+
+  rescue_from FirstExceptionInChildController, 'SecondExceptionInChildController', :with => lambda { head :gone }
+
+  def raise_first_exception_in_child_controller
+    raise FirstExceptionInChildController
+  end
+
+  def raise_second_exception_in_child_controller
+    raise SecondExceptionInChildController
+  end
+end
+
+class ControllerInheritanceRescueControllerTest < Test::Unit::TestCase
+
+  def setup
+    @controller = ControllerInheritanceRescueController.new
+    @request    = ActionController::TestRequest.new
+    @response   = ActionController::TestResponse.new
+  end
+
+  def test_first_exception_in_child_controller
+    get :raise_first_exception_in_child_controller
+    assert_response :gone
+  end
+
+  def test_second_exception_in_child_controller
+    get :raise_second_exception_in_child_controller
+    assert_response :gone
+  end
+
+  def test_exception_in_parent_controller
+    get :raise_parent_exception
+    assert_response :created
+  end
+end
 end # uses_mocha