-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Support composite foreign keys via migration helpers #47637
Conversation
a277972
to
ceaf183
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @fatkodima thanks for working on this! I've left some comments.
@@ -1099,6 +1099,17 @@ def foreign_keys(table_name) | |||
# | |||
# ALTER TABLE "articles" ADD CONSTRAINT fk_rails_58ca3d3a82 FOREIGN KEY ("author_id") REFERENCES "users" ("lng_id") | |||
# | |||
# ====== Creating a composite foreign key | |||
# | |||
# # Assuming "carts" table has "(shop_id, id)" as a primary key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# # Assuming "carts" table has "(shop_id, id)" as a primary key. | |
# Assuming "carts" table has "(shop_id, id)" as a primary key. |
activerecord/CHANGELOG.md
Outdated
* Support composite foreign keys via migration helpers. | ||
|
||
```ruby | ||
# Assuming "carts" table has "(shop_id, id)" as a primary key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are actually going to discourage using id
as one of the ids in the composite key when we write the docs so can we change this to something like (shop_id, oid)
?
# | ||
# ALTER TABLE "orders" ADD CONSTRAINT fk_rails_6f5e4cb3a4 FOREIGN KEY ("cart_shop_id", "cart_id") REFERENCES "carts" ("shop_id", "id") | ||
# | ||
# Note: At least one of +column+ or +primary_key+ options must be provided. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super clear on why we need to support both, can we support just primary_key and simplify the code a bit?
ceaf183
to
45f6015
Compare
@eileencodes Thanks for the review, addressed the feedback. I initially added support for both |
45f6015
to
3f17465
Compare
We only set the column to a single column if the primary key is a single value, so move that code to an explicit else.
This method is internal so we can just always pass the argument.
Closes #47593.
We discussed 3 possible ways of using
add_foreign_key
.The first one (
add_foreign_key :brochures, :cars
) (no:column
and:primary_key
options) turned out to be complex to implement, as it introduced a lot of kinda ugly changes in many places because we needed to properly generate these column names (consider configured table name prefixes and suffixes, pass a connection around to be able to query a primary key for theto_table
. Some classes, likeForeignKeyDefinition
does not have access to the connection, and so we needed to calculate these columns at several places differently and pass there).And the biggest problem is when we need to add a foreign key for a self referencing tables, like
rails/activerecord/test/cases/migration/references_foreign_key_test.rb
Lines 221 to 225 in 1df6ad0
So I made that it is required to pass at least one of
:column
or:primary_key
options, as it is easy to infer one from the other.cc @eileencodes @nvasilevski