Skip to content

Fix child record's validation clearing the parent's errors (has_one/belongs_to) #54417

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ellnix
Copy link

@ellnix ellnix commented Jan 31, 2025

Motivation / Background

This Pull Request has been created because I wanted to try fixing a bug, specifically FIXES #54367

I don't have a particular need for the bug to be fixed, I'm just trying to get more familiar with Rails.

Detail

This Pull Request changes the validation logic on has_one/belongs_to associations to allow validation of has_one's inverse while the belongs_to record is being validated.

Let pirate be the has_one side and ship be the belongs_to side. Ship's attributes have errors, pirate's do not.

Step 1:

  • pirate.valid? -> pirate is validated -> pirate validates ship -> pirate now has ship's nested error

From my understanding, the way this worked before this PR was:
Step 2:

  • ship.valid?
    • -> ship is validated, sets a flag that belongs_to association is being validated
    • -> ship validates pirate
      • -> pirate sees that belongs_to association flag is up, does not validate ship
      • -> pirate has no errors

With this change:
Step 2:

  • ship.valid?
    • -> ship is validated, sets a flag that belongs_to association is being validated
    • -> ship validates pirate
      • -> pirate does not check flag
      • -> pirate validates ship
        • -> ship sees the belongs_to association flag is set, does not validate pirate
        • -> ship gets error on :name
      • -> pirate has a nested error on ship.name
    • -> ship has a nested error on pirate.ship.name

This seems to be consistent with 7.2.2. I confirmed manually by using the gist on the issue and changing the test:

  def test_association_stuff
    dojo = Dojo.new({"master_attributes"=>{"name"=>""}})
    assert_equal false, dojo.valid?
    assert_equal 1, dojo.master.errors.count
    p dojo.master.errors
    assert_equal false, dojo.master.valid? # Call to dojo.master.valid? unexpectedly clears dojo.errors with Rails 8.0.1
    assert_equal 1, dojo.errors.count
    p dojo.master.errors
    assert_equal 2, dojo.master.errors.count
    assert_equal dojo.master.errors.objects.last.attribute, :"dojo.master.name"
  end

The outcome is the same on my branch and on 7.2.2. If I should make an attempt to avoid the double validation of the belongs_to side, please let me know and I will try to make the change.

Additional information

Please see the gist posted on the issue.

This is my first time reading or modifying significant (and it was significant 😩) amounts of Rails source code. It's also my first PR here, hopefully of many to come.

That said please don't feel the need to be pleasant, I'm willing to put in time to get this done right. I don't take offense at being told my code is bad or that I should make more changes, be as rude as you want.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes 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.

See rails#54367
This test is close to the provided reproduction, but is a little bit
more strict about parent's errors being equal to what they were before
child was validated.
This change
- prevents nested errors from being "cleared" on the has_one
  side when the belongs_to side is validated.
- in the previous scenario, the belongs_to side will have a nested error
  referring to the nested error in the has_one side (so it's a nested
  error of a nested error), but this behavior is consistent with Rails
  7.2.2.

[Fix rails#54367]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rails 8 clears ActiveRecord errors on parent record when .valid? is called on a nested record
1 participant