-
Notifications
You must be signed in to change notification settings - Fork 21.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix validation error message for autosave_association #24728
Conversation
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. |
cc @eileencodes |
We would be grateful if you speed up the adoption of this pull request. |
require 'models/member' | ||
require 'models/member_detail' | ||
require 'models/organization' | ||
require 'models/guitar' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you reorder these models?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like me to revert the reordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been updated.
and so, why this not approved? |
r? @sgrif |
any update on getting this merged? |
I'd like to bump this. If this is completed it would be very helpful. |
@schneems it looks like your assignee hasn't picked up reviewing this. |
Hello. There are 672 open pull requests that we have to process. Your patience is appreciated. |
df15c55
to
a26a3a0
Compare
3a8ead2
to
9919757
Compare
9919757
to
c449b28
Compare
Here's how this feature is documented in the Configuring Rails Applications guide:
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 |
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. |
Maybe what this PR describes could be enabled by setting |
@TigerWolf - what did your monkey patch look like? |
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!! |
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. |
There's another potential pitfall of this that I just ran into. If this validation is being done through a 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 This makes it very tricky to highlight the erroneous field back in a view. |
@sgrif any updates for this issue? |
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. |
Closing this PR since it's been sitting here for over 2 years. Can reopen if need be. |
The bug is still in the 5.2 version and @lynndylanhurley fix is still working |
Looking forward to this PR getting merged. Appreciate you guys working on this. |
🎉 🎈 Happy Third Birthday #24728! 🎈 🎉 |
I ran into this, but I think this solution will have performance issues, because will validate all unchanged associated records. |
any solution on this? |
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.. |
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. |
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 |
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