Split ShowExceptions responsibilities in two middlewares.

7 files changed, 123 insertions, 93 deletions

diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb
index 1f403b5e1a..89f515ee3d 100644
--- a/actionpack/lib/action_dispatch.rb
+++ b/actionpack/lib/action_dispatch.rb
@@ -51,6 +51,7 @@ module ActionDispatch
     autoload :BestStandardsSupport
     autoload :Callbacks
     autoload :Cookies
+    autoload :DebugExceptions
     autoload :ExceptionWrapper
     autoload :Flash
     autoload :Head
diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
new file mode 100644
index 0000000000..5da21ddd3d
--- /dev/null
+++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
@@ -0,0 +1,77 @@
+require 'action_dispatch/http/request'
+require 'action_dispatch/middleware/exception_wrapper'
+
+module ActionDispatch
+  # This middleware is responsible for logging exceptions and
+  # showing a debugging page in case the request is local.
+  class DebugExceptions
+    RESCUES_TEMPLATE_PATH = File.join(File.dirname(__FILE__), 'templates')
+
+    def initialize(app)
+      @app = app
+    end
+
+    def call(env)
+      begin
+        response = @app.call(env)
+
+        if response[1]['X-Cascade'] == 'pass'
+          raise ActionController::RoutingError, "No route matches [#{env['REQUEST_METHOD']}] #{env['PATH_INFO'].inspect}"
+        end
+      rescue Exception => exception
+        raise exception if env['action_dispatch.show_exceptions'] == false
+      end
+
+      response ? response : render_exception(env, exception)
+    end
+
+    private
+
+    def render_exception(env, exception)
+      wrapper = ExceptionWrapper.new(env, exception)
+      log_error(env, wrapper)
+
+      if env['action_dispatch.show_detailed_exceptions']
+        template = ActionView::Base.new([RESCUES_TEMPLATE_PATH],
+          :request => Request.new(env),
+          :exception => wrapper.exception,
+          :application_trace => wrapper.application_trace,
+          :framework_trace => wrapper.framework_trace,
+          :full_trace => wrapper.full_trace
+        )
+
+        file = "rescues/#{wrapper.rescue_template}"
+        body = template.render(:template => file, :layout => 'rescues/layout')
+        render(wrapper.status_code, body)
+      else
+        raise exception
+      end
+    end
+
+    def render(status, body)
+      [status, {'Content-Type' => "text/html; charset=#{Response.default_charset}", 'Content-Length' => body.bytesize.to_s}, [body]]
+    end
+
+    def log_error(env, wrapper)
+      logger = logger(env)
+      return unless logger
+
+      exception = wrapper.exception
+
+      ActiveSupport::Deprecation.silence do
+        message = "\n#{exception.class} (#{exception.message}):\n"
+        message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code)
+        message << "  " << wrapper.application_trace.join("\n  ")
+        logger.fatal("#{message}\n\n")
+      end
+    end
+
+    def logger(env)
+      env['action_dispatch.logger'] || stderr_logger
+    end
+
+    def stderr_logger
+      @stderr_logger ||= Logger.new($stderr)
+    end
+  end
+end
diff --git a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
index 96598b529d..c0532c80c4 100644
--- a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
+++ b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
@@ -1,3 +1,4 @@
+require 'action_controller/metal/exceptions'
 require 'active_support/core_ext/exception'
 
 module ActionDispatch
diff --git a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb
index aa7a5e4d64..346770acb7 100644
--- a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb
+++ b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb
@@ -1,4 +1,3 @@
-require 'action_controller/metal/exceptions'
 require 'action_dispatch/http/request'
 require 'action_dispatch/middleware/exception_wrapper'
 require 'active_support/deprecation'
@@ -36,100 +35,47 @@ module ActionDispatch
 
     def call(env)
       begin
-        status, headers, body = @app.call(env)
-        exception = nil
-
-        # Only this middleware cares about RoutingError. So, let's just raise
-        # it here.
-        if headers['X-Cascade'] == 'pass'
-           raise ActionController::RoutingError, "No route matches [#{env['REQUEST_METHOD']}] #{env['PATH_INFO'].inspect}"
-        end
+        response  = @app.call(env)
       rescue Exception => exception
         raise exception if env['action_dispatch.show_exceptions'] == false
       end
 
-      exception ? render_exception(env, exception) : [status, headers, body]
+      response ? response : render_exception(env, exception)
     end
 
     private
-      def render_exception(env, exception)
-        wrapper = ExceptionWrapper.new(env, exception)
-        log_error(env, wrapper)
 
-        if env['action_dispatch.show_detailed_exceptions'] == true
-          rescue_action_diagnostics(wrapper)
-        else
-          rescue_action_error_page(wrapper)
-        end
-      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)
 
-      # Render detailed diagnostics for unhandled exceptions rescued from
-      # a controller action.
-      def rescue_action_diagnostics(wrapper)
-        template = ActionView::Base.new([RESCUES_TEMPLATE_PATH],
-          :request => Request.new(wrapper.env),
-          :exception => wrapper.exception,
-          :application_trace => wrapper.application_trace,
-          :framework_trace => wrapper.framework_trace,
-          :full_trace => wrapper.full_trace
-        )
-        file = "rescues/#{wrapper.rescue_template}"
-        body = template.render(:template => file, :layout => 'rescues/layout')
-        render(wrapper.status_code, body)
-      end
+      status      = wrapper.status_code
+      locale_path = "#{public_path}/#{status}.#{I18n.locale}.html" if I18n.locale
+      path        = "#{public_path}/#{status}.html"
 
-      # Attempts to render a static error page based on the
-      # <tt>status_code</tt> thrown, or just return headers if no such file
-      # exists. At first, it will try to render a localized static page.
-      # For example, if a 500 error is being handled Rails and locale is :da,
-      # it will first attempt to render the file at <tt>public/500.da.html</tt>
-      # then attempt to render <tt>public/500.html</tt>. If none of them exist,
-      # the body of the response will be left empty.
-      def rescue_action_error_page(wrapper)
-        status = wrapper.status_code
-        locale_path = "#{public_path}/#{status}.#{I18n.locale}.html" if I18n.locale
-        path = "#{public_path}/#{status}.html"
-
-        if locale_path && File.exist?(locale_path)
-          render(status, File.read(locale_path))
-        elsif File.exist?(path)
-          render(status, File.read(path))
-        else
-          render(status, '')
-        end
+      if locale_path && File.exist?(locale_path)
+        render(status, File.read(locale_path))
+      elsif File.exist?(path)
+        render(status, File.read(path))
+      else
+        render(status, '')
       end
+    rescue Exception => failsafe_error
+      render_failsafe(failsafe_error)
+    end
 
-      def render(status, body)
-        [status, {'Content-Type' => "text/html; charset=#{Response.default_charset}", 'Content-Length' => body.bytesize.to_s}, [body]]
-      end
-
-      def public_path
-        defined?(Rails.public_path) ? Rails.public_path : 'public_path'
-      end
-
-      def log_error(env, wrapper)
-        logger = logger(env)
-        return unless logger
-
-        exception = wrapper.exception
-
-        ActiveSupport::Deprecation.silence do
-          message = "\n#{exception.class} (#{exception.message}):\n"
-          message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code)
-          message << "  " << wrapper.application_trace.join("\n  ")
-          logger.fatal("#{message}\n\n")
-        end
-      end
+    def render_failsafe(failsafe_error)
+      $stderr.puts "Error during failsafe response: #{failsafe_error}\n  #{failsafe_error.backtrace * "\n  "}"
+      FAILSAFE_RESPONSE
+    end
 
-      def logger(env)
-        env['action_dispatch.logger'] || stderr_logger
-      end
+    def render(status, body)
+      [status, {'Content-Type' => "text/html; charset=#{Response.default_charset}", 'Content-Length' => body.bytesize.to_s}, [body]]
+    end
 
-      def stderr_logger
-        @stderr_logger ||= Logger.new($stderr)
-      end
+    # TODO: Make this a middleware initialization parameter once
+    # we deprecated the second option
+    def public_path
+      defined?(Rails.public_path) ? Rails.public_path : 'public_path'
+    end
   end
 end
diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb
index d191a203dd..c95b8221a1 100644
--- a/actionpack/test/abstract_unit.rb
+++ b/actionpack/test/abstract_unit.rb
@@ -175,6 +175,7 @@ class ActionDispatch::IntegrationTest < ActiveSupport::TestCase
   def self.build_app(routes = nil)
     RoutedRackApp.new(routes || ActionDispatch::Routing::RouteSet.new) do |middleware|
       middleware.use "ActionDispatch::ShowExceptions"
+      middleware.use "ActionDispatch::DebugExceptions"
       middleware.use "ActionDispatch::Callbacks"
       middleware.use "ActionDispatch::ParamsParser"
       middleware.use "ActionDispatch::Cookies"
@@ -338,16 +339,19 @@ end
 module ActionDispatch
   class ShowExceptions
     private
-      remove_method :public_path
-      def public_path
-        "#{FIXTURE_LOAD_PATH}/public"
-      end
+    remove_method :public_path
+    def public_path
+      "#{FIXTURE_LOAD_PATH}/public"
+    end
+  end
 
-      remove_method :stderr_logger
-      # Silence logger
-      def stderr_logger
-        nil
-      end
+  class DebugExceptions
+    private
+    remove_method :stderr_logger
+    # Silence logger
+    def stderr_logger
+      nil
+    end
   end
 end
 
diff --git a/actionpack/test/controller/show_exceptions_test.rb b/actionpack/test/controller/show_exceptions_test.rb
index 74067cb895..5eff1eb09d 100644
--- a/actionpack/test/controller/show_exceptions_test.rb
+++ b/actionpack/test/controller/show_exceptions_test.rb
@@ -3,6 +3,7 @@ require 'abstract_unit'
 module ShowExceptions
   class ShowExceptionsController < ActionController::Base
     use ActionDispatch::ShowExceptions
+    use ActionDispatch::DebugExceptions
 
     def boom
       raise 'boom!'
diff --git a/actionpack/test/dispatch/show_exceptions_test.rb b/actionpack/test/dispatch/show_exceptions_test.rb
index 90f13a3bb9..4f1140b5d3 100644
--- a/actionpack/test/dispatch/show_exceptions_test.rb
+++ b/actionpack/test/dispatch/show_exceptions_test.rb
@@ -29,8 +29,8 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
     end
   end
 
-  ProductionApp = ActionDispatch::ShowExceptions.new(Boomer.new(false))
-  DevelopmentApp = ActionDispatch::ShowExceptions.new(Boomer.new(true))
+  ProductionApp  = ActionDispatch::ShowExceptions.new(ActionDispatch::DebugExceptions.new(Boomer.new(false)))
+  DevelopmentApp = ActionDispatch::ShowExceptions.new(ActionDispatch::DebugExceptions.new(Boomer.new(true)))
 
   test "rescue with error page when show_exceptions is false" do
     @app = ProductionApp