Hi @yosiat ! Thanks for the bug report and for the steps to reproduce it. Please note debug doesn't need comment as a parameter.

def debug
  puts "Comment Ids: in after commit: #{comment_ids.inspect}"
end

I would like to further investigate this and submit a patch to fix it. In the meanwhile you can do instead:

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

    assert_equal [Comment.last.id], post.comment_ids
  end
end

Would that be OK?

@albertoalmagro since lots of places are adding comments, the fix we did is on after_add we call comments.load

btw, I didn't explain my self in issue (I will edit the original issue as well) -
if I remove the reference to comment_ids in after_commit callback it will work properly

@yosiat I have been investigating this further with the following results:

1. The issue isn't related with after_commit. Executing post.comment_ids seems to memoize the result and makes the test fail.

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    post.comment_ids

    comment = Comment.create!
    post.comments << comment

    assert_equal [comment.id], post.comment_ids
  end
end

2. This only happens with has_and_belongs_to_many relations. The reproduction steps are mixing setup for a has_many / belongs_to and a has_and_belongs_to_many relations, the following would be a correct setup according to the guides. However, even with the correct setup, it still fails:

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

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", "5.2.1"
  gem "sqlite3"
end

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

# Ensure backward compatibility with minitest 4.
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

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

  create_table :comments_posts, id: false do |t|
    t.integer :comment_id
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_and_belongs_to_many :comments
end

class Comment < ActiveRecord::Base
  has_and_belongs_to_many :posts
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    post.comment_ids

    comment = Comment.create!
    post.comments << comment

    assert_equal [comment.id], post.comment_ids
  end
end

3. This started failing in Rails 5.0.0 and still fails in Rails 5.2.1. However it was solved for Rails edge here: 19c8071.

Having said this I will like to add a specific test for has_and_belong_to_many relations in order to avoid regressions.

@kamipo is there a plan to backport the commit above for Rails 5 or will it be simply released with Rails 5.2.2?

Thanks!

Sorry, the above mentioned commit isn't correct, will continue investigating later.

@albertoalmagro thanks a lot for investigating this!

We are currently can't upgrade to rails 5.2 because of a bug in rails which is blocking us so we are still waiting for Rails 5.2.2 release.

OK @yosiat I'm back, yes, confirmed: 19c8071 solves your issue. I have tested your reproduction steps against my local branch after reverting 19c8071 and it fails, after re-applying it, it works again, and it isn't included in Rails 5.2.1. For the time being you can try to create a patch with it in your application.

I will submit now a regression test for has_and_belong_to_many to ensure it never happens again 👍

Hope this helps you!

👍 Backported 7794d00.