Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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

Show exceptions refactor: controller should be responsible for choice to show exceptions
  • Loading branch information...
commit 39ecbfdab9868cbd5166a7b0ee5b59c00389e2bd 2 parents 8cae31c + 3a1d519
@josevalim josevalim authored
View
4 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*
View
10 actionpack/lib/action_controller/metal/rescue.rb
@@ -3,6 +3,11 @@ module Rescue
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 @@ def rescue_with_handler(exception)
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
View
2  actionpack/lib/action_controller/railtie.rb
@@ -21,6 +21,8 @@ class Railtie < Rails::Railtie
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
View
18 actionpack/lib/action_dispatch/middleware/show_exceptions.rb
@@ -2,6 +2,7 @@
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 @@ class ShowExceptions
"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 @@ def render_exception(env, exception)
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 @@ def render_exception(env, exception)
# 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 @@ def rescue_action_locally(request, exception)
# 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"
View
59 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
View
100 actionpack/test/dispatch/show_exceptions_test.rb
@@ -2,32 +2,38 @@
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' => {
View
2  railties/lib/rails/application.rb
@@ -166,7 +166,7 @@ def default_middleware_stack
middleware.use ::Rack::MethodOverride
middleware.use ::ActionDispatch::RequestId
middleware.use ::Rails::Rack::Logger, config.log_tags # must come after Rack::MethodOverride to properly log overridden methods
- middleware.use ::ActionDispatch::ShowExceptions, config.consider_all_requests_local
+ middleware.use ::ActionDispatch::ShowExceptions
middleware.use ::ActionDispatch::RemoteIp, config.action_dispatch.ip_spoofing_check, config.action_dispatch.trusted_proxies
if config.action_dispatch.x_sendfile_header.present?
middleware.use ::Rack::Sendfile, config.action_dispatch.x_sendfile_header
Please sign in to comment.
Something went wrong with that request. Please try again.