diff --git a/CHANGELOG.md b/CHANGELOG.md index 449f5d25a..4b88ef31c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ Changelog | [#692](https://github.com/bugsnag/bugsnag-ruby/pull/692) * Add `request` to `Report`/`Event` containing HTTP request metadata | [#693](https://github.com/bugsnag/bugsnag-ruby/pull/693) +* Add `add_metadata` and `clear_metadata` to `Report`/`Event` + | [#694](https://github.com/bugsnag/bugsnag-ruby/pull/694) ### Fixes @@ -46,6 +48,7 @@ Changelog * `Report#exceptions` has been deprecated in favour of the new `errors` property * `Report#raw_exceptions` has been deprecated in favour of the new `original_error` property * Accessing request data via `Report#metadata` has been deprecated in favour of using the new `request` property. Request data will be moved out of metadata in the next major version +* The `Report#add_tab` and `Report#remove_tab` methods have been deprecated in favour of the new `add_metadata` and `clear_metadata` methods ## v6.22.1 (11 August 2021) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index 6c47e3f5e..f19d50192 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -34,6 +34,8 @@ require "bugsnag/breadcrumbs/breadcrumb" require "bugsnag/breadcrumbs/breadcrumbs" +require "bugsnag/utility/metadata_delegate" + # rubocop:todo Metrics/ModuleLength module Bugsnag LOCK = Mutex.new diff --git a/lib/bugsnag/report.rb b/lib/bugsnag/report.rb index c1c021274..a18a7bd5d 100644 --- a/lib/bugsnag/report.rb +++ b/lib/bugsnag/report.rb @@ -143,6 +143,8 @@ def initialize(exception, passed_configuration, auto_notify=false) self.severity = auto_notify ? "error" : "warning" self.severity_reason = auto_notify ? {:type => UNHANDLED_EXCEPTION} : {:type => HANDLED_EXCEPTION} self.user = {} + + @metadata_delegate = Utility::MetadataDelegate.new end ## @@ -172,6 +174,8 @@ def context # exists, this will be merged with the existing values. If a Hash is not # given, the value will be placed into the 'custom' tab # @return [void] + # + # @deprecated Use {#add_metadata} instead def add_tab(name, value) return if name.nil? @@ -190,6 +194,8 @@ def add_tab(name, value) # # @param name [String] # @return [void] + # + # @deprecated Use {#clear_metadata} instead def remove_tab(name) return if name.nil? @@ -316,6 +322,43 @@ def request @meta_data[:request] end + ## + # Add values to metadata + # + # @overload add_metadata(section, data) + # Merges data into the given section of metadata + # @param section [String, Symbol] + # @param data [Hash] + # + # @overload add_metadata(section, key, value) + # Sets key to value in the given section of metadata. If the value is nil + # the key will be deleted + # @param section [String, Symbol] + # @param key [String, Symbol] + # @param value + # + # @return [void] + def add_metadata(section, key_or_data, *args) + @metadata_delegate.add_metadata(@meta_data, section, key_or_data, *args) + end + + ## + # Clear values from metadata + # + # @overload clear_metadata(section) + # Clears the given section of metadata + # @param section [String, Symbol] + # + # @overload clear_metadata(section, key) + # Clears the key in the given section of metadata + # @param section [String, Symbol] + # @param key [String, Symbol] + # + # @return [void] + def clear_metadata(section, *args) + @metadata_delegate.clear_metadata(@meta_data, section, *args) + end + private def generate_exception_list diff --git a/lib/bugsnag/utility/metadata_delegate.rb b/lib/bugsnag/utility/metadata_delegate.rb new file mode 100644 index 000000000..8b43c545d --- /dev/null +++ b/lib/bugsnag/utility/metadata_delegate.rb @@ -0,0 +1,102 @@ +module Bugsnag::Utility + # @api private + class MetadataDelegate + # nil is a valid metadata value, so we need a sentinel object so we can tell + # if the value parameter has been provided + NOT_PROVIDED = Object.new + + ## + # Add values to metadata + # + # @overload add_metadata(metadata, section, data) + # Merges data into the given section of metadata + # @param metadata [Hash] The metadata hash to operate on + # @param section [String, Symbol] + # @param data [Hash] + # + # @overload add_metadata(metadata, section, key, value) + # Sets key to value in the given section of metadata. If the value is nil + # the key will be deleted + # @param metadata [Hash] The metadata hash to operate on + # @param section [String, Symbol] + # @param key [String, Symbol] + # @param value + # + # @return [void] + def add_metadata(metadata, section, key_or_data, value = NOT_PROVIDED) + case value + when NOT_PROVIDED + merge_metadata(metadata, section, key_or_data) + when nil + clear_metadata(metadata, section, key_or_data) + else + overwrite_metadata(metadata, section, key_or_data, value) + end + end + + ## + # Clear values from metadata + # + # @overload clear_metadata(metadata, section) + # Clears the given section of metadata + # @param metadata [Hash] The metadata hash to operate on + # @param section [String, Symbol] + # + # @overload clear_metadata(metadata, section, key) + # Clears the key in the given section of metadata + # @param metadata [Hash] The metadata hash to operate on + # @param section [String, Symbol] + # @param key [String, Symbol] + # + # @return [void] + def clear_metadata(metadata, section, key = nil) + if key.nil? + metadata.delete(section) + elsif metadata[section] + metadata[section].delete(key) + end + end + + private + + ## + # Merge new metadata into the existing metadata + # + # Any keys with a 'nil' value in the new metadata will be deleted from the + # existing metadata + # + # @param existing_metadata [Hash] + # @param section [String, Symbol] + # @param new_metadata [Hash] + # @return [void] + def merge_metadata(existing_metadata, section, new_metadata) + return unless new_metadata.is_a?(Hash) + + existing_metadata[section] ||= {} + data = existing_metadata[section] + + new_metadata.each do |key, value| + if value.nil? + data.delete(key) + else + data[key] = value + end + end + end + + ## + # Overwrite the value in metadata's section & key + # + # @param metadata [Hash] + # @param section [String, Symbol] + # @param key [String, Symbol] + # @param value + # @return [void] + def overwrite_metadata(metadata, section, key, value) + return unless key.is_a?(String) || key.is_a?(Symbol) + + metadata[section] ||= {} + metadata[section][key] = value + end + end +end diff --git a/spec/report_spec.rb b/spec/report_spec.rb index afdb4ab53..173f1d2c7 100644 --- a/spec/report_spec.rb +++ b/spec/report_spec.rb @@ -2,6 +2,7 @@ require_relative './spec_helper' require 'securerandom' require 'ostruct' +require 'support/shared_examples_for_metadata' module ActiveRecord; class RecordNotFound < RuntimeError; end; end class NestedException < StandardError; attr_accessor :original_exception; end @@ -28,6 +29,24 @@ def gloops end shared_examples "Report or Event tests" do |class_to_test| + context "metadata" do + include_examples( + "metadata delegate", + lambda do |metadata, *args| + report = class_to_test.new(RuntimeError.new, Bugsnag.configuration) + report.metadata = metadata + + report.add_metadata(*args) + end, + lambda do |metadata, *args| + report = class_to_test.new(RuntimeError.new, Bugsnag.configuration) + report.metadata = metadata + + report.clear_metadata(*args) + end + ) + end + it "#headers should return the correct request headers" do fake_now = Time.gm(2020, 1, 2, 3, 4, 5, 123456) expect(Time).to receive(:now).twice.and_return(fake_now) @@ -349,7 +368,53 @@ def gloops end end - # TODO: nested context + it "metadata added with 'add_metadata' ends up in the payload" do + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_metadata( + :some_tab, + { info: "here", data: "also here" } + ) + + report.add_metadata(:some_other_tab, :info, true) + report.add_metadata(:some_other_tab, :data, "very true") + end + + expect(Bugsnag).to(have_sent_notification { |payload, _headers| + event = get_event_from_payload(payload) + expect(event["metaData"]).to eq({ + "some_tab" => { + "info" => "here", + "data" => "also here" + }, + "some_other_tab" => { + "info" => true, + "data" => "very true" + } + }) + }) + end + + it "metadata removed with 'clear_metadata' does not end up in the payload" do + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_metadata( + :some_tab, + { info: "here", data: "also here" } + ) + + report.add_metadata(:some_other_tab, :info, true) + report.add_metadata(:some_other_tab, :data, "very true") + + report.clear_metadata(:some_tab) + report.clear_metadata(:some_other_tab, :info) + end + + expect(Bugsnag).to(have_sent_notification { |payload, _headers| + event = get_event_from_payload(payload) + expect(event["metaData"]).to eq({ + "some_other_tab" => { "data" => "very true" } + }) + }) + end it "accepts tabs in overrides and adds them to metaData" do Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| diff --git a/spec/support/shared_examples_for_metadata.rb b/spec/support/shared_examples_for_metadata.rb new file mode 100644 index 000000000..eb869abe7 --- /dev/null +++ b/spec/support/shared_examples_for_metadata.rb @@ -0,0 +1,157 @@ +require 'spec_helper' + +# shared examples for #add_metadata and #clear_metadata +# use by providing two functions - one that implements add_metadata and one +# that implements clear_metadata, e.g. +# +# RSpec.describe SomeClass do +# include_examples( +# 'metadata delegate', +# ->(metadata, *args) { SomeClass.new(metadata).add_metadata(*args) }, +# ->(metadata, *args) { SomeClass.new(metadata).clear_metadata(*args) } +# ) +# end +RSpec.shared_examples 'metadata delegate' do |add_metadata, clear_metadata| + context "#add_metadata" do + context "with 'section', 'key' and 'value'" do + it "adds the given key/value pair to the section" do + metadata = {} + + add_metadata.call(metadata, :abc, :xyz, "Hello!") + + expect(metadata).to eq({ abc: { xyz: "Hello!" } }) + end + + it "merges the new key/value pair with existing data in the section" do + metadata = { abc: { a: 1, b: 2, c: 3 } } + + add_metadata.call(metadata, :abc, :xyz, "Hello!") + + expect(metadata).to eq({ abc: { a: 1, b: 2, c: 3, xyz: "Hello!" } }) + end + + it "replaces existing metadata if the 'key' already exists" do + metadata = { abc: { xyz: "Hello!" } } + + add_metadata.call(metadata, :abc, :xyz, "Goodbye!") + + expect(metadata).to eq({ abc: { xyz: "Goodbye!" } }) + end + + it "removes the key if 'value' is nil" do + metadata = { abc: { xyz: "Hello!" } } + + add_metadata.call(metadata, :abc, :xyz, nil) + + expect(metadata).to eq({ abc: {} }) + end + end + + context "with 'section' and 'data'" do + it "adds the data to the given section" do + metadata = {} + + add_metadata.call(metadata, :xyz, { abc: "Hello!" }) + + expect(metadata).to eq({ xyz: { abc: "Hello!" } }) + end + + it "merges the new data with any existing data in the section" do + metadata = { xyz: { x: 1, y: 2, z: 3 } } + + add_metadata.call(metadata, :xyz, { abc: "Hello!" }) + + expect(metadata).to eq({ xyz: { x: 1, y: 2, z: 3, abc: "Hello!" } }) + end + + it "does not deep merge conflicting data in the section" do + metadata = { xyz: { x: { a: 1, b: 2 } } } + + add_metadata.call(metadata, :xyz, { x: { c: 3 } }) + + # if this was a deep merge, metadata[:xyz][:x] would be { a: 1, b: 2, c: 3 } + expect(metadata).to eq({ xyz: { x: { c: 3 } } }) + end + + it "replaces existing keys in the section" do + metadata = { xyz: { x: 0, z: "yes", abc: "??" } } + + add_metadata.call(metadata, :xyz, { x: 1, y: 2, z: 3 }) + + expect(metadata).to eq({ xyz: { x: 1, y: 2, z: 3, abc: "??" } }) + end + + it "removes keys that have a value of 'nil'" do + metadata = { xyz: { x: 0, z: "yes", abc: "??" } } + + add_metadata.call(metadata, :xyz, { x: nil, y: 2, z: 3 }) + + expect(metadata).to eq({ xyz: { y: 2, z: 3, abc: "??" } }) + end + end + + context "with bad parameters" do + it "does nothing if called with a 'key' but no 'value'" do + metadata = {} + + add_metadata.call(metadata, :a, :b) + + expect(metadata).to be_empty + end + + it "does nothing if called with a Hash 'key' and a value" do + metadata = {} + + add_metadata.call(metadata, :a, { b: 1 }, 123) + + expect(metadata).to be_empty + end + end + end + + context "#clear_metadata" do + context "with 'section'" do + it "removes the given section from metadata" do + metadata = { some: "data", goes: "here" } + + clear_metadata.call(metadata, :some) + + expect(metadata).to eq({ goes: "here" }) + end + + it "does nothing if the section does not exist" do + metadata = { some: "data", goes: "here" } + + clear_metadata.call(metadata, :does_not_exist) + + expect(metadata).to eq({ some: "data", goes: "here" }) + end + end + + context "with 'section' and 'key'" do + it "removes the given 'key' from 'section'" do + metadata = { some: { data: { goes: "here" }, also: "there" } } + + clear_metadata.call(metadata, :some, :data) + + expect(metadata).to eq({ some: { also: "there" } }) + end + + it "does nothing if the section does not exist" do + metadata = { some: { data: { goes: "here" }, also: "there" } } + + clear_metadata.call(metadata, :nope, :data) + + expect(metadata).to eq({ some: { data: { goes: "here" }, also: "there" } }) + end + + it "does nothing if the key does not exist in the section" do + metadata = { some: { data: { goes: "here" }, also: "there" } } + + clear_metadata.call(metadata, :some, :nah) + + expect(metadata).to eq({ some: { data: { goes: "here" }, also: "there" } }) + end + end + end +end diff --git a/spec/utility/metadata_delegate_spec.rb b/spec/utility/metadata_delegate_spec.rb new file mode 100644 index 000000000..f08edcdff --- /dev/null +++ b/spec/utility/metadata_delegate_spec.rb @@ -0,0 +1,11 @@ +require 'spec_helper' +require 'bugsnag/utility/metadata_delegate' +require 'support/shared_examples_for_metadata' + +RSpec.describe Bugsnag::Utility::MetadataDelegate do + include_examples( + 'metadata delegate', + Bugsnag::Utility::MetadataDelegate.new.method(:add_metadata), + Bugsnag::Utility::MetadataDelegate.new.method(:clear_metadata) + ) +end