Split and improve show and debug exceptions middlewares.

4 files changed, 145 insertions, 89 deletions

diff --git a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb
index 346770acb7..c801bcf2ef 100644
--- a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb
+++ b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb
@@ -3,8 +3,8 @@ require 'action_dispatch/middleware/exception_wrapper'
 require 'active_support/deprecation'
 
 module ActionDispatch
-  # This middleware rescues any exception returned by the application and renders
-  # nice exception pages if it's being rescued locally.
+  # This middleware rescues any exception returned by the application
+  # and wraps them in a format for the end user.
   class ShowExceptions
     RESCUES_TEMPLATE_PATH = File.join(File.dirname(__FILE__), 'templates')
 
@@ -40,11 +40,18 @@ module ActionDispatch
         raise exception if env['action_dispatch.show_exceptions'] == false
       end
 
-      response ? response : render_exception(env, exception)
+      response ? response : render_exception_with_failsafe(env, exception)
     end
 
     private
 
+    def render_exception_with_failsafe(env, exception)
+      render_exception(env, exception)
+    rescue Exception => failsafe_error
+      $stderr.puts "Error during failsafe response: #{failsafe_error}\n  #{failsafe_error.backtrace * "\n  "}"
+      FAILSAFE_RESPONSE
+    end
+
     def render_exception(env, exception)
       wrapper = ExceptionWrapper.new(env, exception)
 
@@ -59,13 +66,6 @@ module ActionDispatch
       else
         render(status, '')
       end
-    rescue Exception => failsafe_error
-      render_failsafe(failsafe_error)
-    end
-
-    def render_failsafe(failsafe_error)
-      $stderr.puts "Error during failsafe response: #{failsafe_error}\n  #{failsafe_error.backtrace * "\n  "}"
-      FAILSAFE_RESPONSE
     end
 
     def render(status, body)
@@ -73,7 +73,7 @@ module ActionDispatch
     end
 
     # TODO: Make this a middleware initialization parameter once
-    # we deprecated the second option
+    # we removed the second option (which is deprecated)
     def public_path
       defined?(Rails.public_path) ? Rails.public_path : 'public_path'
     end
diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb
new file mode 100644
index 0000000000..e6c0a06878
--- /dev/null
+++ b/actionpack/test/dispatch/debug_exceptions_test.rb
@@ -0,0 +1,116 @@
+require 'abstract_unit'
+
+class DebugExceptionsTest < ActionDispatch::IntegrationTest
+
+  class Boomer
+    def initialize(detailed  = false)
+      @detailed = detailed
+    end
+
+    def call(env)
+      env['action_dispatch.show_detailed_exceptions'] = @detailed
+      req = ActionDispatch::Request.new(env)
+      case req.path
+      when "/not_found"
+        raise ActionController::UnknownAction
+      when "/runtime_error"
+        raise RuntimeError
+      when "/method_not_allowed"
+        raise ActionController::MethodNotAllowed
+      when "/not_implemented"
+        raise ActionController::NotImplemented
+      when "/unprocessable_entity"
+        raise ActionController::InvalidAuthenticityToken
+      when "/not_found_original_exception"
+        raise ActionView::Template::Error.new('template', {}, AbstractController::ActionNotFound.new)
+      else
+        raise "puke!"
+      end
+    end
+  end
+
+  ProductionApp  = ActionDispatch::DebugExceptions.new((Boomer.new(false)))
+  DevelopmentApp = ActionDispatch::DebugExceptions.new((Boomer.new(true)))
+
+  test 'skip diagnosis if not showing detailed exceptions' do
+    @app = ProductionApp
+    assert_raise RuntimeError do
+      get "/", {}, {'action_dispatch.show_exceptions' => true}
+    end
+  end
+
+  test 'skip diagnosis if not showing exceptions' do
+    @app = DevelopmentApp
+    assert_raise RuntimeError do
+      get "/", {}, {'action_dispatch.show_exceptions' => false}
+    end
+  end
+
+  test "rescue with diagnostics message" do
+    @app = DevelopmentApp
+
+    get "/", {}, {'action_dispatch.show_exceptions' => true}
+    assert_response 500
+    assert_match(/puke/, body)
+
+    get "/not_found", {}, {'action_dispatch.show_exceptions' => true}
+    assert_response 404
+    assert_match(/#{ActionController::UnknownAction.name}/, body)
+
+    get "/method_not_allowed", {}, {'action_dispatch.show_exceptions' => true}
+    assert_response 405
+    assert_match(/ActionController::MethodNotAllowed/, body)
+  end
+
+  test "does not show filtered parameters" do
+    @app = DevelopmentApp
+
+    get "/", {"foo"=>"bar"}, {'action_dispatch.show_exceptions' => true,
+      'action_dispatch.parameter_filter' => [:foo]}
+    assert_response 500
+    assert_match("&quot;foo&quot;=&gt;&quot;[FILTERED]&quot;", body)
+  end
+
+  test "show registered original exception for wrapped exceptions" do
+    @app = DevelopmentApp
+
+    get "/not_found_original_exception", {}, {'action_dispatch.show_exceptions' => true}
+    assert_response 404
+    assert_match(/AbstractController::ActionNotFound/, body)
+  end
+
+  test "show the controller name in the diagnostics template when controller name is present" do
+    @app = DevelopmentApp
+    get("/runtime_error", {}, {
+      'action_dispatch.show_exceptions' => true,
+      'action_dispatch.request.parameters' => {
+        'action' => 'show',
+        'id' => 'unknown',
+        'controller' => 'featured_tile'
+      }
+    })
+    assert_response 500
+    assert_match(/RuntimeError\n    in FeaturedTileController/, body)
+  end
+
+  test "sets the HTTP charset parameter" do
+    @app = DevelopmentApp
+
+    get "/", {}, {'action_dispatch.show_exceptions' => true}
+    assert_equal "text/html; charset=utf-8", response.headers["Content-Type"]
+  end
+
+  test 'uses logger from env' do
+    @app = DevelopmentApp
+    output = StringIO.new
+    get "/", {}, {'action_dispatch.show_exceptions' => true, 'action_dispatch.logger' => Logger.new(output)}
+    assert_match(/puke/, output.rewind && output.read)
+  end
+
+  test 'uses backtrace cleaner from env' do
+    @app = DevelopmentApp
+    cleaner = stub(:clean => ['passed backtrace cleaner'])
+    get "/", {}, {'action_dispatch.show_exceptions' => true, 'action_dispatch.backtrace_cleaner' => cleaner}
+    assert_match(/passed backtrace cleaner/, body)
+  end
+end
diff --git a/actionpack/test/dispatch/show_exceptions_test.rb b/actionpack/test/dispatch/show_exceptions_test.rb
index 4f1140b5d3..a34f6f1888 100644
--- a/actionpack/test/dispatch/show_exceptions_test.rb
+++ b/actionpack/test/dispatch/show_exceptions_test.rb
@@ -13,14 +13,8 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
       case req.path
       when "/not_found"
         raise ActionController::UnknownAction
-      when "/runtime_error"
-        raise RuntimeError
       when "/method_not_allowed"
         raise ActionController::MethodNotAllowed
-      when "/not_implemented"
-        raise ActionController::NotImplemented
-      when "/unprocessable_entity"
-        raise ActionController::InvalidAuthenticityToken
       when "/not_found_original_exception"
         raise ActionView::Template::Error.new('template', {}, AbstractController::ActionNotFound.new)
       else
@@ -29,10 +23,16 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
     end
   end
 
-  ProductionApp  = ActionDispatch::ShowExceptions.new(ActionDispatch::DebugExceptions.new(Boomer.new(false)))
-  DevelopmentApp = ActionDispatch::ShowExceptions.new(ActionDispatch::DebugExceptions.new(Boomer.new(true)))
+  ProductionApp = ActionDispatch::ShowExceptions.new((Boomer.new(false)))
 
-  test "rescue with error page when show_exceptions is false" do
+  test 'skip diagnosis if not showing exceptions' do
+    @app = ProductionApp
+    assert_raise RuntimeError do
+      get "/", {}, {'action_dispatch.show_exceptions' => false}
+    end
+  end
+
+  test "rescue with error page" do
     @app = ProductionApp
 
     get "/", {}, {'action_dispatch.show_exceptions' => true}
@@ -48,24 +48,7 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
     assert_equal "", body
   end
 
-  test "rescue with diagnostics message when show_exceptions is true" do
-    @app = DevelopmentApp
-
-    get "/", {}, {'action_dispatch.show_exceptions' => true}
-    assert_response 500
-    assert_match(/puke/, body)
-
-    get "/not_found", {}, {'action_dispatch.show_exceptions' => true}
-    assert_response 404
-    assert_match(/#{ActionController::UnknownAction.name}/, body)
-
-    get "/method_not_allowed", {}, {'action_dispatch.show_exceptions' => true}
-    assert_response 405
-    assert_match(/ActionController::MethodNotAllowed/, body)
-  end
-
   test "localize rescue error page" do
-    # Change locale
     old_locale, I18n.locale = I18n.locale, :da
 
     begin
@@ -83,63 +66,18 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
     end
   end
 
-  test "does not show filtered parameters" do
-    @app = DevelopmentApp
-
-    get "/", {"foo"=>"bar"}, {'action_dispatch.show_exceptions' => true,
-      'action_dispatch.parameter_filter' => [:foo]}
-    assert_response 500
-    assert_match("&quot;foo&quot;=&gt;&quot;[FILTERED]&quot;", body)
-  end
-
-  test "show registered original exception for wrapped exceptions when show_exceptions is false" do
-    @app = ProductionApp
-
-    get "/not_found_original_exception", {}, {'action_dispatch.show_exceptions' => true}
-    assert_response 404
-    assert_match(/404 error/, body)
-  end
-
-  test "show registered original exception for wrapped exceptions when show_exceptions is true" do
-    @app = DevelopmentApp
-
-    get "/not_found_original_exception", {}, {'action_dispatch.show_exceptions' => true}
-    assert_response 404
-    assert_match(/AbstractController::ActionNotFound/, body)
-  end
-
-  test "show the controller name in the diagnostics template when controller name is present" do
-    @app = DevelopmentApp
-    get("/runtime_error", {}, {
-      'action_dispatch.show_exceptions' => true,
-      'action_dispatch.request.parameters' => {
-        'action' => 'show',
-        'id' => 'unknown',
-        'controller' => 'featured_tile'
-      }
-    })
-    assert_response 500
-    assert_match(/RuntimeError\n    in FeaturedTileController/, body)
-  end
-
   test "sets the HTTP charset parameter" do
-    @app = DevelopmentApp
+    @app = ProductionApp
 
     get "/", {}, {'action_dispatch.show_exceptions' => true}
     assert_equal "text/html; charset=utf-8", response.headers["Content-Type"]
   end
 
-  test 'uses logger from env' do
+  test "show registered original exception for wrapped exceptions" do
     @app = ProductionApp
-    output = StringIO.new
-    get "/", {}, {'action_dispatch.show_exceptions' => true, 'action_dispatch.logger' => Logger.new(output)}
-    assert_match(/puke/, output.rewind && output.read)
-  end
 
-  test 'uses backtrace cleaner from env' do
-    @app = DevelopmentApp
-    cleaner = stub(:clean => ['passed backtrace cleaner'])
-    get "/", {}, {'action_dispatch.show_exceptions' => true, 'action_dispatch.backtrace_cleaner' => cleaner}
-    assert_match(/passed backtrace cleaner/, body)
+    get "/not_found_original_exception", {}, {'action_dispatch.show_exceptions' => true}
+    assert_response 404
+    assert_match(/404 error/, body)
   end
 end
diff --git a/railties/CHANGELOG.md b/railties/CHANGELOG.md
index 6b0be4c096..d158fdd46f 100644
--- a/railties/CHANGELOG.md
+++ b/railties/CHANGELOG.md
@@ -1,13 +1,15 @@
 ## Rails 3.2.0 (unreleased) ##
 
-*   Display mounted engine's routes in `rake routes`. *Piotr Sarnacki*
+*   Add DebugExceptions middleware which contains features extracted from ShowExceptions middleware *José Valim*
+
+*   Display mounted engine's routes in `rake routes` *Piotr Sarnacki*
 
 *   Allow to change the loading order of railties with `config.railties_order=` *Piotr Sarnacki*
 
     Example:
         config.railties_order = [Blog::Engine, :main_app, :all]
 
-*   Scaffold returns 204 No Content for API requests without content. This makes scaffold work with jQuery out of the box. *José Valim*
+*   Scaffold returns 204 No Content for API requests without content. This makes scaffold work with jQuery out of the box *José Valim*
 
 *   Update Rails::Rack::Logger middleware to apply any tags set in config.log_tags to the newly ActiveSupport::TaggedLogging Rails.logger. This makes it easy to tag log lines with debug information like subdomain and request id -- both very helpful in debugging multi-user production applications *DHH*