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

Delegate primary_key to super in abstract class #240

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dlwr
Copy link
Contributor

@dlwr dlwr commented Jun 24, 2024

Related Issues and PRs:

When I submitted PR #218, my approach was rather symptomatic. Now, I have a better understanding.

  • Rails 7.1 has added support for composite_primary_key as mentioned in the Ruby on Rails 7.1 Release Notes.
  • This change ensures that primary_key is checked for all ActiveRecord classes, including abstract classes.
  • The .primary_key method implemented in this gem throws an exception for abstract classes because table_name is nil. I added a nil guard to make AbstractClass.primary_key return nil (Fix issue to avoid errors when table_name is nil #218).

However, this approach seems inappropriate. As indicated by the following link, ActiveRecord is designed to have AbstractClass.primary_key return 'id' and not nil:
https://github.com/rails/rails/blob/de4d8744744acab2dd9db0683ccf784ea45810b2/activerecord/lib/active_record/attribute_methods/primary_key.rb#L106-L116

The value of AbstractClass.primary_key is not the concern of this PR, so I have updated the implementation to delegate to super in such cases.

If there are any other approaches or suggestions, please let me know and I will implement them accordingly.

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.

1 participant