Skip to content

Commit a8d1d92

Browse files
nvasilevskirafaelfranca
authored andcommitted
Do not report rendered errors except 500
In `4067c9565a5da78a72e375a2d959000147f02c34` `ActionDispatch::Executor` started to report all errors, even the ones that were "handled" by the application. This leads to errors like `ActionController::RoutingError` polluting error trackers while not being actionable since they do not represent an exceptional situation. This commit changes the behavior to only report errors that are not considered "handled" based on the `ActionDispatch::ExceptionWrapper.rescue_responses` list.
1 parent c676398 commit a8d1d92

File tree

3 files changed

+29
-2
lines changed

3 files changed

+29
-2
lines changed

actionpack/lib/action_dispatch/middleware/executor.rb

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,12 @@ def call(env)
1414
state = @executor.run!(reset: true)
1515
begin
1616
response = @app.call(env)
17-
if rendered_error = env["action_dispatch.exception"]
18-
@executor.error_reporter.report(rendered_error, handled: false, source: "application.action_dispatch")
17+
18+
if env["action_dispatch.report_exception"]
19+
error = env["action_dispatch.exception"]
20+
@executor.error_reporter.report(error, handled: false, source: "application.action_dispatch")
1921
end
22+
2023
returned = response << ::Rack::BodyProxy.new(response.pop) { state.complete! }
2124
rescue => error
2225
@executor.error_reporter.report(error, handled: false, source: "application.action_dispatch")

actionpack/lib/action_dispatch/middleware/show_exceptions.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ def call(env)
3535
backtrace_cleaner = request.get_header("action_dispatch.backtrace_cleaner")
3636
wrapper = ExceptionWrapper.new(backtrace_cleaner, exception)
3737
request.set_header "action_dispatch.exception", wrapper.unwrapped_exception
38+
request.set_header "action_dispatch.report_exception", !wrapper.rescue_response?
3839

3940
if wrapper.show?(request)
4041
render_exception(request.dup, wrapper)

actionpack/test/dispatch/executor_test.rb

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# frozen_string_literal: true
22

33
require "abstract_unit"
4+
require "active_support/core_ext/object/with"
45

56
class ExecutorTest < ActiveSupport::TestCase
67
class MyBody < Array
@@ -147,6 +148,28 @@ def test_error_reporting_with_show_exception
147148
assert_instance_of TypeError, error_report.error
148149
end
149150

151+
class BusinessAsUsual < StandardError; end
152+
153+
def test_handled_error_is_not_reported
154+
middleware = Rack::Lint.new(
155+
ActionDispatch::Executor.new(
156+
ActionDispatch::ShowExceptions.new(
157+
Rack::Lint.new(->(_env) { raise BusinessAsUsual }),
158+
->(env) { [418, {}, ["I'm a teapot"]] },
159+
),
160+
executor,
161+
)
162+
)
163+
164+
env = Rack::MockRequest.env_for("", {})
165+
ActionDispatch::ExceptionWrapper.with(rescue_responses: { BusinessAsUsual.name => 418 }) do
166+
assert_no_error_reported do
167+
response = middleware.call(env)
168+
assert_equal 418, response[0]
169+
end
170+
end
171+
end
172+
150173
private
151174
def call_and_return_body(&block)
152175
app = block || proc { [200, {}, []] }

0 commit comments

Comments
 (0)