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

Infer foreign_key when inverse_of is present #47797

Merged
merged 4 commits into from Mar 30, 2023
Merged

Conversation

Tiedye
Copy link
Contributor

@Tiedye Tiedye commented Mar 28, 2023

Motivation / Background

This Pull Request makes the behaviour of has_one and has_many associations more convenient. This new behaviour lines up with what I thought rails did based on the docs, as it is not explicit about if the inverse_of option depends on the foreign_key option.

This makes has_one and has_many associations automatically infer the foreign_key option when the inverse_of option is present.

Detail

When inverse_of is present, rails has all the info it needs to figure out what the foreign_key on the associated model should be. I can't imagine this breaking anything as it doesn't change the default, and anywhere where this would change the inferred foreign_key it would've been broken otherwise I think.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rafaelfranca
Copy link
Member

Can you write a test for this? This code doesn't break any existing test, but nothing prevents this code to be removed in he future, so we need a test to make sure the feature you are adding will not be removed.

Also, this needs a changelog since it is a new feature. It is small, but it is still a feature.

@Tiedye
Copy link
Contributor Author

Tiedye commented Mar 29, 2023

@rafaelfranca I updated some of the existing models to use the new behaviour so some various old test now explode when the new behaviour is reverted (test_delete_associate_when_deleting_from_has_many_through_with_nonstandard_id, etc.).

Is that sufficient? It was quick and I figure it is simpler than adding new associations that rely on the new inference, as I am not sure what the conventions are for doing that.

@@ -4,7 +4,7 @@ class Book < ActiveRecord::Base
belongs_to :author
belongs_to :format_record, polymorphic: true

has_many :citations, foreign_key: "book1_id", inverse_of: :book
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha! good idea

@Tiedye
Copy link
Contributor Author

Tiedye commented Mar 29, 2023

I'm not sure whats causing the markdown lint failure, the change I made seems to obey the rules the error is complaining about.

@Tiedye
Copy link
Contributor Author

Tiedye commented Mar 29, 2023

I'm not sure whats causing the markdown lint failure, the change I made seems to obey the rules the error is complaining about.

Ah, probably just bad timing. #47779

@zzak
Copy link
Member

zzak commented Mar 30, 2023

I'm not sure whats causing the markdown lint failure, the change I made seems to obey the rules the error is complaining about.

Whoops, yeah that must have snuck in. Fixed by #47812 if you want to rebase. 🙏 Sorry for the trouble!

Tiedye and others added 4 commits March 30, 2023 14:03
Automatically infer `foreign_key` on `has_one` and `has_many` associations when `inverse_of` is present.

When inverse of is present, rails has all the info it needs to figure out what the foreign_key on the associated model should be.  I can't imagine this breaking anything
Co-authored-by: Rafael Mendonça França <rafael@franca.dev>
@Tiedye
Copy link
Contributor Author

Tiedye commented Mar 30, 2023

I think this is good to go, the build failure seems unrelated (the failing test TestCaseTest#test_body_stream was already failing)

@rafaelfranca rafaelfranca merged commit 15369fd into rails:main Mar 30, 2023
8 checks passed
@Tiedye Tiedye deleted the patch-1 branch March 31, 2023 14:34
@@ -1,3 +1,19 @@
* Infer `foerign_key` when `inverse_of` is present on `has_one` and `has_many` associations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

misspelling

@zzak zzak changed the title Infer foerign_key when inverse_of is present Infer foreign_key when inverse_of is present Mar 31, 2023
northeastprince added a commit to hackclub/hackathons-backend that referenced this pull request Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants