Using previously changed attributes with `after_commit`

I don’t know if I consider this an bug in Rails, but at least a gotcha. Wanted to post about it to make sure I’m not just missing something and see if there is any interest in making this less of a gotcha in Rails and what that might look like.

My gotcha is when using the “dirty” ActiveRecord module with an after_commit hook. It is not reliable. If the model is only saved once it works just fine (so most of the time). But there is another save within the same transaction to that model (which can happen via subtle way with other AR hooks) the previously changed attributes are overwritten in the second save so when the after_commit runs it doesn’t see any of the changes from the first save.

I put together a quick test file to demonstrate this. The first test passes just fine. The second test does not pass and this might be a surprise is more subtle code.

#!/usr/bin/env ruby

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'activerecord'
  gem 'sqlite3'
end

require "minitest/autorun"
require 'active_record'

ActiveRecord::Base.establish_connection adapter: 'sqlite3', database: 'test.db'

ActiveRecord::Base.connection.tap do |conn|
  conn.create_table :widgets, force: true do |t|
    t.string :fld1, :fld2
    t.boolean :hook_run, null: false, default: false
  end
end

class Widget < ActiveRecord::Base
  after_commit(if: :fld1_previously_changed?) { self.hook_run = true }
end

describe 'Changed attributes in after commit hook' do
  subject { Widget.new fld1: 'val1' }

  it 'provides correct changed attributes after single save in after_commit' do
    subject.transaction do
      subject.save!
    end

    expect( subject.hook_run ).must_equal true
  end

  it 'provides correct changed attributes after a second save in the same txn' do
    subject.transaction do
      subject.save!
      subject.update! fld2: 'val2'
    end

    expect( subject.hook_run ).must_equal true
  end
end

What, if anything, should the Rails framework do to help with this gotcha?

One solution would be to merge the previously_changed attributes together for each additional saves until the transaction is finally committed. I lean away from this solution as it really changes the semantics of the “dirty” module. While it might eliminate this surprise it could lead to other surprises.

Another solution might just be to issue a warning to the logger if the “dirty” module is used within the after_commit hooks being run. This is the solution I like the best as it warns the developer that the dirty module is unreliable within an after_commit hook but it doesn’t stop them from using it.

If you are looking to a work-around to this problem the best solution I have found is to use a combination of hooks and instance variables:

class Widget < ActiveRecord::Base
  before_save(if: :fld1_changed?) { @fld1_updated = true }
  after_commit if: -> { @fld1_updated } do
    self.hook_run = true
    remove_instance_variable :@fld1_updated
  end
end

Curious what others think. Am I missing something? Should this be addressed by the Rails framework? If so how?

Isn’t the easiest answer to not call save multiple times on the same object in a transaction? Build up your attributes/changes and call save once at the end of your action/service/whatever. :man_shrugging:

The code above is a demonstration of the problem by reducing it to just the core issue. In an actual application I wouldn’t have two saves next to each other in the code.

Where I have seen this crop us in a real-world application is usually AR callbacks. You save a single object. But that single object has callbacks triggers activity on another object. That other object itself has callbacks that end up updating the original object.

While in theory it would make sense that developers dont use multiple saves colocated in a transactions, practically our organization is experiencing issues with this. In complicated flows, say where module A is calling B in transaction and B is calling in transaction C, there could be save! or update! calls in module C whose changes to previous_changes are getting overwritten in module A.

Regardless, from the perspective of a developer, if Im writing an after_commit where I want the logic to be conditional on what the changes were to the record were, my expectation is that previous_changes shows me all the changes that were committed to the record. Im trying to put myself in the shoes of someone writing code where they would expect it to be overridden after every save inside a transaction block, but that just doesnt make sense to me. Outside a block it would make sense, but the record would be committed on each save anyway.

I can think of one potential way previous_changes can be used that makes me bit hesitant to recommend modifying that method, but in my opinion rails needs another method that should be used in these situations, and possibly a rubocop to avoid these silent bugs using previous_changes in after_commits.

Very curious to hear thoughts, and/or point me to the right modules to edit in order to propose this feature in the repo

Seeing this same bug ourselves due to a double-save condition around associations with inverse_of on the association. We’re doing some pretty convoluted after_save callbacks to store a potential change to fields we want to trigger a background job.

My organization shares the same issues cited by @vaidarnav.

One solution is implementing an ActiveModel Concern that provides committed_change_to_*? methods, which mirror ActiveModel::Dirty’s saved_change_to_*?, but allow for multiple saves within a transaction – here’s a simple implementation:

# When a record is saved multiple times within a single transaction, methods
# like saved_change_to_*? will behave unexpectedly within after_commit
# callbacks, always returning false. To avoid this, add custom
# committed_change_to_*? mutation trackers that mirror the behavior of
# saved_change_to_*? but allow for multiple saves within a transaction.
module MutationTracker

  extend ActiveSupport::Concern

  included do
    # == Callbacks =====================================================================

    after_save :track_mutations

    # == Instance Methods ==============================================================

    def committed_change_to_attribute?(attr_name)
      return true if self.saved_change_to_attribute?(attr_name)
      return false if @mutation_tracker.blank?

      @mutation_tracker.include?(attr_name.to_s)
    end

    self.attribute_names.each do |attr_name|
      define_method(:"committed_change_to_#{attr_name}?") do
        return self.committed_change_to_attribute?(attr_name)
      end
    end

    protected

    def track_mutations
      @mutation_tracker ||= Set.new
      @mutation_tracker += self.saved_changes.keys
    end
  end

end

In terms of Rails contributions, I think it’s safest not to change existing functionality, but to add new methods similar to the above.