Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails 6.1 double assignment leads to duplicate in-memory associations #43222

Closed
davidmyersdev opened this issue Sep 15, 2021 · 5 comments
Closed

Comments

@davidmyersdev
Copy link
Contributor

davidmyersdev commented Sep 15, 2021

Steps to reproduce

rails new --api demo

Create the following task at lib/tasks/ar_dup.rake.

# frozen_string_literal: true

desc "test duplication issue"
task ar_dup: :environment do
  require "minitest/autorun"

  ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
  ActiveRecord::Base.logger = Logger.new(STDOUT)

  ActiveRecord::Schema.define do
    create_table :people do |t|
      t.string :name
    end

    create_table :things do |t|
      t.references :person
      t.string :name
    end
  end

  class Person < ActiveRecord::Base
    has_many :things, inverse_of: :person
  end

  class Thing < ActiveRecord::Base
    belongs_to :person, inverse_of: :things
  end

  class BugTest < Minitest::Test
    def test_happy_path
      person = Person.new(name: 'Tiff')
      person.things << Thing.new(person: person)

      assert_equal 1, person.things.length
    end
  end
end

Run the rake task.

bin/rails ar_dup

The test fails, because person.things.length is 2. Oddly enough, I could not reproduce this in a standalone ActiveRecord script. It had to be in a Rails app.

Expected behavior

Only 1 association should be added to person.things.

Actual behavior

The new association is duplicated (same object_ids).

System configuration

Rails version: Rails 6.1.4.1

Ruby version: ruby 2.6.6p146 (2020-03-31 revision 67876) [x86_64-darwin20]

@davidmyersdev
Copy link
Contributor Author

As I mentioned above, this does not happen in a standalone ActiveRecord test. This is the test I wrote to reproduce the bug, but it actually passes (as I would expect the full Rails app to do).

# 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.1.4.1"
  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 :people do |t|
    t.string :name
  end

  create_table :things do |t|
    t.references :person
    t.string :name
  end
end

class Person < ActiveRecord::Base
  has_many :things, inverse_of: :person
end

class Thing < ActiveRecord::Base
  belongs_to :person, inverse_of: :things
end

class BugTest < Minitest::Test
  def test_happy_path
    person = Person.new(name: 'Tiff')
    person.things << Thing.new(person: person)

    assert_equal 1, person.things.length
  end
end

@davidmyersdev
Copy link
Contributor Author

It turns out this actually only happens when using inverse_of on the belongs_to association.

@ghiculescu
Copy link
Member

Rails 7 alpha just launched, could you please check if it is fixed in there?

There were a fair few changes around has_many_inversing in 6.1 and there's probably some bug fixes in 7.

@ClementTsang
Copy link

ClementTsang commented Oct 13, 2021

Tried following the original steps to run the demo rake task with 7.0.0.alpha from main, and it seems to still fail:

# Running:

F

Failure:
BugTest#test_happy_path [/home/spin/demo/lib/tasks/ar_dup.rake:34]:
Expected: 1
  Actual: 2


rails test lib/tasks/ar_dup.rake:30

jstncarvalho added a commit to jstncarvalho/rails that referenced this issue Oct 13, 2021
Fixes a bug that causes duplicate references to the same object to be
added to the collection association on an active_record object when
inverse_of is used.

In cases where has_many_inversing is set to true, the
set_inverse_instance function calls target= on collection_association during concat
resulting in multiple appends to target. This only occurs for new records. This
PR introduces changes that sets the index so the duplicate object
replaces the original.

Closes issue rails#43222.

Co-authored-by: Terence Li <terence.li@shopify.com>
Co-authored-by: Dave Rose <dave.rose@shopify.com>
@jstncarvalho
Copy link
Contributor

Hey @voraciousdev,

#43445 fixed this.

The test was passing with ActiveRecord, because the has_many_inversing config option was not set (in Rails it is). You can get the same behavior by adding ActiveRecord::Base.has_many_inversing = true to the AR test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants