rails: index_errors does not consider indexes of correct existing sub records

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.

https://github.com/rails/rails/blob/4174d04f5bfaa451e150b8f97d6050dcfc51233d/activerecord/lib/active_record/autosave_association.rb#L337

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 🙂

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.

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.

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.

https://github.com/rails/rails/blob/4174d04f5bfaa451e150b8f97d6050dcfc51233d/activerecord/lib/active_record/autosave_association.rb#L337

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 🙂

Hi everyone, we in GitLab faced this issue and have created a new fix https://github.com/rails/rails/pull/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!

I rewrote @AkshayGoyal022 reindex_nested_error method to work in Rails 6. See how to use it in his comment here: https://github.com/rails/rails/issues/24390#issuecomment-483610929

  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

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!

@maclover7 I am willing to give this a try

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