Fix validation error message for autosave_association

[tijwelch]

Summary

Bug: when updating multiple sub records through an autosave_association, index_errors only considers indexes of changed entries which can cause an incorrect error message.

Reproduction test:
https://gist.github.com/tijwelch/c8438775011b20deb4a5bfaaaa43f2e1

Fixes #24390

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[vipulnsward]

cc @eileencodes

[gmrash]

We would be grateful if you speed up the adoption of this pull request.

[eileencodes]

Why did you reorder these models?

[tijwelch]

No reason other than to order alphabetically for readability. Doesn't add any value to this pull request though. Want me to change the order back to how it was?

[tijwelch]

Would you like me to revert the reordering?

[justedd]

Changes that are cosmetic in nature and do not add anything substantial to the stability, functionality, or testability of Rails will generally not be accepted

from here

[tijwelch]

This has been updated.

[justedd]

and so, why this not approved?

[schneems]

r? @sgrif

[tijwelch]

any update on getting this merged?

[katsuya94]

I'd like to bump this. If this is completed it would be very helpful.

[katsuya94]

@schneems it looks like your assignee hasn't picked up reviewing this.

[sgrif]

Hello. There are 672 open pull requests that we have to process. Your patience is appreciated.

[eugeneius]

Here's how this feature is documented in the Configuring Rails Applications guide:

config.active_record.index_nested_attribute_errors allows errors for nested has_many relationships to be displayed with an index as well as the error. Defaults to false.

It's not totally clear what "displayed with an index" means, but based on the discussion in the pull request that originally proposed the feature (#8638) it seems like the index should point to the location of the error in the changes that were applied, not the records in the association.

If that's the case, then this patch would break some operations that currently work correctly, like adding a new record:

  def test_create_invalid_entry
    @order.update(entries_attributes: [
      { title: "" }
    ])
    assert_equal 1, @order.errors.count
    assert_equal "entries[0].title", @order.errors.messages.keys.first.to_s
  end

Or updating an existing record:

  def test_update_invalid_entry
    @order.update(entries_attributes: [
      { id: @entry_2.id, title: "" }
    ])
    assert_equal 1, @order.errors.count
    assert_equal "entries[0].title", @order.errors.messages.keys.first.to_s
  end

[TigerWolf]

I came across this today when building an application with a JSON API in rails. My application will update the nested attributes by posting the entire association to the controller and then returning these validation errors in JSON. If the existing objects are not modified then the validation error will return for the wrong row.

I can either fix these issue by forcing and update of each row or implementing this fix as a monkey patch. I have currently chosen the monkey patch.

I would be great if I could remove this if this merge request was implemented.

[freysie]

Maybe what this PR describes could be enabled by setting index_nested_attribute_errors to something like :association_order, while retaining the current behavior when set to true?

[lynndylanhurley]

@TigerWolf - what did your monkey patch look like?

[lynndylanhurley]

Justed added this to my project and now everything is working as expected:

# workaround for rails bug with association indices.
# see https://github.com/rails/rails/pull/24728
module ActiveRecord
  module AutosaveAssociation
    # Returns the record for an association collection that should be validated
    # or saved. If +autosave+ is +false+ only new records will be returned,
    # unless the parent is/was a new record itself.
    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
  end
end

Thanks @tijwelch!!

[TigerWolf]

This was what I used:

module ActiveRecord
  module AutosaveAssociation
    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
  end
end

I think its the same as what you have.

[markedmondson]

There's another potential pitfall of this that I just ran into. If this validation is being done through a accepts_nested_attributes_for and there's a reject_if block on it, the index doesn't count any rejected values.

Consider the following (very rough) example:

class Question < ApplicationRecord
   has_many :answers

   accepts_nested_attributes_for :answers, reject_if: lambda { |attributes| attributes["value"].blank? }
...
end

class Answer < ApplicationRecord
   belongs_to :question

   validates :value, numericality: { only_integer: true }, if: -> { question.type == 'integer' }
...
end

attributes = {
  "answers_attributes" => {
    "0" => {
      "value" => "",
      "question_id" => <text question uuid>
    },
    "1" => {
      "value" => "text",
      "question_id" => <integer question uuid>
    },
  }
}
@integer_question.update_attributes(questions_attributes: attributes)
@integer_question.valid?
@integer_question.errors => {:"questions[0].answers.value"=>["must be an integer"]}

Since the first blank answer (for a text type question) is rejected in the reject_if block, the index gets shifted up by one. The second answer fails as it's not an integer.

This makes it very tricky to highlight the erroneous field back in a view.

[simonoff]

@sgrif any updates for this issue?

[dwieringa]

In my case I have a Rails + Vue.js app. When I save, I was counting on the indexes to tell me which table row has the error. This works great as long as all rows get validated, but in the case where only some rows are validated the indexes point to the "wrong" rows.

I'm actually pushing all of the rows back to Rails via ajax. Rails decides which ones need to be saved/validated, so I'm not sure how I would decipher the indexes upon response.

[tijwelch]

Closing this PR since it's been sitting here for over 2 years. Can reopen if need be.

[vddgil]

The bug is still in the 5.2 version and @lynndylanhurley fix is still working
Maybe someone should reopen this PR ?

[cyrusstoller]

Looking forward to this PR getting merged. Appreciate you guys working on this.

[nicklozon]

🎉 🎈 Happy Third Birthday #24728! 🎈 🎉

[night91]

I ran into this, but I think this solution will have performance issues, because will validate all unchanged associated records.

[teekenl]

any solution on this?

[marvinthepa]

I think this solution will have performance issues, because will validate all unchanged associated records.

It is easy to limit this performance drawback to only the apps/associations that have error indices enabled...

However, this does not address the reject_if issue mentioned above..

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[askrynnikov]

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 24390#issuecomment-944860713