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