From 6a2176f7d92c4818f1a1f572aed339b39aef087e Mon Sep 17 00:00:00 2001 From: Sundus Yousuf Date: Tue, 8 Oct 2024 13:54:39 -0500 Subject: [PATCH] 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 --- CHANGELOG.md | 2 + lib/chewy/strategy/delayed_sidekiq/worker.rb | 2 +- spec/chewy/strategy/delayed_sidekiq_spec.rb | 37 ++++++++++++++----- spec/support/active_record.rb | 39 ++++++++++++++++++-- 4 files changed, 65 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c27c85fa..a7963c22f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/chewy/strategy/delayed_sidekiq/worker.rb b/lib/chewy/strategy/delayed_sidekiq/worker.rb index af5fa793d..7b539c3ca 100644 --- a/lib/chewy/strategy/delayed_sidekiq/worker.rb +++ b/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 diff --git a/spec/chewy/strategy/delayed_sidekiq_spec.rb b/spec/chewy/strategy/delayed_sidekiq_spec.rb index a4a8fdd09..03ead7755 100644 --- a/spec/chewy/strategy/delayed_sidekiq_spec.rb +++ b/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 diff --git a/spec/support/active_record.rb b/spec/support/active_record.rb index d081e5be4..e64660563 100644 --- a/spec/support/active_record.rb +++ b/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|