Merge pull request #717 from bugsnag/active-record-integration

Avoid rescuing from errors in Active Record transaction callbacks in versions of Rails where they will be re-raised

CHANGELOG.md

@@ -1,6 +1,14 @@
 Changelog
 =========
 
+## TBD
+
+### Fixes
+
+* Avoid rescuing from errors in Active Record transaction callbacks in versions of Rails where they will be re-raised
+  | [#709](https://github.com/bugsnag/bugsnag-ruby/pull/709)
+  | [apalmblad](https://github.com/apalmblad)
+
 ## v6.24.1 (30 November 2021)
 
 ### Fixes

features/fixtures/rails3/app/app/controllers/unhandled_controller.rb

@@ -8,4 +8,21 @@ def index
   def error
     generate_unhandled_error
   end
+
+  def error_in_active_record_callback
+    User.class_eval do
+      after_commit { raise 'Oh no!' }
+    end
+
+    user = User.new({
+      email: "uuu3@example.com",
+      name: "User U. User III",
+      first_name: "User",
+      last_name: "User"
+    })
+
+    user.save
+
+    render json: {}
+  end
 end

features/fixtures/rails4/app/app/controllers/unhandled_controller.rb

@@ -8,4 +8,22 @@ def index
   def error
     generate_unhandled_error
   end
+
+  def error_in_active_record_callback
+    User.class_eval do
+      after_commit { raise 'Oh no!' }
+    end
+
+    user = User.new({
+      email: "uuu3@example.com",
+      password: "*******",
+      name: "User U. User III",
+      first_name: "User",
+      last_name: "User"
+    })
+
+    user.save
+
+    render json: {}
+  end
 end

features/fixtures/rails4/app/config/application.rb

@@ -19,5 +19,11 @@ class Application < Rails::Application
     # The default locale is :en and all translations from config/locales/*.rb,yml are auto loaded.
     # config.i18n.load_path += Dir[Rails.root.join('my', 'locales', '*.{rb,yml}').to_s]
     # config.i18n.default_locale = :de
+
+    raise_in_transactional_callbacks = ActiveRecord::Type::Boolean.new.type_cast_from_user(
+      ENV.fetch("RAISE_IN_TRANSACTIONAL_CALLBACKS", "true")
+    )
+
+    config.active_record.raise_in_transactional_callbacks = raise_in_transactional_callbacks
   end
 end

features/fixtures/rails5/app/app/controllers/unhandled_controller.rb

@@ -4,4 +4,22 @@ class UnhandledController < ActionController::Base
   def error
     generate_unhandled_error
   end
+
+  def error_in_active_record_callback
+    User.class_eval do
+      after_commit { raise 'Oh no!' }
+    end
+
+    user = User.new({
+      email: "uuu3@example.com",
+      password: "*******",
+      name: "User U. User III",
+      first_name: "User",
+      last_name: "User"
+    })
+
+    user.save
+
+    render json: {}
+  end
 end

features/fixtures/rails5/app/config/routes.rb

@@ -2,6 +2,7 @@
   get '/', to: 'application#index'
 
   get 'unhandled/error', to: 'unhandled#error'
+  get 'unhandled/error_in_active_record_callback', to: 'unhandled#error_in_active_record_callback'
 
   get 'handled/unthrown', to: 'handled#unthrown'
   get 'handled/thrown', to: 'handled#thrown'

features/fixtures/rails6/app/app/controllers/unhandled_controller.rb

@@ -4,4 +4,22 @@ class UnhandledController < ActionController::Base
   def error
     generate_unhandled_error
   end
+
+  def error_in_active_record_callback
+    User.class_eval do
+      after_commit { raise 'Oh no!' }
+    end
+
+    user = User.new({
+      email: "uuu3@example.com",
+      password: "*******",
+      name: "User U. User III",
+      first_name: "User",
+      last_name: "User"
+    })
+
+    user.save
+
+    render json: {}
+  end
 end

features/fixtures/rails6/app/config/routes.rb

@@ -2,6 +2,7 @@
   get '/', to: 'application#index'
 
   get 'unhandled/error', to: 'unhandled#error'
+  get 'unhandled/error_in_active_record_callback', to: 'unhandled#error_in_active_record_callback'
 
   get 'handled/unthrown', to: 'handled#unthrown'
   get 'handled/thrown', to: 'handled#thrown'

features/rails_features/active_record.feature

@@ -0,0 +1,29 @@
+Feature: Active Record
+
+@rails3 @rails4 @rails5 @rails6
+Scenario: An unhandled error in a transaction callback will be delivered
+  Given I start the rails service
+  When I navigate to the route "/unhandled/error_in_active_record_callback" on the rails app
+  And I wait to receive a request
+  Then the request is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier"
+  And the event "unhandled" is true
+  And the exception "errorClass" equals "RuntimeError"
+  And the exception "message" equals "Oh no!"
+  And the event "app.type" equals "rails"
+  And the event "metaData.request.url" ends with "/unhandled/error_in_active_record_callback"
+  And the event "severity" equals "error"
+
+# The "raise_in_transactional_callbacks" config option only exists in Rails 4.2
+@rails4
+Scenario: An unhandled error in a transaction callback will be delivered when raise in transactional callbacks is false
+  Given I set environment variable "RAISE_IN_TRANSACTIONAL_CALLBACKS" to "false"
+  And I start the rails service
+  When I navigate to the route "/unhandled/error_in_active_record_callback" on the rails app
+  And I wait to receive a request
+  Then the request is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier"
+  And the event "unhandled" is true
+  And the exception "errorClass" equals "RuntimeError"
+  And the exception "message" equals "Oh no!"
+  And the event "app.type" equals "rails"
+  And the event "metaData.request.url" ends with "/unhandled/error_in_active_record_callback"
+  And the event "severity" equals "error"

lib/bugsnag/integrations/railtie.rb

@@ -47,6 +47,29 @@ def event_subscription(event)
       end
     end
 
+    ##
+    # Do we need to rescue (& notify) in Active Record callbacks?
+    #
+    # On Rails versions < 4.2, Rails did not raise errors in AR callbacks
+    # On Rails version 4.2, a config option was added to control this
+    # On Rails version 5.0, the config option was removed and errors in callbacks
+    # always bubble up
+    #
+    # @api private
+    def self.rescue_in_active_record_callbacks?
+      # Rails 5+ will re-raise errors in callbacks, so we don't need to rescue them
+      return false if ::Rails::VERSION::MAJOR > 4
+
+      # before 4.2, errors were always swallowed, so we need to rescue them
+      return true if ::Rails::VERSION::MAJOR < 4
+
+      # a config option was added in 4.2 to control this, but won't exist in 4.0 & 4.1
+      return true unless ActiveRecord::Base.respond_to?(:raise_in_transactional_callbacks)
+
+      # if the config option is false, we need to rescue and notify
+      ActiveRecord::Base.raise_in_transactional_callbacks == false
+    end
+
     rake_tasks do
       require "bugsnag/integrations/rake"
       load "bugsnag/tasks/bugsnag.rake"
@@ -70,8 +93,10 @@ def event_subscription(event)
       end
 
       ActiveSupport.on_load(:active_record) do
-        require "bugsnag/integrations/rails/active_record_rescue"
-        include Bugsnag::Rails::ActiveRecordRescue
+        if Bugsnag::Railtie.rescue_in_active_record_callbacks?
+          require "bugsnag/integrations/rails/active_record_rescue"
+          include Bugsnag::Rails::ActiveRecordRescue
+        end
       end
 
       ActiveSupport.on_load(:active_job) do