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.

Sign up for GitHub

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

Jump to bottom

Sam/reenable automatic inference of inverse of for autosave associations #26952

Merged
merged 1 commit into from
Nov 2, 2016
Merged

Sam/reenable automatic inference of inverse of for autosave associations #26952

merged 1 commit into from
Nov 2, 2016

Conversation

samsondav
Copy link

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (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.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@rails-bot
Copy link

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against 5-0-stable. Please double check that you specified the right target!

@samsondav
Copy link
Author

cc @sgrif

Re-enable automatic inference of inverse_of for autosave associations
9fe08e8 
Fixes rails#9336, fixes rails#22391, fixes rails#22358, fixes rails#23163, fixes rails#23171, and probably more.

We weren't automatically inferring the inverse_of for autosaved associations. This appears to have been a hack in order to avoid double-save problems with nested attributes but at the expense of not being able to access the parent model during the save callback chain.

Among other things this was preventing validation of parent(s) when creating childen via nested attributes.

The correct fix is to automatically infer inverse_of for autosaved associations and to address the root issue of double queries, which is to ensure that child records saved in callbacks do not cause a duplicate save on the parent.

We do this by autosaves of associated records in a block which temporarily disables reverse autosave on the record which triggered it. This ensures we only save each record in the chain once.
@samsondav samsondav force-pushed the sam/reenable_automatic_inference_of_inverse_of_for_autosave_associations branch from ea7bb20 to 9fe08e8 Compare November 2, 2016 09:51
@sgrif sgrif merged commit b01278e into rails:5-0-stable Nov 2, 2016
@rafaelfranca
Copy link
Member

This PR broke a lot of test in my application. I still didn't investigated yet why but this is a sign that it is not safe to applied to a stable branch so I'm reverting.

@rafaelfranca
Copy link
Member

The problem is a has_one association with autosave: true is not inserting the record in the database because the foreign key is NULL in the belongs to side. But I also found this problem in a has_many association without autosave: true. The foreign key is being set to true.

@rafaelfranca
Copy link
Member

Reverted in f5957d0

rafaelfranca added a commit that referenced this pull request Nov 8, 2016
@rafaelfranca
Revert "Merge pull request #26952 from samphilipd/sam/reenable_automa…
f5957d0 
…tic_inference_of_inverse_of_for_autosave_associations"

This reverts commit b01278e, reversing
changes made to b0b4a9a.
@sgrif
Copy link
Contributor

sgrif commented Nov 8, 2016

Have you checked master as well?

On Mon, Nov 7, 2016, 8:45 PM Rafael França notifications@github.com wrote:

The problem is a has_one association with autosave: true is not inserting
the record in the database because the foreign key is NULL in the belongs
to side. But I also found this problem in a has_many association without autosave:
true. The foreign key is being set to true.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#26952 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdWK8Dn1BSEZvTHPmv_GT0IDbqV8IViks5q79RAgaJpZM4KnCgF
.

@rafaelfranca
Copy link
Member

I can't point Shopify to master yet :(

@sgrif
Copy link
Contributor

sgrif commented Nov 8, 2016

Ok. I'll see if I can isolate and investigate further tomorrow

On Mon, Nov 7, 2016, 8:48 PM Rafael França notifications@github.com wrote:

I can't point Shopify to master yet :(

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#26952 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdWK_HSo5q54kES7Z8yiPdLWEFB3dZ6ks5q79T1gaJpZM4KnCgF
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rafaelfranca rafaelfranca

Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@samsondav @rails-bot @rafaelfranca @sgrif @maclover7