Duplicate objects stored in has many association after save

[Tonkpils]

Steps to reproduce

#42524 seems to have introduced an unintended bug where a duplicate object is left in a has many association when added during a before_save callback.

# 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", 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|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments

  before_save :create_comments
  attr_accessor :do_it

  def create_comments
    if do_it
      self.comments = [comments.build]
    end
  end
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    post.do_it = true
    post.save!

    assert_equal 1, post.comments.size
    assert_equal 1, Comment.count
    assert_equal post.id, Comment.first.post.id
  end
end

/cc @ghiculescu

Expected behavior

post.comments only has one object after being saved

Actual behavior

post.comments contains the same object twice after the before_save callback is complete

System configuration

Rails version: 7.0 main @ #42524

Ruby version: 2.7

[ghiculescu]

Looks like this simpler case has also regressed :(

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    post.comments = [post.comments.build]
    post.save!

    assert_equal 1, post.comments.size
    assert_equal 1, Comment.count
    assert_equal post.id, Comment.first.post.id
  end
end

[ghiculescu]

@Tonkpils why did you not write the code like this?

class Post < ActiveRecord::Base
  has_many :comments

  before_save :create_comments
  attr_accessor :do_it

  def create_comments
    if do_it
      self.comments.build
    end
  end
end

The assertions pass when written that way. I agree it's a regression and am working on a fix, just trying to understand if there's a more complex use case I've missed.

[Tonkpils]

@Tonkpils why did you not write the code like this?

It's a simplified example from the actual code I saw failing. The actual code is doing something more like

self.comments = new_comments.map { |attrs| comments.build(attrs) }

[eileencodes]

why did you not write the code like this?

For some background @ghiculescu - at GitHub (where @Tonkpils and I work) we upgrade the main app to the latest SHA of Rails main weekly which allows us to catch regressions early. Often code that starts failing unexpectedly is code we didn't write and potentially could be 10+ years old. We try to not rewrite other teams code unless it's deprecated or never was supposed to work. So you might sometimes see code from us that could be written differently but did still cause a failure.

[ghiculescu]

Yep that makes sense, sorry I didn't meant to sound accusatory. I just wanted to double check my assumption that post.comments = [post.comments.build] and post.comments.build are supposed to do the same thing, or if there was a reason to write it one way over the other.