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

Incorrectly raising ReadonlyAttributeError when autosaving has_one association with readonly foreign key #50897

Closed
Earlopain opened this issue Jan 27, 2024 · 4 comments · Fixed by #50901

Comments

@Earlopain
Copy link
Contributor

Earlopain commented Jan 27, 2024

Rails 7.1 sets raise_on_assign_to_attr_readonly to true. In the following repro I set a column unrelated to attr_readonly yet the error is still being set. If I stop setting this column the error goes away.

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 "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.raise_on_assign_to_attr_readonly = true

ActiveRecord::Schema.define do
  create_table :forum_topics, force: true do |t|
    t.integer :updater_id
    t.text :title
  end

  create_table :forum_posts, force: true do |t|
    t.integer :updater_id
    t.integer :forum_topic_id
    t.text :body
  end
end

class ForumTopic < ActiveRecord::Base
  has_one :original_forum_post, -> { order("forum_posts.id asc") }, class_name: "ForumPost", inverse_of: :forum_topic
  validates_associated :original_forum_post
  accepts_nested_attributes_for :original_forum_post
end

class ForumPost < ActiveRecord::Base
  attr_readonly :forum_topic_id
  belongs_to :forum_topic

  before_validation do |rec|
    # next # Skipping setting this column makes the error go away
    if persisted?
      rec.updater_id = 456
    else
      rec.updater_id = 123
    end
  end
end

class BugTest < Minitest::Test
  def test_readonly_attr_exception
    topic = ForumTopic.create!(title: "Hi!", original_forum_post_attributes: { body: "What's up?" })
    topic.update(title: "nevermind") # => ActiveRecord::ReadonlyAttributeError: forum_topic_id
    assert_equal(456, topic.original_forum_post.updater_id)
  end
end

System configuration

Rails version: master

Ruby version: 3.2.2

@Earlopain Earlopain changed the title Incorrectly raising ReadonlyAttributeError when that attribute hasn't changed Incorrectly raising ReadonlyAttributeError when changing column before validation in validates_associated Jan 27, 2024
@joshuay03
Copy link
Contributor

joshuay03 commented Jan 28, 2024

This is an interesting one! It looks like because updater_id on the nested record is changing, an autosave is triggered (this is always enabled for nested attributes), causing the foreign key on the nested record to be set here despite it not changing:

record[foreign_key] = _read_attribute(primary_key)

Interestingly, this isn't an issue on belongs_to which already only sets the nested record's foreign key if it's changing:

self[foreign_key] = association_id unless self[foreign_key] == association_id

I've got a fix passing against your script, just adding some coverage before I get a PR up.

@joshuay03
Copy link
Contributor

Got this up: #50901

@Earlopain
Copy link
Contributor Author

Thank you, that looks good and gets all my actual tests passing. This seems to be mirroring what #46759 did for belongs_to, I found #46758 while searching for similar issues but didn't connect the dots.

@Earlopain Earlopain changed the title Incorrectly raising ReadonlyAttributeError when changing column before validation in validates_associated Incorrectly raising ReadonlyAttributeError when autosaving has_one association with readonly foreign key Jan 28, 2024
@joshuay03
Copy link
Contributor

Thank you, that looks good and gets all my actual tests passing. This seems to be mirroring what #46759 did for belongs_to, I found #46758 while searching for similar issues but didn't connect the dots.

Ah nice find, I'll reference it in my PR, thanks!

@skipkayhil skipkayhil added this to the 7.1.4 milestone Jan 28, 2024
joshuay03 added a commit to joshuay03/rails that referenced this issue Jan 28, 2024
joshuay03 added a commit to joshuay03/rails that referenced this issue Jan 28, 2024
joshuay03 added a commit to joshuay03/rails that referenced this issue Jan 30, 2024
joshuay03 added a commit to joshuay03/rails that referenced this issue Feb 7, 2024
joshuay03 added a commit to joshuay03/rails that referenced this issue Feb 7, 2024
joshuay03 added a commit to joshuay03/rails that referenced this issue Feb 7, 2024
joshuay03 added a commit to joshuay03/rails that referenced this issue Feb 7, 2024
joshuay03 added a commit to joshuay03/rails that referenced this issue Feb 21, 2024
rafaelfranca added a commit that referenced this issue Feb 21, 2024
…-fk-when-unchanged

[Fix #50897] Autosaving `has_one` sets foreign key attribute when unchanged
rafaelfranca added a commit that referenced this issue Feb 21, 2024
…-fk-when-unchanged

[Fix #50897] Autosaving `has_one` sets foreign key attribute when unchanged
jathayde pushed a commit to jathayde/rails that referenced this issue Feb 28, 2024
codebender pushed a commit to CirrusMD/rails that referenced this issue Mar 7, 2024
Ridhwana pushed a commit to Ridhwana/rails that referenced this issue Mar 7, 2024
viralpraxis pushed a commit to viralpraxis/rails that referenced this issue Mar 24, 2024
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