Merge branch '325689-delete-all-label-links-async' into 'master'

Delete all label links asynchronously when issuable gets destroyed [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!60487

app/models/concerns/issuable.rb

@@ -63,7 +63,7 @@ module Issuable
 
     has_many :note_authors, -> { distinct }, through: :notes, source: :author
 
-    has_many :label_links, as: :target, dependent: :destroy, inverse_of: :target # rubocop:disable Cop/ActiveRecordDependent
+    has_many :label_links, as: :target, inverse_of: :target
     has_many :labels, through: :label_links
     has_many :todos, as: :target
 

app/models/label_link.rb

@@ -9,4 +9,6 @@ class LabelLink < ApplicationRecord
 
   validates :target, presence: true, unless: :importing?
   validates :label, presence: true, unless: :importing?
+
+  scope :for_target, -> (target_id, target_type) { where(target_id: target_id, target_type: target_type) }
 end

app/services/issuable/destroy_label_links_service.rb

+# frozen_string_literal: true
+
+module Issuable
+  class DestroyLabelLinksService
+    BATCH_SIZE = 100
+
+    def initialize(target_id, target_type)
+      @target_id = target_id
+      @target_type = target_type
+    end
+
+    def execute
+      inner_query =
+        LabelLink
+          .select(:id)
+          .for_target(target_id, target_type)
+          .limit(BATCH_SIZE)
+
+      delete_query = <<~SQL
+      DELETE FROM "#{LabelLink.table_name}"
+      WHERE id IN (#{inner_query.to_sql})
+      SQL
+
+      loop do
+        result = ActiveRecord::Base.connection.execute(delete_query)
+
+        break if result.cmd_tuples == 0
+      end
+    end
+
+    private
+
+    attr_reader :target_id, :target_type
+  end
+end

app/services/issuable/destroy_service.rb

@@ -3,15 +3,13 @@
 module Issuable
   class DestroyService < IssuableBaseService
     def execute(issuable)
-      if issuable.destroy
-        after_destroy(issuable)
-      end
+      after_destroy(issuable) if issuable.destroy
     end
 
     private
 
     def after_destroy(issuable)
-      delete_todos(issuable)
+      delete_associated_records(issuable)
       issuable.update_project_counter_caches
       issuable.assignees.each(&:invalidate_cache_counts)
     end
@@ -20,9 +18,14 @@ module Issuable
       issuable.resource_parent.group
     end
 
-    def delete_todos(issuable)
+    def delete_associated_records(issuable)
       actor = group_for(issuable)
 
+      delete_todos(actor, issuable)
+      delete_label_links(actor, issuable)
+    end
+
+    def delete_todos(actor, issuable)
       if Feature.enabled?(:destroy_issuable_todos_async, actor, default_enabled: :yaml)
         TodosDestroyer::DestroyedIssuableWorker
           .perform_async(issuable.id, issuable.class.name)
@@ -32,6 +35,17 @@ module Issuable
           .perform(issuable.id, issuable.class.name)
       end
     end
+
+    def delete_label_links(actor, issuable)
+      if Feature.enabled?(:destroy_issuable_label_links_async, actor, default_enabled: :yaml)
+        Issuable::LabelLinksDestroyWorker
+          .perform_async(issuable.id, issuable.class.name)
+      else
+        Issuable::LabelLinksDestroyWorker
+          .new
+          .perform(issuable.id, issuable.class.name)
+      end
+    end
   end
 end
 

app/workers/all_queues.yml

@@ -2200,6 +2200,15 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: issuable_label_links_destroy
+  :worker_name: Issuable::LabelLinksDestroyWorker
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: issuables_clear_groups_issue_counter
   :worker_name: Issuables::ClearGroupsIssueCounterWorker
   :feature_category: :issue_tracking

app/workers/issuable/label_links_destroy_worker.rb

+# frozen_string_literal: true
+
+module Issuable
+  class LabelLinksDestroyWorker
+    include ApplicationWorker
+
+    idempotent!
+    feature_category :issue_tracking
+
+    def perform(target_id, target_type)
+      ::Issuable::DestroyLabelLinksService.new(target_id, target_type).execute
+    end
+  end
+end

changelogs/unreleased/325689-delete-all-label-links-async.yml

+---
+title: Delete all label links asynchronously when issuable gets destroyed
+merge_request: 60487
+author:
+type: performance

config/feature_flags/development/destroy_issuable_label_links_async.yml

+---
+name: destroy_issuable_label_links_async
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60487
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/325689
+milestone: '13.12'
+type: development
+group: group::code review
+default_enabled: false

config/sidekiq_queues.yml

@@ -194,6 +194,8 @@
   - 1
 - - issuable_export_csv
   - 1
+- - issuable_label_links_destroy
+  - 1
 - - issuables_clear_groups_issue_counter
   - 1
 - - issue_placement

ee/spec/services/ee/issuable/destroy_service_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe Issuable::DestroyService do
       end
 
       it_behaves_like 'service deleting todos'
+      it_behaves_like 'service deleting label links'
     end
 
     context 'when destroying other issuable type' do

ee/spec/services/issuable/destroy_label_links_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issuable::DestroyLabelLinksService do
+  describe '#execute' do
+    let_it_be(:target) { create(:epic) }
+
+    it_behaves_like 'service deleting label links of an issuable'
+  end
+end

spec/models/label_link_spec.rb

@@ -12,4 +12,15 @@ RSpec.describe LabelLink do
     let(:valid_items_for_bulk_insertion) { build_list(:label_link, 10) }
     let(:invalid_items_for_bulk_insertion) { [] } # class does not have any validations defined
   end
+
+  describe '.for_target' do
+    it 'returns the label links for a given target' do
+      label_link = create(:label_link, target: create(:merge_request))
+
+      create(:label_link, target: create(:issue))
+
+      expect(described_class.for_target(label_link.target_id, label_link.target_type))
+        .to contain_exactly(label_link)
+    end
+  end
 end

spec/services/issuable/destroy_label_links_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issuable::DestroyLabelLinksService do
+  describe '#execute' do
+    context 'when target is an Issue' do
+      let_it_be(:target) { create(:issue) }
+
+      it_behaves_like 'service deleting label links of an issuable'
+    end
+
+    context 'when target is a MergeRequest' do
+      let_it_be(:target) { create(:merge_request) }
+
+      it_behaves_like 'service deleting label links of an issuable'
+    end
+  end
+end

spec/services/issuable/destroy_service_spec.rb

@@ -31,6 +31,10 @@ RSpec.describe Issuable::DestroyService do
       it_behaves_like 'service deleting todos' do
         let(:issuable) { issue }
       end
+
+      it_behaves_like 'service deleting label links' do
+        let(:issuable) { issue }
+      end
     end
 
     context 'when issuable is a merge request' do
@@ -54,6 +58,10 @@ RSpec.describe Issuable::DestroyService do
       it_behaves_like 'service deleting todos' do
         let(:issuable) { merge_request }
       end
+
+      it_behaves_like 'service deleting label links' do
+        let(:issuable) { merge_request }
+      end
     end
   end
 end

spec/support/shared_examples/services/destroy_label_links_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples_for 'service deleting label links of an issuable' do
+  let_it_be(:label_link) { create(:label_link, target: target) }
+
+  def execute
+    described_class.new(target.id, target.class.name).execute
+  end
+
+  it 'deletes label links for specified target ID and type' do
+    control_count = ActiveRecord::QueryRecorder.new { execute }.count
+
+    # Create more label links for the target
+    create(:label_link, target: target)
+    create(:label_link, target: target)
+
+    expect { execute }.not_to exceed_query_limit(control_count)
+    expect(target.reload.label_links.count).to eq(0)
+  end
+end

spec/support/shared_examples/services/issuable/destroy_service_shared_examples.rb

@@ -29,3 +29,33 @@ shared_examples_for 'service deleting todos' do
     end
   end
 end
+
+shared_examples_for 'service deleting label links' do
+  before do
+    stub_feature_flags(destroy_issuable_label_links_async: group)
+  end
+
+  it 'destroys associated label links asynchronously' do
+    expect(Issuable::LabelLinksDestroyWorker)
+      .to receive(:perform_async)
+      .with(issuable.id, issuable.class.name)
+
+    subject.execute(issuable)
+  end
+
+  context 'when destroy_issuable_label_links_async feature is disabled for group' do
+    before do
+      stub_feature_flags(destroy_issuable_label_links_async: false)
+    end
+
+    it 'destroy associated label links synchronously' do
+      expect_next_instance_of(Issuable::LabelLinksDestroyWorker) do |worker|
+        expect(worker)
+          .to receive(:perform)
+          .with(issuable.id, issuable.class.name)
+      end
+
+      subject.execute(issuable)
+    end
+  end
+end

spec/workers/issuable/label_links_destroy_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issuable::LabelLinksDestroyWorker do
+  let(:job_args) { [1, 'MergeRequest'] }
+  let(:service) { double }
+
+  include_examples 'an idempotent worker' do
+    it 'calls the Issuable::DestroyLabelLinksService' do
+      expect(::Issuable::DestroyLabelLinksService).to receive(:new).twice.and_return(service)
+      expect(service).to receive(:execute).twice
+
+      subject
+    end
+  end
+end