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

Add composite primary key support #430

Merged
merged 12 commits into from
Jun 2, 2024

Conversation

divagueame
Copy link
Contributor

This PR adds support for Composite primary key which is supported from Rails 7.1
This was requested in this issue
#417

Locally I had to run bundle exec appraisal install to generate rails_7_1.gemfile to run the tests. I didn't include it in the PR since I am not sure if it' s needed/left out for any particular reason/be dependent on my local environment.
Any feedback is welcome.


def composite_primary_key?
self.class.respond_to?(:composite_primary_key?) && self.class.composite_primary_key?
id = id ? Array.wrap(id) : Array.wrap(primary_keys.map { |k| send(k) })
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the update. Did you try slice(*primary_keys)? With Active Record objects it returns a hash with keys that you provide and the values as they are on the object.

https://api.rubyonrails.org/classes/ActiveRecord/Core.html#method-i-slice

This line could then read:

id ? primary_keys.zip(Array.wrap(id)) : slice(*primary_keys)

Copy link
Contributor Author

@divagueame divagueame May 14, 2024

Choose a reason for hiding this comment

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

Thanks for the advice. That works indeed. I didn't know that and I thought about the default ruby slice method. No wonder how it didn't work for me : D
I changed it now and it seems to work.

I am still refactoring the tests to include scope, but not sure if I'm just complicating it too much in order to reuse test/shared_list.rb I'll try to clean it up a bit before commiting it.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks @divagueame. Yea I didn't write these tests and they've kind of grown into a mess over time. I think it's ok to just repeat some scope change tests in your seperate file for sanity. Single ID primary keys are used directly in the tests which make things brittle. I suppose that test suite could be configured to use a single id and a composite in two seperate runs via some kind of configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored Shared::List so that the Mixin used can be overwritten. I had to add a helper method to be able to create records with composite primary key. Not sure if it's better than having a separate file with some duplicated tests. Let me know what you think.

end

group :postgresql do
gem "pg", "~> 1.2.0"
Copy link
Contributor Author

@divagueame divagueame May 15, 2024

Choose a reason for hiding this comment

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

I am not sure about this one. I run bundle exec appraisal install while using ruby 3.1.5p252 (2024-04-23 revision 1945f8dc0e) [x86_64-linux] and I get gem "pg", "~> 1.2.0"

However, on rails_7_0.gemfile, it uses
gem "pg", "~> 1.3.0"

Tried running it with other higher versions of ruby but always get this error:

Installing pg 1.2.3 with native extensions
Gem::Ext::BuildError: ERROR: Failed to build gem native extension.

Copy link
Owner

Choose a reason for hiding this comment

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

I've found that you can use this line to help with that installation:

bundle config build.pg --with-cflags="-Wno-error=implicit-function-declaration"

However, the whole appraisal testing thing is a real pain. I was able to do it a different way with my other gem:

https://github.com/brendon/positioning/blob/main/.github/workflows/main.yml
https://github.com/brendon/positioning/blob/main/Gemfile

It's then easy to set up a matrix of tests across Rails, Ruby, and database libraries and only pick the compatible ones.

The downside is that this doesn't run well locally. If you want to try this way, by all means, but I realise it's quite an extra job in and of itself :) If the compile line above gets things working for you then just carry on I think :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
I have tried a bit of everything but without much success. I have also tried testing the gem from a clear Dockerfile and also got the same error about installing 'pg' gem. The only solution I can think of is bumping 'pg' version to "~> 1.3.0" but I guess that make the gem not compatible anymore with older versions of Ruby, so that might not be an option in this case.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it might be time to drop support for older rails/ruby versions. Did you first want me to have a go at getting rid of the appraisals gem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would be great. I tried that too but I failed, since I don't know very well how that should be set up.

Copy link
Owner

Choose a reason for hiding this comment

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

That's all good. It's fairly complicated. I've done those changes on master not and the tests pass. It just tests Rails 6.1+ and Ruby 3.0+:

https://github.com/brendon/acts_as_list/actions/runs/9310848684

Feel free to update from master and then try out the tests. You can control the Rails, Ruby and DB versions via the command line:

RAILS_VERSION=7.1 (for example)
DB=sqlite | mysql | postgresql
Ruby: You just need to use rbenv or something like that and change your .ruby-version file.

Appraisals was good but a lot of work to maintain and people also used to not understand it and just edit the automatically generated bundler files :D

Copy link
Owner

Choose a reason for hiding this comment

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

You'll need to run bundle for each rails version you want to use:

RAILS_VERSION=7.1 bundle
RAILS_VERSION=7.1 DB=postgresql bundle exec rake (to run the tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect! That worked well. I was able to successfully run the tests both locally and through the CI on my fork.

@divagueame divagueame requested a review from brendon May 15, 2024 12:04
@divagueame divagueame force-pushed the Add-composite-primary-key-support branch from 685340b to 4645acf Compare May 31, 2024 13:32
@divagueame divagueame force-pushed the Add-composite-primary-key-support branch from 0201a24 to 8ecc8b0 Compare May 31, 2024 14:32
Copy link
Owner

@brendon brendon left a comment

Choose a reason for hiding this comment

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

This is all looking really good. Are you able to give me a quick overview of the tests? I've tried to quickly understand them but there's quite a lot of change there. Here's what I think you've done:

  • You've modified the SharedList tests to be able to cope with an iteration where the keys are composite. It looks like you just set both keys to be the same integer each time for convenience?
  • You've kept the second dedicated suite of tests from the original PR.

Let me know if I got it wrong :)

@divagueame
Copy link
Contributor Author

divagueame commented May 31, 2024

This is all looking really good. Are you able to give me a quick overview of the tests? I've tried to quickly understand them but there's quite a lot of change there. Here's what I think you've done:

  • You've modified the SharedList tests to be able to cope with an iteration where the keys are composite. It looks like you just set both keys to be the same integer each time for convenience?
  • You've kept the second dedicated suite of tests from the original PR.

Let me know if I got it wrong :)

I think you got quite right.
I adjusted shared_list, so that it can be reused with a model different than ListMixin,

test/shared_list.rb

def setup(mixin = ListMixin, mixinError = ListMixinError, id = :id)
...

It can be now used overriding the default Model (ListMixin) used for those test runs, like this:

test/test_list.rb

    def setup
      setup_db

      super(CompositePrimaryKeyListScoped, CompositePrimaryKeyListScopedError, :first_id)
    end

On those iterations, :first_id and :second_id will be the same for the sake of simplicity.

I left the tests of the original PR in which the model does not use scoped lists. I guess it could be possible to refactor Shared::List to account for those cases, however it would need to be further refactored adding more abstraction. I tried to test the main documented APIs from the Readme, so now I'm thinking that maybe it could be improved. Not sure. Let me know what you think about it.

@divagueame divagueame closed this May 31, 2024
@divagueame divagueame reopened this May 31, 2024
@brendon
Copy link
Owner

brendon commented Jun 1, 2024

I think as long as you're running the shared tests with composite keys and have covered off anything unique to composite key tests in the other suite then we're good to go. Let me know if you're happy with that decision and I'll merge :)

Do you need an immediate new version released?

@divagueame
Copy link
Contributor Author

I think it's good like it is. I would say let's merge it like that.
I don't personally need a version released immediately. Just do it as usual. Not really sure how that works : )

@brendon
Copy link
Owner

brendon commented Jun 1, 2024

All good. The project only seldom received updates so I'll release a new version. Are you able to add a changelog entry for me in the Unreleased section?

@divagueame
Copy link
Contributor Author

Sure, just added it to the changelog.

@brendon brendon merged commit 58a4972 into brendon:master Jun 2, 2024
27 checks passed
@brendon
Copy link
Owner

brendon commented Jun 2, 2024

Thanks @divagueame, this is released as 1.2.0. Looking forward to working with you on this project again in the future.

@brendon
Copy link
Owner

brendon commented Aug 2, 2024

Hi @divagueame, just a heads up, in the process of adding this feature to my positioning gem I realised that Rails handles a lot of the overhead of composite primary keys for us so the implementation here can be simplified. I've committed some changes to master if you're interested :)

@divagueame
Copy link
Contributor Author

Sweet! That looks neat. Thanks for the update. : )

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