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.

Sign up for GitHub

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

Jump to bottom

Add composite primary key support #430

Merged
merged 12 commits into from
Jun 2, 2024
Merged

Add composite primary key support #430

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f282b8f
Add tests for CompositePrimaryKeyList
divagueame May 12, 2024
8971888
Adjust query when instance has a composite primary index
divagueame May 12, 2024
39c3e0a
Fix version tested for composite primary key
divagueame May 12, 2024
3a6c180
Update primary_key_condition and use like where.not()
divagueame May 14, 2024
ebdeb62
Add rails_7_1.gemfile + GH Actions Matrix
divagueame May 14, 2024
544d855
Update primary_key_condition
divagueame May 14, 2024
9aed3f5
Refactor shared_list module
divagueame May 15, 2024
1fe83bb
Merge branch 'master' into Add-composite-primary-key-support
divagueame May 31, 2024
115ab4b
Remove ci.yml
divagueame May 31, 2024
4645acf
Clean previously create rails_7_1.gemfile
divagueame May 31, 2024
8ecc8b0
Only create with id if it's composite primary key
divagueame May 31, 2024
0d3a71b
Add Changelog entry
divagueame Jun 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ jobs:
- gemfiles/rails_6_0.gemfile
- gemfiles/rails_6_1.gemfile
- gemfiles/rails_7_0.gemfile
- gemfiles/rails_7_1.gemfile
db:
- sqlite
- mysql
Expand All @@ -40,6 +41,12 @@ jobs:
gemfile: gemfiles/rails_7_0.gemfile
- ruby: 2.6
gemfile: gemfiles/rails_7_0.gemfile
- ruby: 2.4
gemfile: gemfiles/rails_7_1.gemfile
- ruby: 2.5
gemfile: gemfiles/rails_7_1.gemfile
- ruby: 2.6
gemfile: gemfiles/rails_7_1.gemfile
- ruby: 2.7
gemfile: gemfiles/rails_4_2.gemfile
- ruby: 2.7
Expand Down
31 changes: 31 additions & 0 deletions gemfiles/rails_7_1.gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# This file was generated by Appraisal

source "http://rubygems.org"

gem "rake"
gem "appraisal"
gem "activerecord", "~> 7.1.0"

group :development do
gem "github_changelog_generator", "1.9.0"
end

group :test do
gem "minitest", "~> 5.0"
gem "timecop"
gem "mocha"
end

group :sqlite do
gem "sqlite3", "~> 1.4"
end

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

@divagueame divagueame May 15, 2024 •
edited
Loading

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.

end

group :mysql do
gem "mysql2", "~> 0.5.0"
end

gemspec path: "../"
15 changes: 10 additions & 5 deletions lib/acts_as_list/active_record/acts/list.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def higher_items(limit=nil)
limit ||= acts_as_list_list.count
acts_as_list_list.
where("#{quoted_position_column_with_table_name} <= ?", current_position).
where("#{quoted_table_name}.#{self.class.primary_key} != ?", self.send(self.class.primary_key)).
where.not(primary_key_condition).
reorder(acts_as_list_order_argument(:desc)).
limit(limit)
end
Expand All @@ -188,7 +188,7 @@ def lower_items(limit=nil)
limit ||= acts_as_list_list.count
acts_as_list_list.
where("#{quoted_position_column_with_table_name} >= ?", current_position).
where("#{quoted_table_name}.#{self.class.primary_key} != ?", self.send(self.class.primary_key)).
where.not(primary_key_condition).
reorder(acts_as_list_order_argument(:asc)).
limit(limit)
end
Expand Down Expand Up @@ -273,7 +273,7 @@ def bottom_item(except = nil)
scope = acts_as_list_list

if except
scope = scope.where("#{quoted_table_name}.#{self.class.primary_key} != ?", except.id)
scope = scope.where.not(primary_key_condition(except.id))
end

scope.in_list.reorder(acts_as_list_order_argument(:desc)).first
Expand All @@ -300,7 +300,7 @@ def increment_positions_on_lower_items(position, avoid_id = nil)
scope = acts_as_list_list

if avoid_id
scope = scope.where("#{quoted_table_name}.#{self.class.primary_key} != ?", avoid_id)
scope = scope.where.not(primary_key_condition(avoid_id))
end

if sequential_updates?
Expand Down Expand Up @@ -341,7 +341,7 @@ def shuffle_positions_on_intermediate_items(old_position, new_position, avoid_id
scope = acts_as_list_list

if avoid_id
scope = scope.where("#{quoted_table_name}.#{self.class.primary_key} != ?", avoid_id)
scope = scope.where.not(primary_key_condition(avoid_id))
end

if old_position < new_position
Expand Down Expand Up @@ -480,6 +480,11 @@ def active_record_version_is?(version_requirement)
version = Gem.loaded_specs['activerecord'].version
requirement.satisfied_by?(version)
end

def primary_key_condition(id = nil)
primary_keys = Array.wrap(self.class.primary_key)
id ? primary_keys.zip(Array.wrap(id)).to_h : slice(*primary_keys)
end
brendon marked this conversation as resolved.
Show resolved Hide resolved
end

end
Expand Down
Loading
Loading