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

Fix parent record should not get saved with duplicate children records #32952

Merged
merged 1 commit into from May 28, 2018

Conversation

Projects
None yet
7 participants
@mechanicles
Copy link
Contributor

mechanicles commented May 22, 2018

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented May 22, 2018

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@Mangara

This comment has been minimized.

Copy link

Mangara commented May 22, 2018

Thanks for taking a look at this so quickly! Could you verify that the fix also works with these test cases?

many-to-many relation
many-to-many :through

@mechanicles mechanicles force-pushed the mechanicles:32940-fix branch May 22, 2018

@Mangara

This comment has been minimized.

Copy link

Mangara commented May 22, 2018

I'm not sure if it's worth creating new models for the test cases. Would something similar to the tests in PR #11851 work here?

@mechanicles mechanicles force-pushed the mechanicles:32940-fix branch 2 times, most recently May 22, 2018

@mechanicles

This comment has been minimized.

Copy link
Contributor Author

mechanicles commented May 22, 2018

Removed newly created models and used existing ones.

@mechanicles mechanicles force-pushed the mechanicles:32940-fix branch 2 times, most recently May 23, 2018

activerecord/test/cases/autosave_association_test.rb Outdated
@@ -536,7 +538,7 @@ def test_invalid_adding_with_validate_false

assert_predicate firm, :valid?
assert_not_predicate client, :valid?
assert firm.save
assert_not firm.save

This comment has been minimized.

@eugeneius

eugeneius May 24, 2018

Member

This changes documented behaviour:

# [:validate]
# When set to +true+, validates new objects added to association when saving the parent object. +true+ by default.

Associations specified with validate: false should not prevent the parent record from being saved.

This comment has been minimized.

@mechanicles

mechanicles May 27, 2018

Author Contributor

@eugeneius Thanks. I will go through it.

@mechanicles mechanicles force-pushed the mechanicles:32940-fix branch 2 times, most recently May 28, 2018

@mechanicles

This comment has been minimized.

Copy link
Contributor Author

mechanicles commented May 28, 2018

@eugeneius I have updated the code. Please check.

activerecord/lib/active_record/autosave_association.rb Outdated
validate = reflection.options[:validate] != false
if validate
valid = association_valid?(reflection, record, index)
saved = valid ? association.insert_record(record) : false

This comment has been minimized.

@eugeneius

eugeneius May 28, 2018

Member

Calling association_valid? on the line above validates the record:

unless valid = record.valid?(context)

Calling insert_record also validates the record:

def insert_record(record, validate = true, raise = false, &block)
if raise
record.save!(validate: validate, &block)
else
record.save(validate: validate, &block)
end
end

We should pass false as the second argument to insert_record, to avoid the unnecessary second validation.

activerecord/lib/active_record/autosave_association.rb Outdated
saved = association_saved
else
if !reflection.nested?
validate = reflection.options[:validate] != false

This comment has been minimized.

@eugeneius

eugeneius May 28, 2018

Member

Is there any reason to switch from reflection.validate? to this?

activerecord/lib/active_record/autosave_association.rb Outdated
if reflection.validate?
saved = association_saved
else
if !reflection.nested?

This comment has been minimized.

@eugeneius

eugeneius May 28, 2018

Member

This was fine as an elsif, no need to change it.

activerecord/lib/active_record/autosave_association.rb Outdated
next if record.destroyed?

saved = true

This comment has been minimized.

@eugeneius

eugeneius May 28, 2018

Member

There's no need to remove this line ✂️

activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,9 @@
* Fix parent record should not get saved with duplicate children records

Fixes #32940 & #32939

This comment has been minimized.

@eugeneius

eugeneius May 28, 2018

Member

#32939 was already fixed by #32796.

@mechanicles mechanicles force-pushed the mechanicles:32940-fix branch 2 times, most recently May 28, 2018

@mechanicles mechanicles force-pushed the mechanicles:32940-fix branch to d7a3f33 May 28, 2018

@mechanicles

This comment has been minimized.

Copy link
Contributor Author

mechanicles commented May 28, 2018

@eugeneius I have updated the code. Now I have passed the second argument false to insert_method. Please check and let me know if I missed something.

@kamipo

kamipo approved these changes May 28, 2018

@kamipo kamipo merged commit d7a3f33 into rails:master May 28, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

kamipo added a commit that referenced this pull request May 28, 2018

Merge pull request #32952 from mechanicles/32940-fix
Fix parent record should not get saved with duplicate children records

kamipo added a commit that referenced this pull request May 28, 2018

Merge pull request #32952 from mechanicles/32940-fix
Fix parent record should not get saved with duplicate children records

@mechanicles mechanicles deleted the mechanicles:32940-fix branch May 29, 2018

kamipo added a commit that referenced this pull request Jun 6, 2018

@jasonperrone

This comment has been minimized.

Copy link

jasonperrone commented Sep 28, 2018

I have a question about this code. Am I seeing it right that insert_record is called with validate=false if reflection.validate? is true, and called with validate=true if reflection.validate? is false?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.