Fix ActionMailer plugin to add rescue_from to the correct class (#1143

)

* make ActiveJob plugin a true plugin, with disable

* add rescue_from to ActionMailer::Base, not MailDeliveryJob

* test for Rails 6+ first. (DeliveryJob is always defined.)

* limit DeliveryJob tests to Rails < 6.x

* update test app config for rails 6+

* handle undefined job_id

* refactor to test both DeliveryJob and MailDeliveryJob

lib/rollbar/configuration.rb

@@ -18,6 +18,7 @@ class Configuration
                   :custom_data_method,
                   :default_logger,
                   :delayed_job_enabled,
+                  :disable_action_mailer_monkey_patch,
                   :disable_core_monkey_patch,
                   :disable_monkey_patch,
                   :disable_rack_monkey_patch,
@@ -99,6 +100,7 @@ def initialize
       @logger_level = :info
       @delayed_job_enabled = true
       @disable_monkey_patch = false
+      @disable_action_mailer_monkey_patch = false
       @disable_core_monkey_patch = false
       @disable_rack_monkey_patch = false
       @enable_error_context = true

lib/rollbar/plugins/active_job.rb

@@ -1,35 +1,44 @@
-module Rollbar
-  # Report any uncaught errors in a job to Rollbar and reraise
-  module ActiveJob
-    def self.included(base)
-      base.send :rescue_from, Exception do |exception|
-        args = if self.class.respond_to?(:log_arguments?) && !self.class.log_arguments?
-                 arguments.map(&Rollbar::Scrubbers.method(:scrub_value))
-               else
-                 arguments
-               end
+Rollbar.plugins.define('active_job') do
+  dependency { !configuration.disable_monkey_patch }
+  dependency { !configuration.disable_action_mailer_monkey_patch }
 
-        Rollbar.error(exception,
-                      :job => self.class.name,
-                      :job_id => job_id,
-                      :use_exception_level_filters => true,
-                      :arguments => args)
-        raise exception
+  execute do
+    module Rollbar
+      # Report any uncaught errors in a job to Rollbar and reraise
+      module ActiveJob
+        def self.included(base)
+          base.send :rescue_from, Exception do |exception|
+            args = if self.class.respond_to?(:log_arguments?) && !self.class.log_arguments?
+                     arguments.map(&Rollbar::Scrubbers.method(:scrub_value))
+                   else
+                     arguments
+                   end
+
+            job_data = {
+              :job => self.class.name,
+              :use_exception_level_filters => true,
+              :arguments => args
+            }
+
+            # job_id isn't present when this is a mailer class.
+            job_data[:job_id] = job_id if defined?(job_id)
+
+            Rollbar.error(exception, job_data)
+            raise exception
+          end
+        end
       end
     end
-  end
-end
 
-if defined?(ActiveSupport) && ActiveSupport.respond_to?(:on_load)
-  ActiveSupport.on_load(:action_mailer) do
-    # Automatically add to ActionMailer::DeliveryJob
-    if defined?(ActionMailer::DeliveryJob)
-      ActionMailer::DeliveryJob.send(:include,
-                                     Rollbar::ActiveJob)
-    end
-    # Rails >= 6.0
-    if defined?(ActionMailer::MailDeliveryJob)
-      ActionMailer::MailDeliveryJob.send(:include, Rollbar::ActiveJob)
+    if defined?(ActiveSupport) && ActiveSupport.respond_to?(:on_load)
+      ActiveSupport.on_load(:action_mailer) do
+        if defined?(ActionMailer::MailDeliveryJob) # Rails >= 6.0
+          ActionMailer::Base.send(:include, Rollbar::ActiveJob)
+        elsif defined?(ActionMailer::DeliveryJob) # Rails < 6.0
+          ActionMailer::DeliveryJob.send(:include,
+                                         Rollbar::ActiveJob)
+        end
+      end
     end
   end
 end

spec/dummyapp/config/application.rb

@@ -58,5 +58,9 @@ class Application < Rails::Application
     if Gem::Version.new(Rails.version) >= Gem::Version.new('4.2.0')
       config.active_job.queue_adapter = :inline
     end
+
+    if Gem::Version.new(Rails.version) >= Gem::Version.new('6.0.0')
+      config.load_defaults 6.0
+    end
   end
 end

spec/rollbar/plugins/active_job_spec.rb

@@ -22,59 +22,46 @@ def perform(exception, job_id)
     end
   end
 
+  class TestMailer < ActionMailer::Base
+    attr_accessor :arguments
+
+    def test_email(*_arguments)
+      error = StandardError.new('oh no')
+      raise(error)
+    end
+  end
+
   before { reconfigure_notifier }
 
   let(:exception) { StandardError.new('oh no') }
   let(:job_id) { '123' }
   let(:argument) { 12 }
-
-  it 'reports the error to Rollbar' do
-    expected_params = {
+  let(:expected_params) do
+    {
       :job => 'TestJob',
       :job_id => job_id,
       :use_exception_level_filters => true,
       :arguments => [argument]
     }
-    expect(Rollbar).to receive(:error).with(exception, expected_params)
-    begin
-      TestJob.new(argument).perform(exception, job_id)
-    rescue StandardError
-      nil
-    end
   end
 
-  it 'reraises the error so the job backend can handle the failure and retry' do
-    expect do
-      TestJob.new(argument).perform(exception, job_id)
-    end.to raise_error exception
-  end
-
-  it 'scrubs all arguments if job has `log_arguments` disabled' do
-    allow(TestJob).to receive(:log_arguments?).and_return(false)
-
-    expected_params = {
-      :job => 'TestJob',
-      :job_id => job_id,
-      :use_exception_level_filters => true,
-      :arguments => ['******', '******', '******']
-    }
-    expect(Rollbar).to receive(:error).with(exception, expected_params)
-    begin
-      TestJob.new(1, 2, 3).perform(exception, job_id)
-    rescue StandardError
-      nil
+  shared_examples 'an ActiveMailer plugin' do
+    it 'reraises the error so the job backend can handle the failure and retry' do
+      expect do
+        TestJob.new(argument).perform(exception, job_id)
+      end.to raise_error exception
     end
-  end
-
-  context 'using ActionMailer::DeliveryJob', :if => defined?(ActionMailer::DeliveryJob) do
-    include ActiveJob::TestHelper if defined?(ActiveJob::TestHelper)
 
-    class TestMailer < ActionMailer::Base
-      attr_accessor :arguments
+    it 'scrubs all arguments if job has `log_arguments` disabled' do
+      allow(TestJob).to receive(:log_arguments?).and_return(false)
 
-      def test_email(*_arguments)
-        error = StandardError.new('oh no')
-        raise(error)
+      expected_params[:job_id] = job_id
+      expected_params[:arguments] = ['******', '******', '******']
+      expect(Rollbar).to receive(:error).with(exception, expected_params)
+      begin
+        TestJob.new(1, 2, 3).perform(exception, job_id)
+      rescue StandardError
+        nil
       end
     end
 
@@ -85,7 +72,36 @@ def test_email(*_arguments)
       end.to have_enqueued_job.on_queue('mailers')
     end
 
-    it 'reports the error to Rollbar' do
+    context 'disabled in config' do
+      subject { Rollbar.plugins.get('active_job') }
+      before do
+        subject.unload!
+
+        # Configur will load all plugins after applying the config.
+        Rollbar.configure { |c| c.disable_action_mailer_monkey_patch = true }
+      end
+
+      it "isn't loaded" do
+        expect(subject.loaded).to eq(false)
+      end
+    end
+  end
+
+  context 'using ActionMailer::DeliveryJob', :if => Gem::Version.new(Rails.version) < Gem::Version.new('6.0') do
+    include ActiveJob::TestHelper if defined?(ActiveJob::TestHelper)
+
+    it_behaves_like 'an ActiveMailer plugin'
+
+    it 'reports the job error to Rollbar' do
+      expect(Rollbar).to receive(:error).with(exception, expected_params)
+      begin
+        TestJob.new(argument).perform(exception, job_id)
+      rescue StandardError
+        nil
+      end
+    end
+
+    it 'reports the mailer error to Rollbar' do
       expected_params = {
         :job => 'ActionMailer::DeliveryJob',
         :use_exception_level_filters => true,
@@ -137,4 +153,33 @@ def test_email(*_arguments)
              .should match(/^*+$/)
     end
   end
+
+  context 'using ActionMailer::MailDeliveryJob', :if => Gem::Version.new(Rails.version) >= Gem::Version.new('6.0') do
+    include ActiveJob::TestHelper if defined?(ActiveJob::TestHelper)
+
+    let(:expected_params) do
+      {
+        :job => 'TestJob',
+        :use_exception_level_filters => true
+      }
+    end
+
+    it_behaves_like 'an ActiveMailer plugin'
+
+    it 'reports the error to Rollbar' do
+      expected_params.delete(:job)
+
+      # In 6+, the re-raise in the plugin will cause the rescue_from to be called twice.
+      expect(Rollbar).to receive(:error).twice.with(kind_of(StandardError),
+                                              hash_including(expected_params))
+
+      perform_enqueued_jobs do
+        begin
+          TestMailer.test_email(argument).deliver_later
+        rescue StandardError => e
+          nil
+        end
+      end
+    end
+  end
 end