Rails 71 composite pks #837
Conversation
| @@ -941,7 +941,7 @@ def load_association_ids(model) | |||
| association = association.target | |||
| next if association.blank? || model.public_send(column_name).present? | |||
|
|
|||
| association_primary_key = Array(association_reflection.association_primary_key)[column_index] | |||
| association_primary_key = Array(association_reflection.association_primary_key.tr("[]:", "").split(", "))[column_index] | |||
There was a problem hiding this comment.
Without a composite PK, the association_reflection.association_primary_key has a value of the format "id", whereas with a composite PK in ActiveRecord 7.1 the value is of the format "[:id, :account_id]" (again a String). The .tr loses the [, :, ] characters, so that the .split(", ") will give us an array of the two colums: ["id", "account_id"]
| Array(association_reflection.inverse_of&.foreign_key || association_reflection.foreign_key).each_with_index do |column, index| | ||
| child.public_send("#{column}=", Array(model.id)[index]) | ||
| end | ||
|
|
There was a problem hiding this comment.
Another note for the reviewers.
Let's now consider we want to recursively import the model CompositeBook and its children CompositeChapter.
Here is the has_many relation on CompositeBook, mentioning the names of the columns id and author_id. These are the PK columns in the composite_books table.
As we are setting the ids in the children, we need the columns as they are called in the children table (composite_chapters), which is here. So we need the inverse relation (the belongs_to in CompositeChapter), which has the names composite_book_id and author_id.
In order not to break things, I kept the original association_reflection.foreign_key as a fallback.
There was a problem hiding this comment.
Looks good 👍 If an association doesn't explicitly state the inverse_of does AR infer that relationship so that this still works?
There was a problem hiding this comment.
@jkowens I tried it out, and it still works without the inverse_of attribute being declared on the has_many and belongs_to relationships. I don't know how AR infers it, but it does, at least in my case, where the names of the relations are "standard".
There was a problem hiding this comment.
This looks good to me! Thanks @fragkakis 🙌 It doesn't sound like there will be any CPK gem releases for AR 7.1, so I think it's safe to end support with AR 7.0.
Adding support for Rails 7.1 composite primary keys (issue #830 )
For Rails 7.1 I am making the assumption that the composite-primary-keys gem is not going to be used, since Rails 7.1 has out-of-the-box support for composite PKs. I have added some inline comments in the PR for the reviewers.