Fix id conversion issue in delayed_sidekiq strategy (#964)

* Fix id conversion issue in delayed_sidekiq strategy

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.

Move table creations to individual methods.

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.

* Add changelog notes

---------

Co-authored-by: Sundus Yousuf <sundus.yousuf@recruitmilitray.com>

CHANGELOG.md

@@ -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

lib/chewy/strategy/delayed_sidekiq/worker.rb

@@ -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

spec/chewy/strategy/delayed_sidekiq_spec.rb

@@ -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!) }
@@ -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
@@ -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])
@@ -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])
@@ -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'])
@@ -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
@@ -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
@@ -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

spec/support/active_record.rb

@@ -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
@@ -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
@@ -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
+
 module ActiveRecordClassHelpers
   extend ActiveSupport::Concern
 
@@ -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|