rails icon indicating copy to clipboard operation
rails copied to clipboard

Fix accidental building of child records when `inverse_of` is specified

Open clouvet opened this issue 1 year ago • 6 comments

Summary

If an existing record with inverse_of children was duplicated, additional child records would be built. These new records could be persisted upon save or update inadvertently. I added a method to address that case and prevent the accidental creation of additional associations.

I traced the problem back to this line in the replace_on_target method in CollectionAssociation. Using the steps to reproduce the problem in the original issue as an example, I wrote a test to cover the case of new child records being added to existing owner records when inversing is in play.

With this change in place, the original steps to reproduce the problem deliver the expected result of no additional records.

Fixes #44819.

clouvet avatar May 20 '22 08:05 clouvet

I force-pushed an update here to resolve an activerecord/CHANGELOG.md conflict.

clouvet avatar Jun 09 '22 14:06 clouvet

@ghiculescu, since it's been a while I'm giving this a little nudge. I'm happy to revise or close this pull request if this approach isn't preferred.

clouvet avatar Jun 15 '22 11:06 clouvet

Thanks @clouvet, but this would need review from a member of the Rails core / committers teams. I think the approach is fine. They are pretty busy, but they will eventually get to it :)

It looks like you have a merge conflict btw.

ghiculescu avatar Jun 15 '22 14:06 ghiculescu

Thanks, @ghiculescu, I appreciate the response. I resolved the conflict with activerecord/CHANGELOG.md again.

clouvet avatar Jun 16 '22 14:06 clouvet

@ghiculescu I made changes here in response to your review: I dropped the CHANGELOG addition, accepted your suggestion to use unless instead of if !, and revised the comment on new_record_inverse_of_existing_owner? to explain why that new method exists.

I'm happy to make additional changes if necessary, and of course I'll squash these multiple commits down to one when the time comes.

Thanks for the review!

clouvet avatar Jul 16 '22 02:07 clouvet

Sorry for the noise here. I squashed the cleanup commits to make this pull request suitable for merging.

clouvet avatar Sep 16 '22 07:09 clouvet