From 5b24d6c8514e191471ca79d61d050a3dbb70c08b Mon Sep 17 00:00:00 2001 From: Ivanov-Anton Date: Sat, 21 Oct 2023 01:23:24 +0300 Subject: [PATCH] #1276, Refactor AsyncBatchDestroyJob to increase efficiency and resilience The AsyncBatchDestroyJob has been refactored to utilise Rails's `find_in_batches` method as opposed to a custom SQL query. This proves to make the process of record deletion both more efficient and more resilient to database inconsistencies. Previously, records that could not be deleted due to dependencies would raise an exception, breaking the whole transaction. Now, these records are logged and skipped over instead. Updates to associated tests have been made to verify and reflect these changes. --- app/jobs/async_batch_destroy_job.rb | 28 +++++++---- spec/jobs/async_batch_destroy_job_spec.rb | 58 ++++++++++++++++++----- spec/models/routing/numberlist_spec.rb | 13 +++++ 3 files changed, 77 insertions(+), 22 deletions(-) diff --git a/app/jobs/async_batch_destroy_job.rb b/app/jobs/async_batch_destroy_job.rb index 949408dbf..ceaf21d6c 100644 --- a/app/jobs/async_batch_destroy_job.rb +++ b/app/jobs/async_batch_destroy_job.rb @@ -11,18 +11,28 @@ def perform(model_class, sql_query, who_is) @model_class = model_class.constantize @who_is = who_is set_audit_log_data - begin - scoped_records = @model_class.find_by_sql(sql_query + " LIMIT #{BATCH_SIZE}") - @model_class.transaction do - scoped_records.each do |record| - raise ActiveRecord::RecordNotDestroyed.new(error_message_for(record), record) unless record.destroy - end + + @model_class.where(id: record_ids(sql_query)).find_in_batches(batch_size: BATCH_SIZE) do |records| + records.each do |record| + delete_record(record) end - end until scoped_records.empty? + end + end + + def record_ids(sql_query) + @model_class.find_by_sql(sql_query).pluck(:id) + end + + def delete_record(record) + unless record.destroy + Rails.logger.warn "#{@model_class} ##{record.id} can't be deleted: #{record.errors.full_messages.to_sentence}" + end + rescue ActiveRecord::InvalidForeignKey => e + Rails.logger.error error_message_for(id: record.id, error_class: e.class) end - def error_message_for(record) - "#{model_class} ##{record.id} can't be deleted: #{record.errors.full_messages.to_sentence}" + def error_message_for(id:, error_class:) + "#{@model_class} ##{id} can't be deleted: #{error_class}" end def reschedule_at(_time, _attempts) diff --git a/spec/jobs/async_batch_destroy_job_spec.rb b/spec/jobs/async_batch_destroy_job_spec.rb index 2b20916c5..177a90d6b 100644 --- a/spec/jobs/async_batch_destroy_job_spec.rb +++ b/spec/jobs/async_batch_destroy_job_spec.rb @@ -64,9 +64,9 @@ let(:sql_query) { Contractor.all.to_sql } it 'error performed job' do expect do - expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) + expect { subject }.not_to raise_error end.to change(LogicLog, :count).by 1 - expect(LogicLog.last.msg).to start_with 'Error' + expect(LogicLog.last.msg).to start_with 'Success' end end end @@ -74,23 +74,55 @@ context 'when record cannot be destroyed' do let(:model_class) { 'Dialpeer' } - let!(:dialpeers) { FactoryBot.create_list(:dialpeer, 4) } + let!(:dialpeer_with_pricelist) { FactoryBot.create(:dialpeer) } + let!(:dialpeer) { FactoryBot.create(:dialpeer) } let!(:item) do - FactoryBot.create(:rate_management_pricelist_item, :with_pricelist, :filed_from_project, dialpeer: dialpeers.last) + FactoryBot.create(:rate_management_pricelist_item, :with_pricelist, :filed_from_project, dialpeer: dialpeer_with_pricelist) end let(:sql_query) { Dialpeer.all.to_sql } + let(:error_message) { "Dialpeer ##{dialpeer_with_pricelist.id} can't be deleted: Can't be deleted because linked to not applied Rate Management Pricelist(s) ##{item.pricelist_id}" } + let(:rails_logger) { Rails.logger } - it 'should raise validation error' do - error_message = "Dialpeer ##{dialpeers.last.id} can't be deleted: Can't be deleted because linked to not applied Rate Management Pricelist(s) ##{item.pricelist_id}" - expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed, error_message) + before do + allow(Rails).to receive(:logger).and_return(rails_logger) + allow(rails_logger).to receive(:info) + end - dialpeers.first(2).each do |dialpeer| - expect(Dialpeer).not_to be_exists(dialpeer.id) - end + it 'should delete dialpeer that do not have related pricelist' do + expect { subject }.not_to raise_error + expect { dialpeer_with_pricelist.reload }.not_to raise_error ActiveRecord::RecordNotFound + expect { dialpeer.reload }.to raise_error ActiveRecord::RecordNotFound + end - dialpeers.last(2).each do |dialpeer| - expect(Dialpeer).to be_exists(dialpeer.id) - end + it 'should log error through Rails logger' do + expect(rails_logger).to receive(:warn).once.and_wrap_original { |_m, arg| expect(arg).to eq error_message } + subject + end + end + + context 'when Routing::Numberlist has related customer_auth' do + let(:model_class) { 'Routing::Numberlist' } + let(:sql_query) { Routing::Numberlist.where(id: [record_that_cant_be_deleted.id, record_that_can_be_deleted.id]).to_sql } + let!(:record_that_cant_be_deleted) { FactoryBot.create(:numberlist) } + let!(:record_that_can_be_deleted) { FactoryBot.create(:numberlist) } + let!(:customers_auth) { FactoryBot.create(:customers_auth, src_numberlist: record_that_cant_be_deleted) } + + it 'should remove number_lists without related Customer Auth' do + subject + + expect { record_that_can_be_deleted.reload }.to raise_error ActiveRecord::RecordNotFound + end + end + + context 'when NumberList already deleted' do + let(:model_class) { 'Routing::Numberlist' } + let(:sql_query) { Routing::Numberlist.all.to_sql } + let!(:record) { FactoryBot.create(:numberlist) } + + before { record.destroy! } + + it 'should perform job without error' do + expect { subject }.not_to raise_error end end end diff --git a/spec/models/routing/numberlist_spec.rb b/spec/models/routing/numberlist_spec.rb index d0385ef52..84b0c2c8c 100644 --- a/spec/models/routing/numberlist_spec.rb +++ b/spec/models/routing/numberlist_spec.rb @@ -149,4 +149,17 @@ end end end + + describe '.destroy!' do + subject { record.destroy! } + + context 'when record has related customer_auth' do + let!(:record) { FactoryBot.create(:numberlist) } + let!(:customers_auth) { FactoryBot.create(:customers_auth, src_numberlist: record) } + + it 'should destroy record' do + expect { subject }.to change(described_class, :count).by(-1) + end + end + end end