index_errors does not consider indexes of correct existing sub records

[justedd]

Steps to reproduce

Updating existing record with accept_nested_attributes_for with existing sub-records. (Order - Entries), with index_errors: true.
So, i have existing Order, with 2 existing entries, and try to update it with nested attributes

Expected behavior

Same indexes all times

Actual behavior

If im not changing entry_1, and change entry_2(with incorrect attributes) i got this response:

entries[0].title: ['some errors']

If im changing entry_1(correct attributes) and change entry_2(incorrect attributes) i got response

entries[1].title: ['some errors']

System configuration

Rails 5.0.0.beta3
ruby 2.2.3

[maclover7]

Can you create an executable test script using one of the templates here, that would demonstrate the problem you are seeing?

[justedd]

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rails', github: 'rails/rails'
  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 :orders, force: true  do |t|
  end

  create_table :entries, force: true  do |t|
    t.integer :order_id
    t.string :title
  end
end

class Order < ActiveRecord::Base
  has_many :entries, index_errors: true
  accepts_nested_attributes_for :entries
end

class Entry < ActiveRecord::Base
  belongs_to :order
  validates :title, presence: true
end

class BugTest < Minitest::Test
  def setup
    @order = Order.create!(entries_attributes: [
      { title: 'entry_1' },
      { title: 'entry_2' }])
    @entry_1 = @order.entries.first
    @entry_2 = @order.entries.last
  end

  def test_update_both_entries_with_invalid_second
    @order.update(entries_attributes: [
      { id: @entry_1.id, title: 'entry_1_updted' },
      { id: @entry_2.id, title: '' }])
    assert_equal 1, @order.errors.count
    assert_equal 'entries[1].title', @order.errors.messages.keys.first.to_s
  end

  def test_update_only_second_entry_with_invalid_second
    @order.update(entries_attributes: [
      { id: @entry_1.id, title: 'entry_1' },
      { id: @entry_2.id, title: '' }])
    assert_equal 1, @order.errors.count
    #fails here
    assert_equal 'entries[1].title', @order.errors.messages.keys.first.to_s
    #BugTest#test_update_only_second_entry_with_invalid_second [active_record_master.rb:64]:
    #Expected: "entries[1].title"
    #Actual: "entries[0].title"
  end
end

[maclover7]

Yep, this definitely looks like a bug to me. Would you be willing to investigate, and open up a pull request with a fix?

[xw19]

@maclover7 I am willing to give this a try

[zhufenggood]

Actually errors message is index of association.target.find_all(&:changed_for_autosave?) index, confusing people when reading errors message.

Someone can update active_record/autosave_association.rb to fix this.

[utilum]

I'll try.

[vipulnsward]

Nope, not as of now.If you have a fix(since you worked on reproduction), do send it out.

[tijwelch]

As @zhufenggood mentioned, the error message is using the index of items that pass changed_for_autosave?, which in the case of the provided test is an array of one item so the index of 0 is used.

Changing the method associated_records_to_validate_or_save (line 282) to return the full association.target array instead of only the items that pass changed_for_autosave? gets the provided test to pass.

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/autosave_association.rb#L282

Updated method:

def associated_records_to_validate_or_save(association, new_record, autosave)
  if new_record || autosave
    association && association.target
  else
    association.target.find_all(&:new_record?)
  end
end

I have added this commit to my local branch and the ActiveRecord tests are still passing.

Happy to help out however I can. Is opening a PR with this commit the correct next step? Sorry, just getting into contributing here.

[mrageh]

Is opening a PR with this commit the correct next step?

Yes @tijwelch if that fixes the issue then opening a PR with a reproduction test of the bug and your fix is the next step. In your pr reference this issue please

[AkshayGoyal022]

Is this issue resolved?

[tijwelch]

@AkshayGoyal022 the PR is still open #24728

[AkshayGoyal022]

@tijwelch thanks for the update. Does changing associated_records_to_validate_or_save cause any performance issues or unexpected bugs that i should be aware of? The solution provided by you seems good but i am bit skeptical.

[pacop]

any updates on this? I'm still having the same problem in 5.2

[AkshayGoyal022]

For those who don't want to monkey patch, I have implemented a hack to overcome this in one of my projects.

The idea is frontend needs to send a field (in my case "index") and backend will override the rails index with what was sent from the frontend.

Code looks something like this:

I made two concerns as the code was being used in multiple models.

  # This concern needs to be included in nested models
  included do
    attr_accessor :index # Will be sent by the frontend
    after_validation :append_index_to_errors
  end

  def append_index_to_errors
    if self.errors.messages.present? && self.index.present?
      self.errors.messages[:index] = [self.index]
    end
  end

This below concern is to reindex the errors. This needs to be included in parent models.

included do
    after_validation :reindex_nested_errors
  end

  # This is required because errors indexes gets messed up for nested attributes
  # To handle this issue, frontend will be sending index alongwith the item object
  # We will be sending errors on the same index that frontend sent.
  # https://github.com/rails/rails/issues/24390
  def reindex_nested_errors
    errors_hash = self.errors.messages
    # We are adding index in the nested errors in the items
    # Structure will be like errors_hash = {:"items[0].xyz"=>["must be present"], :"items[0].index"=>[3]}
    arr = errors_hash.collect{|k, v| [k.to_s.split(".").first,v.first] if k.to_s.include?("].index")}.compact

    # arr would be like [["items[0]", 3], ["items[1]", 5]]

    new_hash = {}

    arr.each do |a|
      errors_hash.each do |k,v|
        if k.to_s.include?(a.first)
          error_key_with_correct_index = a.first.split("[").first.to_s + [a.last].to_s
          new_key = k.to_s.gsub(a.first, error_key_with_correct_index)
          new_hash["#{new_key}".to_sym] = v unless new_key.to_s.include?("].index") # we dont want to send the index that we appended in the errors.
          errors_hash.delete(k)
        end
      end
    end
    errors_hash.merge!(new_hash)
  end

This might not be the best solution but it works fine for my case.

[heaven]

Instead of current indexes, I'd prefer a solution above with an _index attribute which could be passed from the frontend, which seems more reliable.

The solution proposed by @tijwelch causes deadlocks in some circumstances.

[aenain]

Just stumbled across this issue and decided to share our way of solving this problem.
We have it in one service for now. It could be moved to a dedicated module / concern if needed.

def assign_association_with_indexed_errors(owner, association_name, collection)
  association_errors = ActiveModel::Errors.new(owner)

  collection.each_with_index do |record, idx|
    next if record.valid?

    record.errors.each do |attribute, error|
      association_errors.add(:"#{association_name}[#{idx}].#{attribute}", error)
    end
  end

  if association_errors.empty?
    owner.public_send(:"#{association_name}=", collection)
    owner.save!
  else
    owner.errors.merge!(association_errors)
  end
end

Usage

class Blog < ApplicationRecord
  has_many :posts
  accepts_nested_attributes_for :posts
end

class Post < ApplicationRecord
  belongs_to :blog
end

posts = params[:posts_attributes].map { |attrs| blog.posts.new(attrs) }
assign_association_with_indexed_errors(blog, :posts, posts)

It works nicely as long as you can ensure that owner is persisted first and collection contains instantiated records.

[antoinematyja]

I just encountered this issue too. I'm a big fan of nested attributes and was surprised to see that something was broken and has not been fixed for 4 years.

I tried the monkey patch above, changing associated_records_to_validate_or_save to return all records, but it was breaking my specs: some callbacks on nested records were called when they should not.

So I tried to fix it myself with another approach. I'm not very familiar with the existing code so this might be a wrong approach too but here it is.

rails/activerecord/lib/active_record/autosave_association.rb

Line 337 in 4174d04

records.each_with_index { |record, index| association_valid?(reflection, record, index) }

The (wrong) index is created here: the records array is created via associated_records_to_validate_or_save which returns the subset of records of the association which have changed. Then it iterates with each_with_index, which provides a "relative" index (we want an "absolute" index).

So my solution is to replace this index with the "absolute" index. First I get all the records of the association (whether or not it changed):
all_records = association.target.find_all
Then for each record (each_with_index is not required anymore), I get the "absolute" index with
index = all_records.find_index(record) and provide it to association_valid?.

Here is the full monkey patch:

module ActiveRecord
  module AutosaveAssociation
    def validate_collection_association(reflection)
      if association = association_instance_get(reflection.name)
        if records = associated_records_to_validate_or_save(association, new_record?, reflection.options[:autosave])
          all_records = association.target.find_all
          records.each do |record|
            index = all_records.find_index(record)
            association_valid?(reflection, record, index)
          end
        end
      end
    end
  end
end

With this, my specs are good and the right index is returned. It looks like the index param is not used anywhere else but to generate errors index.

I will try to submit a PR to get reviews from other contributors and eventually get this in Rails 🙂

[cnhuye]

Thanks! You saved my life.

I can not imagine it's near 2021, this bug still exist in RAILS6.

And associated_records_to_validate_or_save fixing method gave me a lot BUGs in after_save callbacks.

Your patch works pretty fine for me!

I just encountered this issue too. I'm a big fan of nested attributes and was surprised to see that something was broken and has not been fixed for 4 years.

I tried the monkey patch above, changing associated_records_to_validate_or_save to return all records, but it was breaking my specs: some callbacks on nested records were called when they should not.

So I tried to fix it myself with another approach. I'm not very familiar with the existing code so this might be a wrong approach too but here it is.

rails/activerecord/lib/active_record/autosave_association.rb

Line 337 in 4174d04

records.each_with_index { |record, index| association_valid?(reflection, record, index) }

The (wrong) index is created here: the records array is created via associated_records_to_validate_or_save which returns the subset of records of the association which have changed. Then it iterates with each_with_index, which provides a "relative" index (we want an "absolute" index).

So my solution is to replace this index with the "absolute" index. First I get all the records of the association (whether or not it changed):
all_records = association.target.find_all
Then for each record (each_with_index is not required anymore), I get the "absolute" index with
index = all_records.find_index(record) and provide it to association_valid?.

Here is the full monkey patch:

module ActiveRecord
  module AutosaveAssociation
    def validate_collection_association(reflection)
      if association = association_instance_get(reflection.name)
        if records = associated_records_to_validate_or_save(association, new_record?, reflection.options[:autosave])
          all_records = association.target.find_all
          records.each do |record|
            index = all_records.find_index(record)
            association_valid?(reflection, record, index)
          end
        end
      end
    end
  end
end

With this, my specs are good and the right index is returned. It looks like the index param is not used anywhere else but to generate errors index.

I will try to submit a PR to get reviews from other contributors and eventually get this in Rails 🙂

[javinto]

In your pull request, could you please add the index_errors option to the documentation of the has_many association, and may be refer to it in the accepts_nested_attributes_for method? The option is nowhere documented accept for the Rails 5.0 update readme. Would be great!

[askrynnikov]

Unfortunately @antoinematyja's solution doesn't work if reject_if block is used, @markedmondson wrote about this problem
24728#issuecomment-321361869.

@AkshayGoyal022's solution in RAILS6 doesn't work

The solution is to store the index of the original collection of nested attributes. As it seems to me, the easiest way to do this is by adding a service field (_nested_attributes_index) to the checked record

module ActiveRecord
  module AutosaveAssociation
    private

    def validate_collection_association(reflection)
      if association = association_instance_get(reflection.name)
        if records = associated_records_to_validate_or_save(association, new_record?, reflection.options[:autosave])
          # #24390 https://github.com/rails/rails/issues/24390 ++++++++++++++++++++++++++++++++++++++++++++
          # records.each_with_index { |record, index| association_valid?(reflection, record, index) }
          if reflection.klass.attribute_method?( :_nested_attributes_index)
            records.each { |record| association_valid?(reflection, record, record._nested_attributes_index) }
          else
            records.each_with_index { |record, index| association_valid?(reflection, record, index) }
          end
          #  #24390 ---------------------------------------------------------------------------------------
        end
      end
    end
  end

  module NestedAttributes # :nodoc:
    private

    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
      # #24390 https://github.com/rails/rails/issues/24390 ++++++++++++++++++++++++++++++++++
      # attributes_collection.each do |attributes|
      association.reflection.klass.module_eval { attr_accessor :_nested_attributes_index }
      attributes_collection.each_with_index do |attributes, index|
        #  #24390 ---------------------------------------------------------------------------
        if attributes.respond_to?(:permitted?)
          attributes = attributes.to_h
        end
        # #24390 https://github.com/rails/rails/issues/24390 ++++++++++++++++++++++++++++++++
        attributes.merge!({ _nested_attributes_index: index })
        # #24390 ----------------------------------------------------------------------------
        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
  end
end

[rikardwissing]

I rewrote @AkshayGoyal022 reindex_nested_error method to work in Rails 6.
See how to use it in his comment here: #24390 (comment)

  def reindex_nested_errors
    renaming_hash = {}

    self.errors.errors.dup.each do |error|
      next unless error.class == ActiveModel::NestedError
      next unless error.attribute.to_s.include?("].index")

      old_key_prefix = error.attribute.to_s.split(".").first
      new_index = error.type
      new_key_prefix = "#{old_key_prefix.split("[").first}[#{new_index}]"

      renaming_hash[old_key_prefix] = new_key_prefix

      self.errors.delete(error.attribute)
    end

    self.errors.errors.dup.each do |error|
      next unless error.class == ActiveModel::NestedError

      old_key_prefix = error.attribute.to_s.split(".").first
      new_key_prefix = renaming_hash[old_key_prefix]

      next unless new_key_prefix.present?

      new_attribute = error.attribute.to_s.gsub(old_key_prefix, new_key_prefix)
      new_error = ActiveModel::NestedError.new(error.base, error.inner_error, { attribute: new_attribute.to_sym })

      self.errors.delete(error.attribute)
      self.errors.errors.push(new_error)
    end
  end

Also a small update to append_index_to_errors to clear a deprecation warning

  def append_index_to_errors
    if self.errors.present? && self._index.present?
      self.errors.add(:index, self._index)
    end
  end

[defaye]

@antoinematyja any idea if your fix still works in Rails 7?

[lulalala]

Hi everyone, we in GitLab faced this issue and have created a new fix #48727. It should fix the original issue, while addressing reject_if by offering a new indexing mode. I would like to invite you to try it out and see if there is anything that I've overlooked. Thanks!