Order of operations for saving nested associations has changed in Rails v7.2 with automatically_invert_plural_associations enabled about rails HOT 7 OPEN

Just FYI, automatically_invert_plural_associations isn't enabled by default anymore, and it will not be probably until 8.0

from rails.

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).

from rails.

@ezekg Are you willing to try and bisect with your repro script to help identify the cause of the change?

Originally posted by @zzak in keygen-sh/keygen-api#843 (comment)

Commit 68013b3 is the first bad commit via git bisect:

68013b30d2f383177128b356c98ad449dcaf43f7 is the first bad commit
commit 68013b30d2f383177128b356c98ad449dcaf43f7
Author: Sean Doyle <[email protected]>
Date:   Wed Dec 6 11:20:31 2023 -0500

    Infer `:inverse_of` for `has_many ..., through:`

    Closes [#49574][]

    Issue #49574 outlines how an Active Record join model accessed through a
    `has_many ..., through:` association is unable to infer an appropriate
    `:inverse_of` association by pluralizing a predictably pluralizable
    class name.

    This commit resolves that issue by also checking a model's reflections
    for a pluralized inverse name in addition to whatever's provided through
    the `:as` option or inferred in the singular.

    The issue shares a code sample:

    ```ruby
    ActiveRecord::Schema.define do
      create_table "listings", force: :cascade do |t|
        t.bigint "list_id", null: false
        t.bigint "pin_id", null: false
      end

      create_table "lists", force: :cascade do |t|
      end

      create_table "pins", force: :cascade do |t|
      end
    end

    class Pin < ActiveRecord::Base
      has_many :listings
    end

    class List < ActiveRecord::Base
      has_many :listings
      has_many :pins, through: :listings
    end

    class Listing < ActiveRecord::Base
      belongs_to :list
      belongs_to :pin
    end

    class BugTest < Minitest::Test
      def test_association_stuff
        list = List.create!
        pin = list.pins.new

        pin.save!

        assert_equal [pin], list.reload.pins
        assert_equal 1, list.reload.pins.count
      end
    end
    ```

    Unfortunately, there isn't a one-to-one mapping in the test suite's
    `test/model` directory for this type of structure. The most similar
    associations were between the `Book`, `Subscription`, and `Subscriber`.
    For the sake of ease, this commit wraps the test block in a new
    `skipping_validations_for` helper method to ignore validations declared
    on the `Subscription` join table model.

    [#49574]: https://github.com/rails/rails/issues/49574

 activerecord/lib/active_record/reflection.rb                  |  5 +++--
 .../has_one_through_disable_joins_associations_test.rb        |  1 +
 .../test/cases/associations/inverse_associations_test.rb      | 11 +++++++++++
 3 files changed, 15 insertions(+), 2 deletions(-)

And here's my tmp/test script:

#!/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.

from rails.

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 artifact.valid? returns true. However when doing the actual saving after inserting the account the release becomes invalid because the account_id is set on the package but not the release yet.

Rails now returns false because it's invalid and doesn't issue the SQL command... and everything continues like it's okay to continue saving the remaining records.

I was able to get to this line that will only return false if autosaving is turned on:

Since false isn't returned in this test case

I've created this patch If you want to try it in the test case.

This may also affect has_one associations. But it doesn't look like collection associations are affected.

I think the validation failure should be surfaced even though autosave is false. But haven't investigated the ramifications of that.

from rails.

@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 account_id foreign key is nil even though account.id is not nil). It seemingly causes some sort of cyclic validation failure which I haven't been able to fully track down yet, where like you said, some records are silently discarded.

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 has_one associations when running Keygen's test suite, but I haven't been able to put together a failing test like I did for the has_many association.

from rails.

@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 save!, even though the records it was saving failed validation. I think that is wrong and would have been easier for you to figure out by raising the correct error on save! instead of swallowing the error and letting your NOT NULL constraint.

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 save! or add the error on save and return false.

from rails.

@malomalo I think it's deeper than just validation failures, because if you disable automatically_invert_plural_associations, the create! succeeds, i.e. all records are valid and no NOT NULL constraints are raised like when it's enabled. So something isn't right in terms of order-of-operations, because none of these records should actually be invalid.

from rails.