From d3e0029f3fbc928adbe44647239dd9e290b1c8e4 Mon Sep 17 00:00:00 2001 From: Walt Jones Date: Wed, 27 Dec 2023 11:37:13 -0500 Subject: [PATCH 1/7] make ActiveJob plugin a true plugin, with disable --- lib/rollbar/configuration.rb | 2 + lib/rollbar/plugins/active_job.rb | 63 ++++++++++++++----------- spec/rollbar/plugins/active_job_spec.rb | 16 ++++++- 3 files changed, 52 insertions(+), 29 deletions(-) diff --git a/lib/rollbar/configuration.rb b/lib/rollbar/configuration.rb index 0c20a76e..202e8741 100644 --- a/lib/rollbar/configuration.rb +++ b/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 diff --git a/lib/rollbar/plugins/active_job.rb b/lib/rollbar/plugins/active_job.rb index b1baa84e..4951de9d 100644 --- a/lib/rollbar/plugins/active_job.rb +++ b/lib/rollbar/plugins/active_job.rb @@ -1,35 +1,42 @@ -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 + + Rollbar.error(exception, + :job => self.class.name, + :job_id => job_id, + :use_exception_level_filters => true, + :arguments => args) + 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 + # 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) + end + end end end end diff --git a/spec/rollbar/plugins/active_job_spec.rb b/spec/rollbar/plugins/active_job_spec.rb index f7c03070..90faabde 100644 --- a/spec/rollbar/plugins/active_job_spec.rb +++ b/spec/rollbar/plugins/active_job_spec.rb @@ -51,7 +51,7 @@ def perform(exception, job_id) 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, @@ -66,6 +66,20 @@ def perform(exception, job_id) end end + 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 + context 'using ActionMailer::DeliveryJob', :if => defined?(ActionMailer::DeliveryJob) do include ActiveJob::TestHelper if defined?(ActiveJob::TestHelper) From 4d4f391ee6bd11b96c4fe4acd93337aeaf292634 Mon Sep 17 00:00:00 2001 From: Walt Jones Date: Wed, 27 Dec 2023 11:43:29 -0500 Subject: [PATCH 2/7] add rescue_from to ActionMailer::Base, not MailDeliveryJob --- lib/rollbar/plugins/active_job.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/rollbar/plugins/active_job.rb b/lib/rollbar/plugins/active_job.rb index 4951de9d..d552ee4e 100644 --- a/lib/rollbar/plugins/active_job.rb +++ b/lib/rollbar/plugins/active_job.rb @@ -33,8 +33,8 @@ def self.included(base) Rollbar::ActiveJob) end # Rails >= 6.0 - if defined?(ActionMailer::MailDeliveryJob) - ActionMailer::MailDeliveryJob.send(:include, Rollbar::ActiveJob) + if defined?(ActionMailer::Base) + ActionMailer::Base.send(:include, Rollbar::ActiveJob) end end end From abd5156ea123729eb096d2da9851ba2e718e764b Mon Sep 17 00:00:00 2001 From: Walt Jones Date: Wed, 27 Dec 2023 16:51:37 -0500 Subject: [PATCH 3/7] test for Rails 6+ first. (DeliveryJob is always defined.) --- lib/rollbar/plugins/active_job.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/rollbar/plugins/active_job.rb b/lib/rollbar/plugins/active_job.rb index d552ee4e..ced79aef 100644 --- a/lib/rollbar/plugins/active_job.rb +++ b/lib/rollbar/plugins/active_job.rb @@ -27,15 +27,12 @@ def self.included(base) if defined?(ActiveSupport) && ActiveSupport.respond_to?(:on_load) ActiveSupport.on_load(:action_mailer) do - # Automatically add to ActionMailer::DeliveryJob - if defined?(ActionMailer::DeliveryJob) + 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 - # Rails >= 6.0 - if defined?(ActionMailer::Base) - ActionMailer::Base.send(:include, Rollbar::ActiveJob) - end end end end From eea33a5ce37bcb9f014b9ebae63cb59ad8703b88 Mon Sep 17 00:00:00 2001 From: Walt Jones Date: Thu, 28 Dec 2023 14:23:24 -0500 Subject: [PATCH 4/7] limit DeliveryJob tests to Rails < 6.x --- spec/rollbar/plugins/active_job_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/rollbar/plugins/active_job_spec.rb b/spec/rollbar/plugins/active_job_spec.rb index 90faabde..cfef11ff 100644 --- a/spec/rollbar/plugins/active_job_spec.rb +++ b/spec/rollbar/plugins/active_job_spec.rb @@ -80,7 +80,7 @@ def perform(exception, job_id) end end - context 'using ActionMailer::DeliveryJob', :if => defined?(ActionMailer::DeliveryJob) do + context 'using ActionMailer::DeliveryJob', :if => Gem::Version.new(Rails.version) < Gem::Version.new('6.0') do include ActiveJob::TestHelper if defined?(ActiveJob::TestHelper) class TestMailer < ActionMailer::Base From 2dcaa5ac5e671ffe3c2a15850cbc92351e3feaf8 Mon Sep 17 00:00:00 2001 From: Walt Jones Date: Tue, 2 Jan 2024 13:57:03 -0500 Subject: [PATCH 5/7] update test app config for rails 6+ --- spec/dummyapp/config/application.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/dummyapp/config/application.rb b/spec/dummyapp/config/application.rb index 172d2eb2..64c5b488 100644 --- a/spec/dummyapp/config/application.rb +++ b/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 From ea734f7cda10cd10146ed6c6c5d160ba2fd9869f Mon Sep 17 00:00:00 2001 From: Walt Jones Date: Tue, 2 Jan 2024 13:58:59 -0500 Subject: [PATCH 6/7] handle undefined job_id --- lib/rollbar/plugins/active_job.rb | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/rollbar/plugins/active_job.rb b/lib/rollbar/plugins/active_job.rb index ced79aef..a9dc3d7d 100644 --- a/lib/rollbar/plugins/active_job.rb +++ b/lib/rollbar/plugins/active_job.rb @@ -14,11 +14,16 @@ def self.included(base) arguments end - Rollbar.error(exception, - :job => self.class.name, - :job_id => job_id, - :use_exception_level_filters => true, - :arguments => args) + 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 From 752d68b7ba8221bfe72f3a696c8f438159580ad5 Mon Sep 17 00:00:00 2001 From: Walt Jones Date: Tue, 2 Jan 2024 13:59:48 -0500 Subject: [PATCH 7/7] refactor to test both DeliveryJob and MailDeliveryJob --- spec/rollbar/plugins/active_job_spec.rb | 131 +++++++++++++++--------- 1 file changed, 81 insertions(+), 50 deletions(-) diff --git a/spec/rollbar/plugins/active_job_spec.rb b/spec/rollbar/plugins/active_job_spec.rb index cfef11ff..6e62a6f7 100644 --- a/spec/rollbar/plugins/active_job_spec.rb +++ b/spec/rollbar/plugins/active_job_spec.rb @@ -22,84 +22,86 @@ 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 + 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 - it 'scrubs all arguments if job has `log_arguments` disabled' do - allow(TestJob).to receive(:log_arguments?).and_return(false) + 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 + 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 - end - 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 } + it 'job is created' do + ActiveJob::Base.queue_adapter = :test + expect do + TestMailer.test_email(argument).deliver_later + end.to have_enqueued_job.on_queue('mailers') end - it "isn't loaded" do - expect(subject.loaded).to eq(false) + 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) - class TestMailer < ActionMailer::Base - attr_accessor :arguments + it_behaves_like 'an ActiveMailer plugin' - def test_email(*_arguments) - error = StandardError.new('oh no') - raise(error) + 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 'job is created' do - ActiveJob::Base.queue_adapter = :test - expect do - TestMailer.test_email(argument).deliver_later - end.to have_enqueued_job.on_queue('mailers') - end - - it 'reports the error to Rollbar' do + it 'reports the mailer error to Rollbar' do expected_params = { :job => 'ActionMailer::DeliveryJob', :use_exception_level_filters => true, @@ -151,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