Merge pull request #3717 from lest/show-exceptions-refactor

Show exceptions refactor: controller should be responsible for choice to show exceptions

6 files changed, 126 insertions, 67 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index dc52262503..fe422f71d5 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,5 +1,9 @@
 ## Rails 3.2.0 (unreleased) ##
 
+*   Refactor ActionDispatch::ShowExceptions. Controller is responsible for choice to show exceptions. *Sergey Nartimov*
+
+    It's possible to override +show_detailed_exceptions?+ in controllers to specify which requests should provide debugging information on errors.
+
 *   Responders now return 204 No Content for API requests without a response body (as in the new scaffold) *José Valim*
 
 *   Added ActionDispatch::RequestId middleware that'll make a unique X-Request-Id header available to the response and enables the ActionDispatch::Request#uuid method. This makes it easy to trace requests from end-to-end in the stack and to identify individual requests in mixed logs like Syslog *DHH*
diff --git a/actionpack/lib/action_controller/metal/rescue.rb b/actionpack/lib/action_controller/metal/rescue.rb
index eb037aa1b0..736ff5b31c 100644
--- a/actionpack/lib/action_controller/metal/rescue.rb
+++ b/actionpack/lib/action_controller/metal/rescue.rb
@@ -3,6 +3,11 @@ module ActionController #:nodoc:
     extend ActiveSupport::Concern
     include ActiveSupport::Rescuable
 
+    included do
+      config_accessor :consider_all_requests_local
+      self.consider_all_requests_local = false if consider_all_requests_local.nil?
+    end
+
     def rescue_with_handler(exception)
       if (exception.respond_to?(:original_exception) &&
           (orig_exception = exception.original_exception) &&
@@ -12,10 +17,15 @@ module ActionController #:nodoc:
       super(exception)
     end
 
+    def show_detailed_exceptions?
+      consider_all_requests_local || request.local?
+    end
+
     private
       def process_action(*args)
         super
       rescue Exception => exception
+        request.env['action_dispatch.show_detailed_exceptions'] = show_detailed_exceptions?
         rescue_with_handler(exception) || raise(exception)
       end
   end
diff --git a/actionpack/lib/action_controller/railtie.rb b/actionpack/lib/action_controller/railtie.rb
index f0c29825ba..de7b837ecc 100644
--- a/actionpack/lib/action_controller/railtie.rb
+++ b/actionpack/lib/action_controller/railtie.rb
@@ -21,6 +21,8 @@ module ActionController
       paths   = app.config.paths
       options = app.config.action_controller
 
+      options.consider_all_requests_local ||= app.config.consider_all_requests_local
+
       options.assets_dir           ||= paths["public"].first
       options.javascripts_dir      ||= paths["public/javascripts"].first
       options.stylesheets_dir      ||= paths["public/stylesheets"].first
diff --git a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb
index 2fa68c64c5..52dce4cc81 100644
--- a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb
+++ b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb
@@ -2,6 +2,7 @@ require 'active_support/core_ext/exception'
 require 'action_controller/metal/exceptions'
 require 'active_support/notifications'
 require 'action_dispatch/http/request'
+require 'active_support/deprecation'
 
 module ActionDispatch
   # This middleware rescues any exception returned by the application and renders
@@ -38,9 +39,9 @@ module ActionDispatch
        "application's log file and/or the web server's log file to find out what " <<
        "went wrong.</body></html>"]]
 
-    def initialize(app, consider_all_requests_local = false)
+    def initialize(app, consider_all_requests_local = nil)
+      ActiveSupport::Deprecation.warn "Passing consider_all_requests_local option to ActionDispatch::ShowExceptions middleware no longer works" unless consider_all_requests_local.nil?
       @app = app
-      @consider_all_requests_local = consider_all_requests_local
     end
 
     def call(env)
@@ -65,11 +66,10 @@ module ActionDispatch
         log_error(exception)
         exception = original_exception(exception)
 
-        request = Request.new(env)
-        if @consider_all_requests_local || request.local?
-          rescue_action_locally(request, exception)
+        if env['action_dispatch.show_detailed_exceptions'] == true
+          rescue_action_diagnostics(env, exception)
         else
-          rescue_action_in_public(exception)
+          rescue_action_error_page(exception)
         end
       rescue Exception => failsafe_error
         $stderr.puts "Error during failsafe response: #{failsafe_error}\n  #{failsafe_error.backtrace * "\n  "}"
@@ -78,9 +78,9 @@ module ActionDispatch
 
       # Render detailed diagnostics for unhandled exceptions rescued from
       # a controller action.
-      def rescue_action_locally(request, exception)
+      def rescue_action_diagnostics(env, exception)
         template = ActionView::Base.new([RESCUES_TEMPLATE_PATH],
-          :request => request,
+          :request => Request.new(env),
           :exception => exception,
           :application_trace => application_trace(exception),
           :framework_trace => framework_trace(exception),
@@ -98,7 +98,7 @@ module ActionDispatch
       # 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_in_public(exception)
+      def rescue_action_error_page(exception)
         status = status_code(exception)
         locale_path = "#{public_path}/#{status}.#{I18n.locale}.html" if I18n.locale
         path = "#{public_path}/#{status}.html"
diff --git a/actionpack/test/controller/show_exceptions_test.rb b/actionpack/test/controller/show_exceptions_test.rb
new file mode 100644
index 0000000000..39245e9574
--- /dev/null
+++ b/actionpack/test/controller/show_exceptions_test.rb
@@ -0,0 +1,59 @@
+require 'abstract_unit'
+
+module ShowExceptions
+  class ShowExceptionsController < ActionController::Base
+    use ActionDispatch::ShowExceptions
+
+    def boom
+      raise 'boom!'
+    end
+  end
+
+  class ShowExceptionsTest < ActionDispatch::IntegrationTest
+    test 'show error page from a remote ip' do
+      @app = ShowExceptionsController.action(:boom)
+      self.remote_addr = '208.77.188.166'
+      get '/'
+      assert_equal "500 error fixture\n", body
+    end
+
+    test 'show diagnostics from a local ip' do
+      @app = ShowExceptionsController.action(:boom)
+      ['127.0.0.1', '127.0.0.127', '::1', '0:0:0:0:0:0:0:1', '0:0:0:0:0:0:0:1%0'].each do |ip_address|
+        self.remote_addr = ip_address
+        get '/'
+        assert_match /boom/, body
+      end
+    end
+
+    test 'show diagnostics from a remote ip when consider_all_requests_local is true' do
+      ShowExceptionsController.any_instance.stubs(:consider_all_requests_local).returns(true)
+      @app = ShowExceptionsController.action(:boom)
+      self.remote_addr = '208.77.188.166'
+      get '/'
+      assert_match /boom/, body
+    end
+  end
+
+  class ShowExceptionsOverridenController < ShowExceptionsController
+    private
+
+    def show_detailed_exceptions?
+      params['detailed'] == '1'
+    end
+  end
+
+  class ShowExceptionsOverridenTest < ActionDispatch::IntegrationTest
+    test 'show error page' do
+      @app = ShowExceptionsOverridenController.action(:boom)
+      get '/', {'detailed' => '0'}
+      assert_equal "500 error fixture\n", body
+    end
+
+    test 'show diagnostics message' do
+      @app = ShowExceptionsOverridenController.action(:boom)
+      get '/', {'detailed' => '1'}
+      assert_match /boom/, body
+    end
+  end
+end
diff --git a/actionpack/test/dispatch/show_exceptions_test.rb b/actionpack/test/dispatch/show_exceptions_test.rb
index 42f6c7f79f..9f4d6c530f 100644
--- a/actionpack/test/dispatch/show_exceptions_test.rb
+++ b/actionpack/test/dispatch/show_exceptions_test.rb
@@ -2,32 +2,38 @@ require 'abstract_unit'
 
 class ShowExceptionsTest < ActionDispatch::IntegrationTest
 
-  Boomer = lambda do |env|
-    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!"
+  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::ShowExceptions.new(Boomer, false)
-  DevelopmentApp = ActionDispatch::ShowExceptions.new(Boomer, true)
+  ProductionApp = ActionDispatch::ShowExceptions.new(Boomer.new(false))
+  DevelopmentApp = ActionDispatch::ShowExceptions.new(Boomer.new(true))
 
-  test "rescue in public from a remote ip" do
+  test "rescue with error page when show_exceptions is false" do
     @app = ProductionApp
-    self.remote_addr = '208.77.188.166'
 
     get "/", {}, {'action_dispatch.show_exceptions' => true}
     assert_response 500
@@ -42,32 +48,28 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
     assert_equal "", body
   end
 
-  test "rescue locally from a local request" do
-    @app = ProductionApp
-    ['127.0.0.1', '127.0.0.127', '::1', '0:0:0:0:0:0:0:1', '0:0:0:0:0:0:0:1%0'].each do |ip_address|
-      self.remote_addr = ip_address
+  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 "/", {}, {'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 "/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
+    get "/method_not_allowed", {}, {'action_dispatch.show_exceptions' => true}
+    assert_response 405
+    assert_match(/ActionController::MethodNotAllowed/, body)
   end
 
-  test "localize public rescue message" do
+  test "localize rescue error page" do
     # Change locale
     old_locale, I18n.locale = I18n.locale, :da
 
     begin
       @app = ProductionApp
-      self.remote_addr = '208.77.188.166'
 
       get "/", {}, {'action_dispatch.show_exceptions' => true}
       assert_response 500
@@ -81,23 +83,6 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
     end
   end
 
-  test "always rescue locally in development mode" do
-    @app = DevelopmentApp
-    self.remote_addr = '208.77.188.166'
-
-    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
 
@@ -107,16 +92,15 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
     assert_match("&quot;foo&quot;=&gt;&quot;[FILTERED]&quot;", body)
   end
 
-  test "show registered original exception for wrapped exceptions when consider_all_requests_local is false" do
+  test "show registered original exception for wrapped exceptions when show_exceptions is false" do
     @app = ProductionApp
-    self.remote_addr = '208.77.188.166'
 
     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 consider_all_requests_local is true" do
+  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}
@@ -125,7 +109,7 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
   end
 
   test "show the controller name in the diagnostics template when controller name is present" do
-    @app = ProductionApp
+    @app = DevelopmentApp
     get("/runtime_error", {}, {
       'action_dispatch.show_exceptions' => true,
       'action_dispatch.request.parameters' => {