Support composite foreign keys via migration helpers#47637
Support composite foreign keys via migration helpers#47637rafaelfranca merged 3 commits intorails:mainfrom
Conversation
a277972 to
ceaf183
Compare
eileencodes
left a comment
There was a problem hiding this comment.
Hey @fatkodima thanks for working on this! I've left some comments.
There was a problem hiding this comment.
| # # 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
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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:columnand:primary_keyoptions) 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, likeForeignKeyDefinitiondoes 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
:columnor:primary_keyoptions, as it is easy to infer one from the other.cc @eileencodes @nvasilevski