Skip to content

Commit

Permalink
Fix id conversion issue in delayed_sidekiq strategy
Browse files Browse the repository at this point in the history
Ensure ids extracted from Redis remain strings, preventing UUID issues.
Previously, ids were being converted to integers, causing problems
with UUIDs in the `delayed_sidekiq` strategy.

This update also enhances the test suite:
- Existing tests are updated.
- A new test ensures the issue is resolved.

Due to SQLite's lack of UUID support, a `stub_uuid_model` method is
added. This method stubs models with UUIDs, using `SecureRandom.uuid`
for the primary key.
  • Loading branch information
Sundus Yousuf committed Sep 4, 2024
1 parent d2c38f7 commit 92b9806
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 11 deletions.
2 changes: 1 addition & 1 deletion lib/chewy/strategy/delayed_sidekiq/worker.rb
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
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
13 changes: 13 additions & 0 deletions spec/support/active_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@
t.column :commented_id, :integer
t.column :updated_at, :datetime
end

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

module ActiveRecordClassHelpers
Expand Down Expand Up @@ -73,6 +78,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

0 comments on commit 92b9806

Please sign in to comment.