Skip to content

Commit

Permalink
Error reporting API: Add a source attribute
Browse files Browse the repository at this point in the history
Ref: #43625 (comment)

Some users may not be interested by some internal errors.
By providing a `source` attribute we allow to easilly filter
these errors out.
  • Loading branch information
byroot committed May 5, 2022
1 parent df1e1bc commit c9a2bc2
Show file tree
Hide file tree
Showing 7 changed files with 41 additions and 19 deletions.
7 changes: 7 additions & 0 deletions activesupport/CHANGELOG.md
@@ -1,3 +1,10 @@
* `ActiveSupport::ErrorReporter` now accepts and forward a `source:` parameter.

This allow libraries to signal the origin of the errors, and reporters
to easily ignore some sources.

*Jean Boussier*

* Fix and add protections for XSS in `ActionView::Helpers` and `ERB::Util`.

Add the method `ERB::Util.xml_name_escape` to escape dangerous characters
Expand Down
6 changes: 5 additions & 1 deletion activesupport/lib/active_support/cache/mem_cache_store.rb
Expand Up @@ -300,8 +300,12 @@ def deserialize_entry(payload, raw: false, **)
def rescue_error_with(fallback)
yield
rescue Dalli::DalliError => error
ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning)
logger.error("DalliError (#{error}): #{error.message}") if logger
ActiveSupport.error_reporter&.report(
error,
severity: :warning,
source: "mem_cache_store.active_support",
)
fallback
end
end
Expand Down
6 changes: 5 additions & 1 deletion activesupport/lib/active_support/cache/redis_cache_store.rb
Expand Up @@ -60,6 +60,11 @@ class RedisCacheStore < Store
if logger
logger.error { "RedisCacheStore: #{method} failed, returned #{returning.inspect}: #{exception.class}: #{exception.message}" }
end
ActiveSupport.error_reporter&.report(
exception,
severity: :warning,
source: "redis_cache_store.active_support",
)
end

# The maximum number of entries to receive per SCAN call.
Expand Down Expand Up @@ -460,7 +465,6 @@ def serialize_entries(entries, **options)
def failsafe(method, returning: nil)
yield
rescue ::Redis::BaseError => error
ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning)
@error_handler&.call(method: method, exception: error, returning: returning)
returning
end
Expand Down
13 changes: 7 additions & 6 deletions activesupport/lib/active_support/error_reporter.rb
Expand Up @@ -40,6 +40,7 @@ module ActiveSupport
# end
class ErrorReporter
SEVERITIES = %i(error warning info)
DEFAULT_SOURCE = "application"

attr_accessor :logger

Expand All @@ -54,17 +55,17 @@ def initialize(*subscribers, logger: nil)
# 1 + '1'
# end
#
def handle(error_class = StandardError, severity: :warning, context: {}, fallback: nil)
def handle(error_class = StandardError, severity: :warning, context: {}, fallback: nil, source: DEFAULT_SOURCE)
yield
rescue error_class => error
report(error, handled: true, severity: severity, context: context)
report(error, handled: true, severity: severity, context: context, source: source)
fallback.call if fallback
end

def record(error_class = StandardError, severity: :error, context: {})
def record(error_class = StandardError, severity: :error, context: {}, source: DEFAULT_SOURCE)
yield
rescue error_class => error
report(error, handled: false, severity: severity, context: context)
report(error, handled: false, severity: severity, context: context, source: source)
raise
end

Expand Down Expand Up @@ -107,7 +108,7 @@ def set_context(...)
# When the block based +handle+ and +record+ methods are not suitable, you can directly use +report+
#
# Rails.error.report(error)
def report(error, handled: true, severity: handled ? :warning : :error, context: {})
def report(error, handled: true, severity: handled ? :warning : :error, context: {}, source: DEFAULT_SOURCE)
unless SEVERITIES.include?(severity)
raise ArgumentError, "severity must be one of #{SEVERITIES.map(&:inspect).join(", ")}, got: #{severity.inspect}"
end
Expand All @@ -116,7 +117,7 @@ def report(error, handled: true, severity: handled ? :warning : :error, context:
disabled_subscribers = ActiveSupport::IsolatedExecutionState[self]
@subscribers.each do |subscriber|
unless disabled_subscribers&.any? { |s| s === subscriber }
subscriber.report(error, handled: handled, severity: severity, context: full_context)
subscriber.report(error, handled: handled, severity: severity, context: full_context, source: source)
end
rescue => subscriber_error
if logger
Expand Down
2 changes: 1 addition & 1 deletion activesupport/lib/active_support/execution_wrapper.rb
Expand Up @@ -91,7 +91,7 @@ def self.wrap
begin
yield
rescue => error
error_reporter.report(error, handled: false)
error_reporter.report(error, handled: false, source: "unhandled_error.active_support")
raise
ensure
instance.complete!
Expand Down
20 changes: 13 additions & 7 deletions activesupport/test/error_reporter_test.rb
Expand Up @@ -15,8 +15,8 @@ def initialize
@events = []
end

def report(error, handled:, severity:, context:)
@events << [error, handled, severity, context]
def report(error, handled:, severity:, source:, context:)
@events << [error, handled, severity, source, context]
end
end

Expand All @@ -31,14 +31,20 @@ def report(error, handled:, severity:, context:)
@reporter.set_context(section: "admin")
error = ArgumentError.new("Oops")
@reporter.report(error, handled: true)
assert_equal [[error, true, :warning, { section: "admin" }]], @subscriber.events
assert_equal [[error, true, :warning, "application", { section: "admin" }]], @subscriber.events
end

test "passed context has priority over the execution context" do
@reporter.set_context(section: "admin")
error = ArgumentError.new("Oops")
@reporter.report(error, handled: true, context: { section: "public" })
assert_equal [[error, true, :warning, { section: "public" }]], @subscriber.events
assert_equal [[error, true, :warning, "application", { section: "public" }]], @subscriber.events
end

test "passed source is forwarded" do
error = ArgumentError.new("Oops")
@reporter.report(error, handled: true, source: "my_gem")
assert_equal [[error, true, :warning, "my_gem", {}]], @subscriber.events
end

test "#disable allow to skip a subscriber" do
Expand All @@ -60,7 +66,7 @@ def report(error, handled:, severity:, context:)
@reporter.handle do
raise error
end
assert_equal [[error, true, :warning, {}]], @subscriber.events
assert_equal [[error, true, :warning, "application", {}]], @subscriber.events
end

test "#handle can be scoped to an exception class" do
Expand Down Expand Up @@ -117,7 +123,7 @@ def report(error, handled:, severity:, context:)
raise error
end
end
assert_equal [[error, false, :error, {}]], @subscriber.events
assert_equal [[error, false, :error, "application", {}]], @subscriber.events
end

test "#record can be scoped to an exception class" do
Expand Down Expand Up @@ -164,7 +170,7 @@ def initialize(error)
@error = error
end

def report(_error, handled:, severity:, context:)
def report(_error, handled:, severity:, context:, source:)
raise @error
end
end
Expand Down
6 changes: 3 additions & 3 deletions activesupport/test/executor_test.rb
Expand Up @@ -13,8 +13,8 @@ def initialize
@events = []
end

def report(error, handled:, severity:, context:)
@events << [error, handled, severity, context]
def report(error, handled:, severity:, source:, context:)
@events << [error, handled, severity, source, context]
end
end

Expand All @@ -27,7 +27,7 @@ def test_wrap_report_errors
raise error
end
end
assert_equal [[error, false, :error, {}]], subscriber.events
assert_equal [[error, false, :error, "unhandled_error.active_support", {}]], subscriber.events
end

def test_wrap_invokes_callbacks
Expand Down

0 comments on commit c9a2bc2

Please sign in to comment.