composite-primary-keys / composite_primary_keys

Composite Primary Keys support for Active Record
1.03k stars 351 forks source link

AR#preload doesn't work as expected with duplicated ids in 13.0.3 #577

Closed willnet closed 1 year ago

willnet commented 2 years ago

If execute AR#preload method from a model with duplicate ids, the associated model will be incorrect.

This seems to be the problem with ActiveRecord::Associations::Preloader::Association#records_by_owner. https://github.com/composite-primary-keys/composite_primary_keys/blob/721742c60c166f9091b1086fa5af0f41c2806ee7/lib/composite_primary_keys/associations/preloader/association.rb#L36-L48

Rails itself does not have this problem because it uses Hash#compare_by_identity https://github.com/rails/rails/blob/f80179db9082e25f1bceb3bdb9c58ced4ea0e04e/activerecord/lib/active_record/associations/preloader/association.rb#L44

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

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

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

  gem "activerecord", "6.1.6"
  # if comment out following line, the test pass
  gem "composite_primary_keys", "13.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 :parents, force: true do |t|
  end

  create_table :children, id: false, force: true do |t|
    t.bigint :id
    t.bigint :parent_id
  end
end

class Parent < ActiveRecord::Base
end

class Child < ActiveRecord::Base
  belongs_to :parent
end

class BugTest < Minitest::Test
  def test_association_stuff
    p1 = Parent.create!
    p2 = Parent.create!
    p3 = Parent.create!
    Child.create!(id: 100, parent_id: p1.id)
    Child.create!(id: 100, parent_id: p2.id)
    Child.create!(id: 100, parent_id: p3.id)

    children = Child.preload(:parent)
    # if uncomment following line, this test pass
    # children = Child.all
    parents = children.map(&:parent)
    assert_equal [p1.id, p2.id, p3.id], parents.map(&:id)
  end
end

Expected behavior

the test pass

Actual behavior

1) Failure:
BugTest#test_association_stuff [hoge.rb:54]:
Expected: [1, 2, 3]
  Actual: [1, 1, 1]
cfis commented 1 year ago

Sorry to have missed this. I can confirm this does NOT work on CPK version 6.1.x but it does WORK on 7.0.x.

cfis commented 1 year ago

Thanks for the excellent bug report and making it easy to reproduce. Version 13.0.6 fixes the issue.