rails: Autosaving associations twice in Rails 6.0.3 violates uniqueness

Thinking this through a bit more, I think we would ideally be able to handle all of these edge cases but since the autosave and callback system relies on instance variables on the model instance, we’re probably going to end up playing a long winded game of whack of mole for all eternity as these callbacks can logically collide with one another in many ways (since they work on the instance variables on the same instance and assume some level of isolation).

Therefore, I’d propose:

1. Revert the bug fix I originally submitted
2. Change it to a warning:

index 96a3effcf9..56f32f91bb 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -365,7 +365,10 @@ def normalize_reflection_attribute(indexed_attribute, reflection, index, attribu
       # Is used as a before_save callback to check while saving a collection
       # association whether or not the parent was a new record before saving.
       def before_save_collection_association
-        @new_record_before_save ||= new_record?
+        if defined?(@new_record_before_save)
+          logger.warn "Calling a method that emits callbacks in a callback is not supported. Please blah blah change blah blah ... doc link"
+        end
+        @new_record_before_save = new_record?
       end

1. Keep the warning in the ActiveRecord docs
2. In Rails 6.1, change it to a raise ActiveRecord::NestedCallback error (or similar)

This will make sure that we help people remove problemsome code (nested callbacks) while giving them time to act on the warnings.

Any update on this? There were security updates bundled into 6.0.3.1 that I can’t update to because of this issue. Any chance of pushing forward @jules2689 proposition?

I understand that we should be trying to remove nested callbacks, but when dealing with large apps, it is very difficult to track them all down. I have tried deploying the update a couple of times, every time running into this problem in a subtly new scenario.

We really shouldn’t have to be playing games with our apps’ existing functionality and security so I humbly request that this be pushed out with urgency.

Thanks for the reproduction steps! Both bisect to https://github.com/rails/rails/pull/38166. I’ll take a closer look later this evening.

I believe https://github.com/rails/rails/pull/39771 fixes the problem reported here. @geoffharcourt (or anyone else affected): would you mind testing it against your application? https://github.com/eugeneius/rails/tree/6_0_stable_autosave_exactly_once is a branch containing that commit on top of 6-0-stable, in case that makes it easier.

@danielricecodes: https://github.com/rails/rails/pull/38166 wasn’t backported to Rails 5.2, so I don’t think you’re seeing the same problem as the other reports here, despite the similar-looking error. Rails 5.2 is no longer supported for bug fixes, but if you can reproduce your problem in a sample application or an executable test case that uses the 6-0-stable or master branch, please do open a new issue.

👍 not breaking existing apps on a minor update but giving a warning instead, so users can fix it until 6.1, is the best approach imo.

Hi all - bumping the request here. Since ‘callbacks within callbacks’ is part of the public API (even if it’s heavily discouraged) it’s really frustrating that this occurred during a patch update. Are there are any updates on when this might be fixed? Like @hashwin mentions above, having to choose between security compliance and existing functionality is not a fun place to suddenly be.

This is being caused by a documented limitation of the Rails system. On associations, there exist 2 callbacks in ActiveRecord for associations: https://github.com/rails/rails/blob/0571da95934ff8004e0116cf6a91d9182707fb23/activerecord/lib/active_record/autosave_association.rb#L367-L373

What this means is that before_save we tell ActiveRecord that we are a new record (this means we should create the association), and in the after_save we reset that to false.

after_create happens before after_save, so we aren’t set to false yet by after_save_collection_association. By defining a method that updates the model with callbacks (e.g. update, save, save!, etc) you are invoking the callbacks with @new_record_before_save set to true still. This will save the record twice.

This is why @yosiat had luck using after_commit because that happens after after_save.

There is a warning in the ActiveRecord documentation that states:

Care should be taken in callbacks to avoid updating attributes. For example, avoid running update(attribute: "value") and similar code (like saves) during callbacks. This can alter the state of the model and may result in unexpected side effects during commit. Instead, you should try to assign values in the before_create or earlier callbacks.

While this probably shouldn’t cause issues, it is still not advisable to run methods with callbacks in callbacks.

In the bug report here (https://github.com/rails/rails/issues/39173#issuecomment-625178248), the after_create should be changed to a before_save or before_validation to be something like self.last_post_creation_time = Time.now and let the normal save callbacks work (bonus is a saved SQL query!)

Unfortunately undoing some recent changes re-introduces a different bug of a similar nature that causes children records to not be saved if added to an unsaved parent that is subsequently saved (e.g. comment = Comment.create!; post = Post.new(comments: [comment]); post.save; # comment does not have "post_id" assigned).

I’ll leave the ultimate decision of which bug is worse up to the Rails team, but this could be fixed with this diff, which would re-introduce the bug that introduced this one 🙃

index 96a3effcf9..84274bef8a 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -365,7 +365,7 @@ def normalize_reflection_attribute(indexed_attribute, reflection, index, attribu
       # Is used as a before_save callback to check while saving a collection
       # association whether or not the parent was a new record before saving.
       def before_save_collection_association
-        @new_record_before_save ||= new_record?
+        @new_record_before_save = new_record?
       end
 
       def after_save_collection_association

Hello, I’m experiencing this issue after upgrading to Rails 6.1.0-alpha. Has this fix been reverted?

@eugeneius I confirmed that your fix resolves the issue for us where it fails in 6.0.3.

@eugeneius can confirm that branch works for my test cases 👍

hi @adipasquale !

I am seeing the same issue in my rails app, when upgrading to rails 6.0.3, If I do an extra save! in after_create I will get the same record being written twice. But If I change the after_create to be after_commit on create it will solve the issue for me.

Wrote reproduction script with failing test on rails 6.0.3 but passing on 6.0.2:

# frozen_string_literal: true

require "bundler/inline"

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

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

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "6.0.3"
  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 :users, force: true do |t|
  end

  create_table :posts, force: true do |t|
    t.timestamp :last_post_creation_time
  end

  create_join_table :posts, :users
end

class User < ActiveRecord::Base
  has_and_belongs_to_many :posts
end

class Post < ActiveRecord::Base
  has_and_belongs_to_many :users

  after_create :post_process

  def post_process
    self.last_post_creation_time = Time.now
    self.save!
  end
end

class BugTest < Minitest::Test
  def test_association_stuff
    user = User.create!

    post = Post.create(
      users: [user]
    )

    user.reload
    post.reload

    assert_equal [user.id], dbs.users.ids
    assert_equal [post.id], user.posts.ids
  end
end

@eugeneius can you take a look?

No, the fix is still present on the master branch. Please open a new issue with an executable test case.

Hi @eugeneius I haven’t been at a computer in a day, but I should be able to test this later today. Thank you for your work to help resolve!

@maxvlc for my application, changing to after_commit on: :create fixed the issue. You can try this on your app, it might work for you use case.

I’ve tried reproducing with your script by switching to pg, and using something close to my case (setting team_ids instead of teams) but it still does not reproduce 😕 I’ll let you know if I manage to reproduce.