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

Bugfix foreign key replacement in inverse association #31575

Merged
merged 1 commit into from Dec 31, 2017

Conversation

@bogdan
Copy link
Contributor

@bogdan bogdan commented Dec 27, 2017

when model is added to collection association

Summary

The foreign key is not updated while the associated model is updated in the following scenario:

    bulb = Bulb.create!(car: Car.create!)
    Car.new.bulbs << bulb
    assert_nil bulb.car_id
when model is added to collection association
@rails-bot
Copy link

@rails-bot rails-bot commented Dec 27, 2017

r? @georgeclaghorn

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

@kamipo
Copy link
Member

@kamipo kamipo commented Dec 31, 2017

replace_keys will be worked in set_inverse_instance as well by this fix. Looks fine to me.

@kamipo kamipo merged commit 7928401 into rails:master Dec 31, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bogdan bogdan deleted the bogdan:bugfix-has-many-reattachment branch Dec 31, 2017
tgxworld pushed a commit to tgxworld/rails that referenced this pull request Apr 26, 2018
…ozen error

This is to ensure that the behavior has not changed before and after
rails#31575.
kamipo added a commit that referenced this pull request May 25, 2018
Since #31575, `BelongsToAssociation#target=` replaces owner record's
foreign key to fix an inverse association bug.

But the method is not only used for inverse association but also used
for eager loading/preloading, it caused some public behavior changes
(#32338, #32375).

To avoid any side-effect in loading associations, I reverted the
overriding `#target=`, then introduced `#inversed_from` to replace
foreign key in `set_inverse_instance`.

Closes #32375.
kamipo added a commit that referenced this pull request May 25, 2018
Since #31575, `BelongsToAssociation#target=` replaces owner record's
foreign key to fix an inverse association bug.

But the method is not only used for inverse association but also used
for eager loading/preloading, it caused some public behavior changes
(#32338, #32375).

To avoid any side-effect in loading associations, I reverted the
overriding `#target=`, then introduced `#inversed_from` to replace
foreign key in `set_inverse_instance`.

Closes #32375.
kamipo added a commit that referenced this pull request Oct 7, 2018
…efore

Since #31575, `set_inverse_instance` replaces the foreign key by the
current owner immediately to make it happen when a record is added to
collection association.

But `set_inverse_instance` is not only called when a record is added,
but also when a record is loaded from queries. And also, that loaded
records are not always associated records for some reason (using `or`,
`unscope`, `rewhere`, etc).

It is hard to distinguish whether or not we should invoke
`set_inverse_instance`, but at least we should avoid the undesired
side-effect which was brought from #31575.

Fixes #34108.
kamipo added a commit that referenced this pull request Oct 7, 2018
…efore

Since #31575, `set_inverse_instance` replaces the foreign key by the
current owner immediately to make it happen when a record is added to
collection association.

But `set_inverse_instance` is not only called when a record is added,
but also when a record is loaded from queries. And also, that loaded
records are not always associated records for some reason (using `or`,
`unscope`, `rewhere`, etc).

It is hard to distinguish whether or not we should invoke
`set_inverse_instance`, but at least we should avoid the undesired
side-effect which was brought from #31575.

Fixes #34108.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.