Skip to content
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

Rails bi-directional accepts_nested_attributes_for leads to unexpected validation result #41811

Open
rsalehi2013 opened this issue Mar 31, 2021 · 11 comments · May be fixed by #41878
Open

Rails bi-directional accepts_nested_attributes_for leads to unexpected validation result #41811

rsalehi2013 opened this issue Mar 31, 2021 · 11 comments · May be fixed by #41878

Comments

@rsalehi2013
Copy link

rsalehi2013 commented Mar 31, 2021

Steps to reproduce

script: https://gist.github.com/rsalehi2013/7cf9c458af9923bcd3e2f3053cefcc55

Setup
Migration:

create_table :posts do |t|
  t.string :title
end

create_table :comments do |t|
  t.integer :post_id
  t.string :body
end

post.rb

class Post < ActiveRecord::Base
  has_many :comments, inverse_of: :post
  accepts_nested_attributes_for :comments

  validates :title, presence: true
end

comment.rb

class Comment < ActiveRecord::Base
  belongs_to :post, inverse_of: :comments
  accepts_nested_attributes_for :post
  
  validates :body, presence: true
end

Procedure
post = Post.new(title: 'foo', comments_attributes: [{body: 'bar'}, {body: nil}, {body: 'baz'}])
post.valid?
post.errors

Expected behavior

valid? -> false
errors -> @messages={:"comments.body"=>["can't be blank"]}, @details={:"comments.body"=>[{:error=>:blank}]}
user is prevented from persisting to the database

Actual behavior

valid? -> true
errors -> @messages={}, @details={}
user is able to persist the invalid records to the database

NOTE: if the invalid comments attribute is at the end of the comments_attributes list, or if there is only one item in the comments_attributes list, the post is correctly marked as invalid

System configuration

Rails version: Rails 5.2.4.5

Ruby version: ruby 2.5.8p224 (2020-03-31 revision 67882) [x86_64-linux]

@intrip
Copy link
Contributor

intrip commented Apr 1, 2021

What happens if you set validate: true on the belongs_to ? belongs_to :post, inverse_of: :comments, validate: true

Also would be great if you can put your steps into a runnable repro script :)

@rsalehi2013
Copy link
Author

@intrip adding validate: true to the association definition on Comments appears to have no effect on the validity in this case
here's a reproduction script: https://gist.github.com/rsalehi2013/7cf9c458af9923bcd3e2f3053cefcc55

@intrip
Copy link
Contributor

intrip commented Apr 2, 2021

@rsalehi2013 thanks I'll take a look

@intrip
Copy link
Contributor

intrip commented Apr 8, 2021

@rsalehi2013 #41878 fixes this issue: please check.

Waiting for feedback from the core team now 😄

intrip added a commit to intrip/rails that referenced this issue May 3, 2021
When defining bi-directional `accepts_nested_attributes` the validation
of a collection doesn't work well: in particular only the last record
of the collection is used. This happens because each child item in
the collection calls `valid?` on the parent record erasing the previous
validation errors stored in the parent.

Fixes the issue by tracking the record under validation and
skipping cyclic `valid?` calls.

Fixes rails#41811
@rails-bot
Copy link

rails-bot bot commented Aug 17, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Aug 17, 2021
@intrip
Copy link
Contributor

intrip commented Aug 18, 2021

Still relevant, PR attached.

@Dwimcore
Copy link

Dwimcore commented Feb 4, 2022

Hello, any news on a fix ?

@intrip
Copy link
Contributor

intrip commented Feb 10, 2022

There is an attached PR waiting for review

@Dwimcore
Copy link

@intrip I've seen it, thank you. I was more asking if it would be merged sometime soon, my bad.
I have to thank you, we temporarily applied your solution as a monkey patch and it seems to work well :)

@donquxiote
Copy link

donquxiote commented Apr 22, 2022

Hi @intrip, I had a quick question about your fix. As the code stands in the PR now, should the fix be backwards compatible with Rails 6.1? I'm trying to monkey patch it in an app that's on Rails 6.1, but I'm not seeing the change I'd expect.
I see you force pushed a few different commits but couldn't find a way to view the older commits. Was there significant differences in a fix for Rails 6.1 verses the current PR? (That is if the current PR is only for Rails 7.) While it's possible my issue is different or I monkey patched your solution improperly, I just want to check all options.

Nevermind, my issue was different and project specific!

@goulvench
Copy link
Contributor

Hi, this issue looks a lot like this Stack Overflow bug report, where calling update and passing nested attributes returns true even though at least one nested record is invalid.

The workaround indicated in the accepted answer is easy and I confirm that it works: load the has_many before updating them. Here would be a way to apply this to the situation described:

# app/models/post.rb
class Post < ActiveRecord::Base
  has_many :comments, inverse_of: :post
  accepts_nested_attributes_for :comments

  validates :title, presence: true

  # WORKAROUND STARTS HERE
  def comments_attributes(params)
    # Preload comments to ensure validation state remains in memory
    comments.load
    super
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants