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

Rails 71 composite pks #837

Merged
merged 2 commits into from
Apr 26, 2024
Merged

Conversation

fragkakis
Copy link
Contributor

@fragkakis fragkakis commented Apr 22, 2024

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.

@@ -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]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"]

Comment on lines +999 to +1002
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good 👍 If an association doesn't explicitly state the inverse_of does AR infer that relationship so that this still works?

Copy link
Contributor Author

@fragkakis fragkakis Apr 24, 2024

Choose a reason for hiding this comment

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

@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".

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's awesome 😄

Copy link
Collaborator

@jkowens jkowens left a comment

Choose a reason for hiding this comment

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

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.

@jkowens jkowens merged commit f41a578 into zdennis:master Apr 26, 2024
21 checks passed
@jkowens
Copy link
Collaborator

jkowens commented May 17, 2024

Released in v1.7.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants