-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Order of operations for saving nested associations has changed in Rails v7.2 with automatically_invert_plural_associations enabled #51853
Comments
I updated the reproduction script and OP to reflect what I've learned so far while digging into this issue. I think the underlying issue is a change in order of operations when saving nested associations like this, resulting in foreign keys being null when the value is actually available (i.e. the owner was saved but its primary key value is not propagated down to some associations). |
Originally posted by @zzak in keygen-sh/keygen-api#843 (comment) Commit 68013b3 is the first bad commit via
And here's my #!/usr/local/bin/ruby
require "pathname"
Dir["*/lib/*.rb"].each do |file|
path = Pathname.new(file) # add local rails libs to load path
unless $LOAD_PATH.include?(path.dirname.to_s)
$LOAD_PATH.unshift(path.dirname.to_s)
end
end
require "rails"
require "active_record"
pp(
revision: `git rev-parse HEAD --quiet 2>/dev/null`.chomp,
version: Rails.version,
)
# This connection will do for database-independent bug reports.
ActiveRecord::Base.tap do |config|
config.establish_connection(adapter: "sqlite3", database: ":memory:")
config.logger = Logger.new(STDOUT)
if config.respond_to?(:automatically_invert_plural_associations)
config.automatically_invert_plural_associations = true
end
config.has_many_inversing = true
end
ActiveRecord::Schema.define do
create_table :accounts, force: true do |t|
t.string :name, null: false
end
create_table :packages, force: true do |t|
t.references :account, null: false
end
create_table :releases, force: true do |t|
t.references :account, null: false
t.references :package, null: false
end
create_table :artifacts, force: true do |t|
t.references :account, null: false
t.references :release, null: false
end
end
class Account < ActiveRecord::Base
has_many :packages
has_many :releases
has_many :artifacts
end
class Package < ActiveRecord::Base
belongs_to :account
has_many :releases
validates :account, presence: true
end
class Release < ActiveRecord::Base
belongs_to :account, default: -> { package.account }
belongs_to :package
validates :account, presence: true
validates :package, presence: true
validate do
# Passes:
# errors.add :package unless package.account_id == account.id
# Fails:
errors.add :package unless package.account_id == account_id
end
end
class Artifact < ActiveRecord::Base
belongs_to :account, default: -> { release.account }
belongs_to :release
validates :account, presence: true
validates :release, presence: true
validate do
# Passes:
# errors.add :release unless release.account_id == account.id
# Fails:
errors.add :release unless release.account_id == account_id
end
end
require "minitest/autorun"
require "rack/test"
class AutomaticInverseOfTest < Minitest::Test
def test_automatic_inverse_of
account = Account.new(name: "Test")
package = Package.new(account:)
release = Release.new(account:, package:)
artifact = Artifact.new(account:, release:)
assert_not_raised ActiveRecord::RecordInvalid do
artifact.save!
end
end
private
def assert_not_raised(exception_class, failure_message = nil)
yield
rescue => e
if e.is_a?(exception_class)
flunk(failure_message || "An exception was not expected but was raised: #{e.inspect}")
else
raise
end
end
end I also ran a bisect on Keygen's full test suite and it resulted in the same first bad commit. |
I think there might a bigger Rails issue here. I orginally thought it was related to #51065 but in my investigation it appears Rails is silently skipping records that are invalid and in this test case causing the SQL error down the road. Looking at the test Rails now returns I was able to get to this line that will only return false if
Since
I've created this patch If you want to try it in the test case. This may also affect I think the validation failure should be surfaced even though |
@malomalo I believe all of that is ultimately happening because of a change in order of operations. In the case of the artifact not saving, IIRC, it's because its release is invalid and not saved because its artifacts are invalid (or maybe because like you said, the release's Maybe the autosave association should track whether or not a particular association or record is already being saved, and if so, skip validation in that case. I've tried a few patches, but nothing seems to work 100%. And then you have the foreign key propagation issue where some associations do not get their foreign keys set. Since you mentioned it, I have seen the same behavior for |
@ezekg I think the validation itself is flakey and was working, but Rails knows the validations failed after Rails 7.2 and swallowed it. Rails didn't raise an error when you called Now that may be a separate issue but would have let you find the issue significantly faster and not been silent. I can try an make an example patch next week that would at least raise an error on |
Just FYI, |
@malomalo I think it's deeper than just validation failures, because if you disable |
Steps to reproduce
Full reproduction script: https://gist.github.com/ezekg/df58f1e5d1c5a3786afbe8511913836b
Expected behavior
I would expect the records to be saved successfully, as they are in v7.1. With
automatically_invert_plural_associations
enabled in Rails 7.2, saving the artifact also saves its implicit associations. Before that config was introduced, saving nested has-many/collection associations was always explicit. This is fine and "correct" behavior i.r.t. associations (even if it took a few days to debug). With this change, when the artifact is saved, it cascades down to the account, and saving the account saves its implicit has-many associations as well, which results in the account's artifacts, packages and releases being saved, which seemingly results in some associations being saved out of order (I'm still digging into this so I'm not 100% on this yet).All this results in what is ultimately a change in order of operations when saving nested associations, and this change in order of operations results in some records being saved before they're ready, or without foreign key information they should have through precedingly saved associations, resulting in validation or constraint failures.
I'd expect the records to be saved regardless of any internal change in order of operations.
Actual behavior
I receive an error because the release's
account_id
foreign key is out of sync with its account association's primary key. This shows that a record's implicit foreign keys aren't populated until after the record is saved, even though they may be known via the association's primary key (i.e.association_id
vsassociation.id
).This results in a validation error, causing some nested associations to not be saved (without error), causing null values in the insert query, causing a SQL constraint failure.
If the null constraint on
artifacts.release_id
wasn't there, this would fail silently — inserting only an account and a half-baked artifact, skipping the "invalid" release and package altogether (I'm putting quotations there because it's not actually invalid — it's just "invalid" due to the order of operations changing between v7.1 and v7.2).If you disable
automatically_invert_plural_associations
, the test passes. It also passes on7-1-stable
. In addition, if you update the validations to checkaccount.id
instead ofaccount_id
, the test passes.System configuration
Rails version: 7.2.0-stable branch
Ruby version: 3.2.2
The text was updated successfully, but these errors were encountered: