Bugfix: Fix false autosave for has_one :through association #35683
Conversation
Kukunin
changed the title
|
How can this be backported to 5.2.x branch? |
There was a problem hiding this comment.
Thanks for fixing @Kukunin - please don't backport until we have decided whether to merge this PR. Once merged we will backport or ask you to open a new PR if there are conflicts with the backport.
I restarted the travis build because there were a lot of broken builds but they seemed related to some problem with travis and not here - but also meant it couldn't verify the tests passed.
I also left a few comments but otherwise this is looking good 👍
|
@eileencodes does it look good now? |
| end | ||
|
|
||
| def _association_foreign_key_changed?(reflection, record, key) | ||
| return false if reflection.through_reflection |
There was a problem hiding this comment.
Use through_reflection?.
| return false if reflection.through_reflection | |
| !reflection.through_reflection? && | |
| record.has_attribute?(reflection.foreign_key) && record[reflection.foreign_key] != key |
There was a problem hiding this comment.
I dared to leave this as is. Comparing with the other case, where are 3 guards, here is only one.
And mentally things are different enough to not join them into a single complex condition phrase. I consider this as a mentally easier, however less concise
|
@kamipo @eileencodes so we got a conflict on condition form: return true if some_condition_A
return true if some_condition_B
return true if some_extremely_long_condition_C
falseor some_condition_A ||
some_condition_B ||
some_extremely_long_condition_CWhich one should I use in this PR? |
|
We usually don’t use multiple early returning. By extracted a little complex condition, this is already simple enough. |
Fixes rails#35680 The problem occurred, when a `has one through` association contains a foreign key (it belongs to the intermediate association). For example, Comment belongs to Post, Post belongs to Author, and Author `has_one :first_comment, through: :first_post`. In this case, the value of the foreign key is comparing with the original record, and since they are likely different, the association is marked as changed. So it updates every time when the origin record updates.
|
Can’t you merge squashed? GitHub supports this feature. If no, I’ll squash it in a couple of hours |
|
squashed, ready to merge |
Bugfix: Fix false autosave for has_one :through association
Summary
Fixes #35680
Other Information
The problem occurred, when a
has one throughassociation contains a foreign key (it belongs to the intermediate association).For example, Comment belongs to Post, Post belongs to Author, and Author
has_one :first_comment, through: :first_post.In this case, the value of the foreign key is comparing with the original record, and since they are likely different, the association is marked as changed. So it updates every time when the origin record updates.