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

Support composite keys #17

Closed
wants to merge 2 commits into from
Closed

Conversation

jackozi
Copy link

@jackozi jackozi commented Jul 11, 2024

Adds support for composite primary keys.

@brendon
Copy link
Owner

brendon commented Jul 13, 2024

Hi @jackozi, thanks for tackling this :) It's been something on my todo list. Allow me some time to go through it all and make sure I'm happy with it. Ideally, more tests would be good. I see the CI suite failed.

@jackozi
Copy link
Author

jackozi commented Jul 13, 2024

Hi @jackozi, thanks for tackling this :) It's been something on my todo list. Allow me some time to go through it all and make sure I'm happy with it. Ideally, more tests would be good. I see the CI suite failed.

Glad to hear. There are three ways I considered doing this. Always casting in the initial get, so we're always dealing with arrays in the rest of the code, and then casting could be skipped there. However, this would alter that non-composite keys are also dealt with as arrays, I don't prefer this option. Instead of casting, it is also possible to check and see what value is being dealt with (Array/non-Array). I am open to this option, so if you prefer that, just let me know, and I'll swap it around.

As for the CI not passing this is due to old Rails versions not supporting composite primary keys. Would you be OK with disabling CPK tests for Rails versions that do not support this?

@brendon
Copy link
Owner

brendon commented Jul 15, 2024

Ah, good point about old rails. Certainly skip those tests for older versions. I think there's a graceful way to detect that. I won't probably be able to have a good look at this until next week when I'm back at work.

@brendon
Copy link
Owner

brendon commented Aug 1, 2024

Hi @jackozi, just letting you know I'm looking into this now. There appears to be a much cleaner way to implement this that doesn't rely on array detection (at least not in our code). A lot of the work is already done here: https://github.com/rails/rails/blob/v7.1.3.4/activerecord/lib/active_record/attribute_methods/primary_key.rb

@brendon
Copy link
Owner

brendon commented Aug 2, 2024

Hi @jackozi, I've implemented composite primary keys on main. Give it a go and let me know if you encounter any issues. As you can see, the implementation turned out to be fairly straightforward. I was also able to use the existing test suite as a superclass with minor modifications to conditionally test composite primary keys on versions of Rails that support it.

Thanks for your work on this. It certainly helped motivate me to implement it as a feature.

@brendon brendon closed this Aug 2, 2024
@jackozi
Copy link
Author

jackozi commented Aug 2, 2024

Hi @jackozi, I've implemented composite primary keys on main. Give it a go and let me know if you encounter any issues. As you can see, the implementation turned out to be fairly straightforward. I was also able to use the existing test suite as a superclass with minor modifications to conditionally test composite primary keys on versions of Rails that support it.

Thanks for your work on this. It certainly helped motivate me to implement it as a feature.

Hey Brandon, glad I was able to be of any help. I will have a look at it somewhere next week as my calendar for the coming days is already overflowing. Thanks for all the effort you put in and I'll keep you posted!

@brendon
Copy link
Owner

brendon commented Aug 2, 2024

No worries! :) It's been a hectic time over this way too :D

@jackozi
Copy link
Author

jackozi commented Aug 9, 2024

No worries! :) It's been a hectic time over this way too :D

Played around with your version and all appear to be working for my use case. Thanks for your work on the gem and have an amazing weekend :)

@brendon
Copy link
Owner

brendon commented Aug 9, 2024

Excellent! I've released 0.2.5 for you to use :) Have a great weekend too!

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