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

#update on CPK association requires an explicit reload to work correctly #51231

Open
jarl-dk opened this issue Mar 1, 2024 · 4 comments · May be fixed by #50297
Open

#update on CPK association requires an explicit reload to work correctly #51231

jarl-dk opened this issue Mar 1, 2024 · 4 comments · May be fixed by #50297

Comments

@jarl-dk
Copy link

jarl-dk commented Mar 1, 2024

Steps to reproduce

Save the below script as ar.rb and run

ruby ar.rb
# 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 :books, force: true, primary_key: %i[number] do |t|
    t.integer :number
    t.string :title
    t.integer :main_chapter_number, null: true
  end

  create_table :chapters, force: true, primary_key: %i[book_number number] do |t|
    t.integer :book_number
    t.integer :number
    t.string :title
  end

  add_foreign_key :books, :chapters, column: [:number, :main_chapter_number], primary_key: [:book_number, :number]
end

class Book < ActiveRecord::Base
  self.primary_key = %i[number]
  has_many :chapters,
           foreign_key: :book_number,
           inverse_of: :book

  belongs_to :main_chapter, class_name: "Chapter",
             query_constraints: %i[number main_chapter_number],
             optional: true,
             inverse_of: :book
end

class Chapter < ActiveRecord::Base
  self.primary_key = %i[book_number number]

  belongs_to :book, foreign_key: :book_number, inverse_of: :chapters
end

class BugTest < Minitest::Test
  def test_association_stuff
    book = Book.create!(number: 1, title: "First book")

    book.chapters << Chapter.new(book:, number: 1, title: "Chapter one") << Chapter.new(book:, number: 2, title: "Chapter two")

    assert_equal 2, book.chapters.count
    assert_equal 2, Chapter.count
    assert_equal nil, book.main_chapter_number

    book.update!(main_chapter: book.chapters.first)
    assert_equal 1, book.main_chapter_number

    # book.reload # This is a workaround that will make it work, but it should not be necessary

    book.update!(main_chapter_number: nil) # This one will not update the database.
    assert_equal nil, book.main_chapter_number
  end
end

Expected behavior

I expect

1 runs, 5 assertions, 0 failures, 0 errors, 0 skips

Actual behavior

It fails with

Failure:
BugTest#test_association_stuff [ar.rb:72]:
Expected: nil
  Actual: 1

See comment in script for workaround

System configuration

Rails version: main

Ruby version: 3.2.2

Maybe related issues: #49597

@jarl-dk jarl-dk changed the title #update on CPK association requires a explicit reload to work correctly #update on CPK association requires an explicit reload to work correctly Mar 1, 2024
@jarl-dk
Copy link
Author

jarl-dk commented Mar 4, 2024

cc/ @nvasilevski can you take a look at this?

@iamradioactive
Copy link
Contributor

@jarl-dk , This issue is similar to the one addressed by #50297. The root cause for this one seems to be related to the stale state detection. I tested your script against that branch and can confirm it fixes the issue. You can verify the same using the following.

gem "rails", github: "iamradioactive/rails", branch: "handle_composite_foreign_keys"

Let me know if I missed something.

@jarl-dk
Copy link
Author

jarl-dk commented Mar 5, 2024

I confirm that my script works against that branch.

@jarl-dk jarl-dk closed this as completed Mar 5, 2024
@iamradioactive
Copy link
Contributor

@jarl-dk , Since the PR is not merged yet, I think instead of closing this issue we should keep it open or maybe mark as duplicate. I can go ahead and update the PR description to include that this issue will be fixed as well.

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

Successfully merging a pull request may close this issue.

3 participants