Merge pull request #235 from rollbar/fix-broken-custom_data_method

Ensure that custom_data_method doesn't raise up an exception.

README.md

@@ -355,6 +355,8 @@ config.custom_data_method = lambda {
 
 This data will appear in the Occurrences tab and on the Occurrence Detail pages in the Rollbar interface.
 
+If your `custom_data_method` crashes while reporting an error, Rollbar will report that new error and will attach its uuid URL to the parent error report.
+
 ## Exception level filters
 
 By default, all uncaught exceptions are reported at the "error" level, except for the following, which are reported at "warning" level:

lib/rollbar.rb

@@ -35,6 +35,9 @@ module Rollbar
 
   class Notifier
     attr_accessor :configuration
+    attr_accessor :last_report
+
+    @file_semaphore = Mutex.new
 
     def initialize(parent_notifier = nil, payload_options = nil)
       if parent_notifier
@@ -48,11 +51,6 @@ def initialize(parent_notifier = nil, payload_options = nil)
       end
     end
 
-    attr_writer :configuration
-    attr_accessor :last_report
-
-    @file_semaphore = Mutex.new
-
     # Similar to configure below, but used only internally within the gem
     # to configure it without initializing any of the third party hooks
     def preconfigure
@@ -75,6 +73,17 @@ def scope!(options = {})
       self
     end
 
+    # Returns a new notifier with same configuration options
+    # but it sets Configuration#safely to true.
+    # We are using this flag to avoid having inifite loops
+    # when evaluating some custom user methods.
+    def safely
+      new_notifier = scope
+      new_notifier.configuration.safely = true
+
+      new_notifier
+    end
+
     # Turns off reporting for the given block.
     #
     # @example
@@ -195,6 +204,15 @@ def process_payload_safely(payload)
       report_internal_error(e)
     end
 
+    def custom_data
+      data = configuration.custom_data_method.call
+      Rollbar::Util.deep_copy(data)
+    rescue => e
+      return {} if configuration.safely?
+
+      report_custom_data_error(e)
+    end
+
     private
 
     def ignored?(exception, use_exception_level_filters = false)
@@ -312,10 +330,7 @@ def build_payload(level, message, exception, extra)
     end
 
     def build_payload_body(message, exception, extra)
-      unless configuration.custom_data_method.nil?
-        custom = Rollbar::Util.deep_copy(configuration.custom_data_method.call)
-        extra = Rollbar::Util.deep_merge(custom, extra || {})
-      end
+      extra = Rollbar::Util.deep_merge(custom_data, extra || {}) if custom_data_method?
 
       if exception
         build_payload_body_exception(message, exception, extra)
@@ -324,6 +339,20 @@ def build_payload_body(message, exception, extra)
       end
     end
 
+    def custom_data_method?
+      !!configuration.custom_data_method
+    end
+
+    def report_custom_data_error(e)
+      data = safely.error(e)
+
+      return {} unless data[:uuid]
+
+      uuid_url = uuid_rollbar_url(data)
+
+      { :_error_in_custom_data_method => uuid_url }
+    end
+
     def build_payload_body_exception(message, exception, extra)
       traces = trace_chain(exception)
 
@@ -650,10 +679,15 @@ def dump_payload(payload)
 
     def log_instance_link(data)
       if data[:uuid]
-        log_info "[Rollbar] Details: #{configuration.web_base}/instance/uuid?uuid=#{data[:uuid]} (only available if report was successful)"
+        uuid_url = uuid_rollbar_url(data)
+        log_info "[Rollbar] Details: #{uuid_url} (only available if report was successful)"
       end
     end
 
+    def uuid_rollbar_url(data)
+      "#{configuration.web_base}/instance/uuid?uuid=#{data[:uuid]}"
+    end
+
     def logger
       @logger ||= LoggerProxy.new(configuration.logger)
     end
@@ -700,6 +734,10 @@ def configuration
       @configuration ||= Configuration.new
     end
 
+    def safely?
+      configuration.safely?
+    end
+
     def require_hooks
       return if configuration.disable_monkey_patch
       wrap_delayed_worker

lib/rollbar/configuration.rb

@@ -30,6 +30,7 @@ class Configuration
     attr_accessor :report_dj_data
     attr_accessor :request_timeout
     attr_accessor :root
+    attr_accessor :safely
     attr_accessor :scrub_fields
     attr_accessor :uncaught_exception_level
     attr_accessor :scrub_headers
@@ -40,6 +41,8 @@ class Configuration
 
     attr_reader :project_gem_paths
 
+    alias_method :safely?, :safely
+
     DEFAULT_ENDPOINT = 'https://api.rollbar.com/api/1/item/'
     DEFAULT_WEB_BASE = 'https://rollbar.com'
 
@@ -75,6 +78,7 @@ def initialize
                        :confirm_password, :password_confirmation, :secret_token]
       @uncaught_exception_level = 'error'
       @scrub_headers = ['Authorization']
+      @safely = false
       @use_async = false
       @use_eventmachine = false
       @web_base = DEFAULT_WEB_BASE

spec/rollbar_spec.rb

@@ -13,6 +13,10 @@
 
 describe Rollbar do
   let(:notifier) { Rollbar.notifier }
+  before do
+    Rollbar.unconfigure
+    configure
+  end
 
   context 'when notifier has been used before configure it' do
     before do
@@ -287,11 +291,6 @@
     end
 
     context 'build_payload' do
-      after(:each) do
-        Rollbar.unconfigure
-        configure
-      end
-
       context 'a basic payload' do
         let(:extra_data) { {:key => 'value', :hash => {:inner_key => 'inner_value'}} }
         let(:payload) { notifier.send(:build_payload, 'info', 'message', nil, extra_data) }
@@ -409,6 +408,32 @@
         payload['data'][:body][:message][:extra][:f].should == 'f'
       end
 
+      context 'with custom_data_method crashing' do
+        next unless defined?(SecureRandom) and SecureRandom.respond_to?(:uuid)
+
+        let(:crashing_exception) { StandardError.new }
+        let(:custom_method) { proc { raise crashing_exception } }
+        let(:extra) { { :foo => :bar } }
+        let(:custom_data_report) do
+          { :_error_in_custom_data_method => SecureRandom.uuid }
+        end
+        let(:expected_extra) { extra.merge(custom_data_report) }
+
+        before do
+          notifier.configure do |config|
+            config.custom_data_method = custom_method
+          end
+
+          expect(notifier).to receive(:report_custom_data_error).once.and_return(custom_data_report)
+        end
+
+        it 'doesnt crash the report' do
+          payload = notifier.send(:build_payload, 'info', 'message', nil, extra)
+
+          expect(payload['data'][:body][:message][:extra]).to be_eql(expected_extra)
+        end
+      end
+
       it 'should include project_gem_paths' do
         notifier.configure do |config|
           config.project_gems = ['rails', 'rspec']
@@ -1664,6 +1689,35 @@ class DummyClass
     end
   end
 
+  describe '#custom_data' do
+    before do
+      Rollbar.configure do |config|
+        config.custom_data_method = proc { raise 'this-will-raise' } 
+      end
+
+      expect_any_instance_of(Rollbar::Notifier).to receive(:error).and_return(report_data)
+    end
+
+    context 'with uuid in reported data' do
+      next unless defined?(SecureRandom) and SecureRandom.respond_to?(:uuid)
+
+      let(:report_data) { { :uuid => SecureRandom.uuid } }
+      let(:expected_url) { "https://rollbar.com/instance/uuid?uuid=#{report_data[:uuid]}" }
+
+      it 'returns the uuid in :_error_in_custom_data_method' do
+        expect(notifier.custom_data).to be_eql(:_error_in_custom_data_method => expected_url)
+      end
+    end
+
+    context 'without uuid in reported data' do
+      let(:report_data) { { :some => 'other-data' } }
+
+      it 'returns the uuid in :_error_in_custom_data_method' do
+        expect(notifier.custom_data).to be_eql({})
+      end
+    end
+  end
+
   # configure with some basic params
   def configure
     reconfigure_notifier