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

Fix id conversion issue in delayed_sidekiq strategy #964

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Fix id conversion issue in delayed_sidekiq strategy #964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
034d0a8
Fix id conversion issue in delayed_sidekiq strategy
Sep 4, 2024
1adf3bc
Add changelog notes
Sep 4, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### Bugs Fixed

* [#964](https://github.com/toptal/chewy/pull/964): Fix `delayed_sidekiq` worker to handle UUID primary keys correctly.

## 8.0.0-beta (2024-08-27)

### New Features
Expand Down
2 changes: 1 addition & 1 deletion lib/chewy/strategy/delayed_sidekiq/worker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def extract_ids_and_fields(members)

fields = nil if fields.include?(Scheduler::FALLBACK_FIELDS)

[ids.map(&:to_i), fields]
[ids, fields]
end
end
end
Expand Down
37 changes: 27 additions & 10 deletions spec/chewy/strategy/delayed_sidekiq_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,23 @@
update_index('cities') { self }
end

stub_uuid_model(:user) do
update_index('users') { self }
end

stub_index(:cities) do
index_scope City
end

stub_index(:users) do
index_scope User
end
end

let(:city) { City.create!(name: 'hello') }
let(:other_city) { City.create!(name: 'world') }
let(:user) { User.create!(name: 'John') }
let(:other_user) { User.create!(name: 'Jane') }

it 'does not trigger immediate reindex due to it`s async nature' do
expect { [city, other_city].map(&:save!) }
Expand All @@ -36,12 +46,19 @@

it "respects 'refresh: false' options" do
allow(Chewy).to receive(:disable_refresh_async).and_return(true)
expect(CitiesIndex).to receive(:import!).with(match_array([city.id, other_city.id]), refresh: false)
expect(CitiesIndex).to receive(:import!).with(match_array([city.id.to_s, other_city.id.to_s]), refresh: false)
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [city.id, other_city.id])
scheduler.postpone
Chewy::Strategy::DelayedSidekiq::Worker.drain
end

it 'works with models with string primary key' do
expect(UsersIndex).to receive(:import!).with(match_array([user.id, other_user.id]))
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(UsersIndex, [user.id, other_user.id])
scheduler.postpone
Chewy::Strategy::DelayedSidekiq::Worker.drain
end

context 'with default config' do
it 'does schedule a job that triggers reindex with default options' do
Timecop.freeze do
Expand Down Expand Up @@ -109,7 +126,7 @@ def expected_at_time
context 'two reindex call within the timewindow' do
it 'accumulates all ids does the reindex one time' do
Timecop.freeze do
expect(CitiesIndex).to receive(:import!).with(match_array([city.id, other_city.id])).once
expect(CitiesIndex).to receive(:import!).with(match_array([city.id.to_s, other_city.id.to_s])).once
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [city.id])
scheduler.postpone
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [other_city.id])
Expand All @@ -121,7 +138,7 @@ def expected_at_time
context 'one call with update_fields another one without update_fields' do
it 'does reindex of all fields' do
Timecop.freeze do
expect(CitiesIndex).to receive(:import!).with(match_array([city.id, other_city.id])).once
expect(CitiesIndex).to receive(:import!).with(match_array([city.id.to_s, other_city.id.to_s])).once
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [city.id], update_fields: ['name'])
scheduler.postpone
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [other_city.id])
Expand All @@ -134,7 +151,7 @@ def expected_at_time
context 'both calls with different update fields' do
it 'deos reindex with union of fields' do
Timecop.freeze do
expect(CitiesIndex).to receive(:import!).with(match_array([city.id, other_city.id]), update_fields: match_array(%w[name description])).once
expect(CitiesIndex).to receive(:import!).with(match_array([city.id.to_s, other_city.id.to_s]), update_fields: match_array(%w[name description])).once
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [city.id], update_fields: ['name'])
scheduler.postpone
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [other_city.id], update_fields: ['description'])
Expand All @@ -148,8 +165,8 @@ def expected_at_time
context 'two calls within different timewindows' do
it 'does two separate reindexes' do
Timecop.freeze do
expect(CitiesIndex).to receive(:import!).with([city.id]).once
expect(CitiesIndex).to receive(:import!).with([other_city.id]).once
expect(CitiesIndex).to receive(:import!).with([city.id.to_s]).once
expect(CitiesIndex).to receive(:import!).with([other_city.id.to_s]).once
Timecop.travel(20.seconds.ago) do
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [city.id])
scheduler.postpone
Expand All @@ -164,8 +181,8 @@ def expected_at_time
context 'first call has update_fields' do
it 'does first reindex with the expected update_fields and second without update_fields' do
Timecop.freeze do
expect(CitiesIndex).to receive(:import!).with([city.id], update_fields: ['name']).once
expect(CitiesIndex).to receive(:import!).with([other_city.id]).once
expect(CitiesIndex).to receive(:import!).with([city.id.to_s], update_fields: ['name']).once
expect(CitiesIndex).to receive(:import!).with([other_city.id.to_s]).once
Timecop.travel(20.seconds.ago) do
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [city.id], update_fields: ['name'])
scheduler.postpone
Expand All @@ -180,8 +197,8 @@ def expected_at_time
context 'both calls have update_fields option' do
it 'does both reindexes with their expected update_fields option' do
Timecop.freeze do
expect(CitiesIndex).to receive(:import!).with([city.id], update_fields: ['name']).once
expect(CitiesIndex).to receive(:import!).with([other_city.id], update_fields: ['description']).once
expect(CitiesIndex).to receive(:import!).with([city.id.to_s], update_fields: ['name']).once
expect(CitiesIndex).to receive(:import!).with([other_city.id.to_s], update_fields: ['description']).once
Timecop.travel(20.seconds.ago) do
scheduler = Chewy::Strategy::DelayedSidekiq::Scheduler.new(CitiesIndex, [city.id], update_fields: ['name'])
scheduler.postpone
Expand Down
39 changes: 35 additions & 4 deletions spec/support/active_record.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,16 @@
ActiveRecord::Base.raise_in_transactional_callbacks = true
end

ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS 'countries'")
ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS 'cities'")
ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS 'locations'")
ActiveRecord::Schema.define do
def create_countries_table
create_table :countries do |t|
t.column :name, :string
t.column :country_code, :string
t.column :rating, :integer
t.column :updated_at, :datetime
end
end

def create_cities_table
create_table :cities do |t|
t.column :country_id, :integer
t.column :name, :string
Expand All @@ -25,13 +24,17 @@
t.column :rating, :integer
t.column :updated_at, :datetime
end
end

def create_locations_table
create_table :locations do |t|
t.column :city_id, :integer
t.column :lat, :string
t.column :lon, :string
end
end

def create_comments_table
create_table :comments do |t|
t.column :content, :string
t.column :comment_type, :string
Expand All @@ -40,6 +43,26 @@
end
end

def create_users_table
create_table :users, id: false do |t|
t.column :id, :string
t.column :name, :string
end
end

ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS 'countries'")
ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS 'cities'")
ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS 'locations'")
ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS 'comments'")
ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS 'users'")
ActiveRecord::Schema.define do
create_countries_table
create_cities_table
create_locations_table
create_comments_table
create_users_table
end
Copy link

Choose a reason for hiding this comment

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

could elaborate why this is needed? Simply outdated?

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 needed a new table with uuid to test my changes so I created the users table. Then having every table creation inside a single block casued Rubocop Metrics/BlockLength error. To fix it I moved each table creation to an individual method.

Copy link

Choose a reason for hiding this comment

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

you could simply disable rubocop rule, but thanks for slight refactoring 👍


module ActiveRecordClassHelpers
extend ActiveSupport::Concern

Expand Down Expand Up @@ -73,6 +96,14 @@ def expects_no_query(except: nil, &block)
def stub_model(name, superclass = nil, &block)
stub_class(name, superclass || ActiveRecord::Base, &block)
end

def stub_uuid_model(name, superclass = nil, &block)
stub_class(name, superclass || ActiveRecord::Base) do
before_create { self.id = SecureRandom.uuid }
define_singleton_method(:primary_key, -> { 'id' })
class_eval(&block)
end
end
end

RSpec.configure do |config|
Expand Down