ActiveModel::Dirty is broken in after_commit

[kshnurov]

#45280 broke the expected and documented behaviour by introducing a breaking change in Rails 7.1. It leads to hard-to-find silent bugs and should be reverted, there's no workaround except disabling it completely with config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = true. The problem it was trying to solve is well-documented and easily dealt with reload.

Steps to reproduce

Oversimplified example:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails"
  # If you want to test against edge Rails replace the previous line with this:
  # gem "rails", github: "rails/rails", branch: "main"

  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.integer :comments_count, null: false, default: 0
  end

  create_table :comments, force: true do |t|
    t.boolean :hidden, null: false, default: false
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments

  def update_comments_count!
    self.update!(comments_count: comments.size)
  end
end

class Comment < ActiveRecord::Base
  belongs_to :post
  after_commit :update_posts

  def update_posts
    if saved_change_to_post_id?
      post.update_comments_count!
    end
  end
end

class BugTest < Minitest::Test
  def test_association_stuff
    post_1 = Post.create!
    post_1.comments.create!

    assert_equal 1, post_1.comments_count

    post_2 = Post.create!
    comment = post_2.comments.build
    Post.transaction do
      comment.save!
      comment.update!(hidden: true)
    end
    assert_equal 1, post_1.comments_count
    assert_equal 1, post_2.comments_count
  end
end

Expected behavior

saved_change_to_post_id? == true

Actual behavior

saved_change_to_post_id? == false

System configuration

Rails version: 7.1+ Ruby version: 3.2.3

[jreed-mt]

Is this related to #49898 ?

[palexvs]

@jreed-mt no, it's not. The current issue is reproduced on a much simpler configuration with only one model. You only need Rails 7.1 with default configuration + after_commit => changes is empty even there were changes

[tubaxenor]

Having the same issue. Looks like saved_changes should be merged to last saved object when the flag is off in https://github.com/rails/rails/blob/v7.1.3.2/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb#L273

[rails-bot[bot]]

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 7-2-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

[mintyfresh]

Bump, this is absolutely still relevant in 7.2. In fact, config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = true no longer seems to be a functional workaround in 7.2.

[vipulnsward]

Re-opening, as I seem to be seeing the same issue which is blocking one of our App upgrades from 7.0 to 7.1 and looks like there is no confirmed easy work around.

[rails-bot[bot]]

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 8-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

[varvaray]

Up, also, looks like attachment changes have the same issue

...attachment.changes_to_save ...attachment.previous_changes ...attachment.saved_changes

all empty for me on after_commit callback

[mintyfresh]

Still broken in 8.0.1, run_commit_callbacks_on_first_saved_instances_in_transaction does not fix this issue.

Example attached:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails"
  # If you want to test against edge Rails replace the previous line with this:
  # gem "rails", github: "rails/rails", branch: "main"

  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction = false

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.integer :comments_count, null: false, default: 0
  end

  create_table :comments, force: true do |t|
    t.boolean :hidden, null: false, default: false
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments

  def update_comments_count!
    self.update!(comments_count: comments.size)
  end
end

class Comment < ActiveRecord::Base
  belongs_to :post
  after_commit :update_posts

  def update_posts
    if saved_change_to_post_id?
      post.update_comments_count!
    end
  end
end

class BugTest < Minitest::Test
  def test_association_stuff
    post_1 = Post.create!
    post_1.comments.create!

    assert_equal 1, post_1.comments_count

    post_2 = Post.create!
    comment = post_2.comments.build
    Post.transaction do
      comment.save!
      comment.update!(hidden: true)
    end
    assert_equal 1, post_1.comments_count
    assert_equal 1, post_2.comments_count
  end
end

In the example above, setting ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction to false or true or leaving it as its initial value will still all fail the tests.

[gap777]

Up, also, looks like attachment changes have the same issue

...attachment.changes_to_save ...attachment.previous_changes ...attachment.saved_changes

all empty for me on after_commit callback

Ay Caramba!

This just bit me for attachments. Any known work-arounds?

[zzak]

I'm not sure if this helps but removing the comment.update!(hidden: true) the tests pass.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails", "~> 8.0.2"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

# ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction = false
# ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction = true

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.integer :comments_count, null: false, default: 0
  end

  create_table :comments, force: true do |t|
    t.boolean :hidden, null: false, default: false
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments

  def update_comments_count!
    self.update!(comments_count: comments.size)
  end
end

class Comment < ActiveRecord::Base
  belongs_to :post
  after_commit :update_posts

  def update_posts
    if saved_change_to_post_id?
      post.update_comments_count!
    end
  end
end

class BugTest < Minitest::Test
  def test_association_stuff
    post_1 = Post.create!
    post_1.comments.create!

    assert_equal 1, post_1.comments_count

    post_2 = Post.create!
    comment = post_2.comments.build
    Post.transaction do
      comment.save!
      #comment.update!(hidden: true)
    end
    assert_equal 1, post_1.comments_count
    assert_equal 1, post_2.comments_count
  end
end

Otherwise, removing the transaction and leaving the comment.update! passes.

However, the same behavior can be seen on Rails 7.0, which was surprising (given the context).

In any case, with #50011 the test passes, so I rebased that PR but I'm not sure it's correct, so we still need to wait for approval from the team. If you're able to try it out in your apps and let us know how it works in production that would help.

[rails-bot[bot]]

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 8-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

[tkachen]

Still broken in 8.0.2

[rails-bot[bot]]

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 8-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

[gap777]

@tkachen, @mintyfresh (and others) have confirmed this is still broken in Rails 8