Fix index_errors and provide :nested_attributes_order mode

[lulalala]

1. Fix index_errors does not consider indexes of correct existing sub records #24390, a bug on indexing association validation errors.
2. Addindex_errors: :nested_attributes_order mode for an alternative ordering of errors.

Motivation / Background

GitLab is using index_errors but has to workaround its bug, which is #24390. That bug is about association validation error having the incorrect index because the indexing can be computed from an incomplete collection of association records. The fix would be to index from the full association collection.

@tijwelch first created #24728, however @markedmondson pointed out that reject_if would not work properly. Also, from the discussions I found that people have two different interpretations of what "indexing" means:

1. to index by association order (based on database order), which is the current behavior
2. to index by nested attributes order (and reject_if is only applicable here)

Those two are conflicting goals. To cater to both of them, this PR also adds a new ordering mode called nested_attributes_order. This mode is more applicable to GitLab's use case, where the frontend could pass nested attributes in arbitrary order, and still want the error index to match such order.

Detail

This Pull Request adds a new class called ActiveRecord::Associations::NestedError, which handles the calculation of index. The original index logic exists in AutosaveAssociation and is moved into this class. Base on index_errors setting, it would choose whether to index based on association.target order or nested attributes order.

For nested attributes order, when nested_attributes are assigned (via a setter like roles_attributes=), the array of the corresponding records will be stored in the association object as nested_attributes_target. The NestedError can then access this array to compute the index. The reject_if would also work since if something is rejected, the nested_attributes_target would have nil in its place as a placeholder, therefore maintaining the overall index.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[lulalala]

Hi @amatsuda may I ask for your review please if you have time? Sorry I don't know the current process as Rails bot doesn't seem to auto assign anymore.

The lint error seems like a false positive.

[stefannibrasil]

@guilleiguaran @rafaelfranca apologies if this message is not okay with you but is there any chance you have some bandwidth to review this PR? This could be a good solution to this bug that has prompted lots of monkey patches. Thank you!

[booch]

This would really solve a problem that a lot of people are having to work around in #24390.

[stefannibrasil]

I talked with Eileen at RubyConf and she mentioned sharing the PR in the discord server. I can do it later, but if anyone can do it before me, that'd be awesome ⭐

[RichardBlair]

Hey team, I'm just cleaning up some code and came across a monkey patch we have for this. I thought I'd take a moment to toss my hat in the "would love this" pile. Hopefully y'all have time to review soon 🙏

[gr8bit]

In a desperate attempt to prevent this important PR from becoming stale: we need the fix in this PR to keep the errors our Rails API returns to the frontend in sync with the actual objects, too. Please consider reviewing and merging it. It will make Rails better. =) Thanks a lot!

[anandbait]

This is much needed feature for getting errors indexed by params we pass.
I am facing issues in persisted nested objects, when we update values, then errors are returned for only changed objects hence it becomes difficult to assign errors on frontend

[gr8bit]

@lulalala, could you please fix this PR to make it easier for the maintainers to merge it? Thanks a lot for everything you put into this already! 👍🏼

[tenderlove]

Would you mind fixing the conflicts please? I think this is fine.

[tenderlove]

@lulalala thank you!

[lulalala]

@tenderlove Thank you! I was going to to check out the CI result in the morning before telling you.

[RichardBlair]

HOLY SHIT! THANK YOU!

THANK YOU @lulalala AND @tenderlove!!!

[stefannibrasil]

thank you @tenderlove for letting me simply drop by and mention this one during Hack Day at RailsConf 2024. It's a RailsConf hack day miracle 🎉

[ponny]

Thank you! 🙏

[lulalala]

Thank you so much @stefannibrasil for bringing this up.