Unexpected behavior with accepts_nested_attributes_for

[djezzzl]

Hi @DmitryTsepelev,

Could you please help with this case?

require 'bundler/inline'

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

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

  gem 'activerecord', '= 7.0.6'
  require "active_record"
  gem 'ar_lazy_preload'
  gem 'sqlite3'
end

require "ar_lazy_preload"

ArLazyPreload.install_hooks

require 'minitest/autorun'
require 'logger'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new($stdout)

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
  end

  create_table :invitations, force: true do |t|
    t.integer :user_id
    t.integer :company_id
  end

  create_table :companies, force: true do |t|
  end
end

class User < ActiveRecord::Base
  has_many :invitations, dependent: :destroy
  has_many :companies, through: :invitations

  accepts_nested_attributes_for :invitations, allow_destroy: true
end

class Invitation < ActiveRecord::Base
  belongs_to :company
  belongs_to :user

  validate :something

  def something
    # do something
    # if we comment out this but keep ArLazyPreload on the test passes.
    user.invitations.each(&:itself)
  end
end

class Company < ActiveRecord::Base
  has_many :invitations, dependent: :destroy
  has_many :users, through: :invitations
end

ArLazyPreload.config.auto_preload = true

class BugTest < ActiveSupport::TestCase
  def test_nested_preloading
    user = User.create
    company = Company.create

    # This test is failing but if we comment out ArLazyPreload.config.auto_preload = true it will pass as expected
    assert_difference "Invitation.count" do
      user.update(invitations_attributes: [{company_id: company.id}])
      # => true regardless of ArLazyPreload mode

      # It is funny that if we do update twice (with ArLazyPreload on), the test passes by creating only single record
      # user.update(invitations_attributes: [{company_id: company.id}])
    end
  end
end

P.S. I apologize for the messy code above, but it should show the problem.

[codeholic]

I can confirm this is a real issue.

[rohangrg]

This just bit us off-guard too. We've added a lot of lazy_preload calls across our multiple projects and this has started getting reported from multiple users.

[jantunes]

I've run into the same issue and it appears the problem is due to the fact that the association.target is being used when detecting records in the nested attributes:

`def assign_nested_attributes_for_collection_association(association_name, attributes_collection) options = nested_attributes_options[association_name] if attributes_collection.respond_to?(:permitted?) attributes_collection = attributes_collection.to_h end

    unless attributes_collection.is_a?(Hash) || attributes_collection.is_a?(Array)
      raise ArgumentError, "Hash or Array expected for attribute #{association_name}, got #{attributes_collection.class.name} (#{attributes_collection.inspect})"
    end

    check_record_limit!(options[:limit], attributes_collection)

    if attributes_collection.is_a? Hash
      keys = attributes_collection.keys
      attributes_collection = if keys.include?("id") || keys.include?(:id)
        [attributes_collection]
      else
        attributes_collection.values
      end
    end

    association = association(association_name)

    existing_records = if association.loaded?
      association.target
    else
      attribute_ids = attributes_collection.filter_map { |a| a["id"] || a[:id] }
      attribute_ids.empty? ? [] : association.scope.where(association.klass.primary_key => attribute_ids)
    end

    attributes_collection.each do |attributes|
      if attributes.respond_to?(:permitted?)
        attributes = attributes.to_h
      end
      attributes = attributes.with_indifferent_access

      if attributes["id"].blank?
        unless reject_new_record?(association_name, attributes)
          association.reader.build(attributes.except(*UNASSIGNABLE_KEYS))
        end
      elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes["id"].to_s }
        unless call_reject_if(association_name, attributes)
          # Make sure we are operating on the actual object which is in the association's
          # proxy_target array (either by finding it, or adding it if not found)
          # Take into account that the proxy_target may have changed due to callbacks
          target_record = association.target.detect { |record| record.id.to_s == attributes["id"].to_s }
          if target_record
            existing_record = target_record
          else
            association.add_to_target(existing_record, skip_callbacks: true)
          end

          assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
        end
      else
        raise_nested_attributes_record_not_found!(association_name, attributes["id"])
      end
    end
  end`

The issue is here:

target_record = association.target.detect { |record| record.id.to_s == attributes["id"].to_s } if target_record existing_record = target_record else association.add_to_target(existing_record, skip_callbacks: true) end

where the association target is being uses to find if there is an existing record to update. I don't exactly know how to fix it as load_target is overriden by the lazy preloaded and in this case it doesn't detect that it needs to load that association.

IMHO this is makes the auto preloading unusable if you use nested attributes.

[zaidshaikh4518]

ar_lazy_preload-2.1.0/lib/ar_lazy_preload/active_record/association.rb

# frozen_string_literal: true

module ArLazyPreload
  # ActiveRecord::Association patch with a hook for lazy preloading
  module Association
    def load_target
      owner.try_preload_lazily(reflection.name)
      super
    end
  end
end

was there any specific reason that we call owner.try_preload_lazily(reflection.name) first and then super? Shouldn't we first let active record to set the target and then we go for preloading?

I tried below and it did solved the problem of accepts_nested_attributes_for

# frozen_string_literal: true

module ArLazyPreload
  # ActiveRecord::Association patch with a hook for lazy preloading
  module Association
    def load_target
      records = super
      owner.try_preload_lazily(reflection.name)
      records
    end
  end
end