Merge branch 'fix_fetch_labels_query_on_project_transfer' into 'master'

Add a query optimisation for fetching group labels

See merge request gitlab-org/gitlab!73501

app/services/labels/transfer_service.rb

@@ -50,21 +50,32 @@ module Labels
 
     # rubocop: disable CodeReuse/ActiveRecord
     def group_labels_applied_to_issues
-      @group_labels_applied_to_issues ||= Label.joins(:issues)
-        .where(
-          issues: { project_id: project.id },
-          labels: { group_id: old_group.self_and_ancestors }
-        )
+      @labels_applied_to_issues ||= if use_optimized_group_labels_query?
+                                      Label.joins(:issues)
+                                        .joins("INNER JOIN namespaces on namespaces.id = labels.group_id AND namespaces.type = 'Group'" )
+                                        .where(issues: { project_id: project.id }).reorder(nil)
+                                    else
+                                      Label.joins(:issues).where(
+                                        issues: { project_id: project.id },
+                                        labels: { group_id: old_group.self_and_ancestors }
+                                      )
+                                    end
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
     # rubocop: disable CodeReuse/ActiveRecord
     def group_labels_applied_to_merge_requests
-      @group_labels_applied_to_merge_requests ||= Label.joins(:merge_requests)
-        .where(
-          merge_requests: { target_project_id: project.id },
-          labels: { group_id: old_group.self_and_ancestors }
-        )
+      @labels_applied_to_mrs ||= if use_optimized_group_labels_query?
+                                   Label.joins(:merge_requests)
+                                     .joins("INNER JOIN namespaces on namespaces.id = labels.group_id AND namespaces.type = 'Group'" )
+                                     .where(merge_requests: { target_project_id: project.id }).reorder(nil)
+                                 else
+                                   Label.joins(:merge_requests)
+                                     .where(
+                                       merge_requests: { target_project_id: project.id },
+                                       labels: { group_id: old_group.self_and_ancestors }
+                                     )
+                                 end
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -88,5 +99,9 @@ module Labels
         .update_all(label_id: new_label_id)
     end
     # rubocop: enable CodeReuse/ActiveRecord
+
+    def use_optimized_group_labels_query?
+      Feature.enabled?(:use_optimized_group_labels_query, project.root_namespace, default_enabled: :yaml)
+    end
   end
 end

config/feature_flags/development/use_optimized_group_labels_query.yml

+---
+name: use_optimized_group_labels_query
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73501
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344957
+milestone: '14.5'
+type: development
+group: group::workspace
+default_enabled: false

spec/services/labels/transfer_service_spec.rb

@@ -3,107 +3,121 @@
 require 'spec_helper'
 
 RSpec.describe Labels::TransferService do
-  describe '#execute' do
-    let_it_be(:user) { create(:user) }
+  shared_examples 'transfer labels' do
+    describe '#execute' do
+      let_it_be(:user) { create(:user) }
 
-    let_it_be(:old_group_ancestor) { create(:group) }
-    let_it_be(:old_group) { create(:group, parent: old_group_ancestor) }
+      let_it_be(:old_group_ancestor) { create(:group) }
+      let_it_be(:old_group) { create(:group, parent: old_group_ancestor) }
 
-    let_it_be(:new_group) { create(:group) }
+      let_it_be(:new_group) { create(:group) }
 
-    let_it_be(:project) { create(:project, :repository, group: new_group) }
+      let_it_be(:project) { create(:project, :repository, group: new_group) }
 
-    subject(:service) { described_class.new(user, old_group, project) }
+      subject(:service) { described_class.new(user, old_group, project) }
 
-    before do
-      old_group_ancestor.add_developer(user)
-      new_group.add_developer(user)
-    end
+      before do
+        old_group_ancestor.add_developer(user)
+        new_group.add_developer(user)
+      end
 
-    it 'recreates missing group labels at project level and assigns them to the issuables' do
-      old_group_label_1 = create(:group_label, group: old_group)
-      old_group_label_2 = create(:group_label, group: old_group)
+      it 'recreates missing group labels at project level and assigns them to the issuables' do
+        old_group_label_1 = create(:group_label, group: old_group)
+        old_group_label_2 = create(:group_label, group: old_group)
 
-      labeled_issue = create(:labeled_issue, project: project, labels: [old_group_label_1])
-      labeled_merge_request = create(:labeled_merge_request, source_project: project, labels: [old_group_label_2])
+        labeled_issue = create(:labeled_issue, project: project, labels: [old_group_label_1])
+        labeled_merge_request = create(:labeled_merge_request, source_project: project, labels: [old_group_label_2])
 
-      expect { service.execute }.to change(project.labels, :count).by(2)
-      expect(labeled_issue.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_label_1.title))
-      expect(labeled_merge_request.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_label_2.title))
-    end
+        expect { service.execute }.to change(project.labels, :count).by(2)
+        expect(labeled_issue.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_label_1.title))
+        expect(labeled_merge_request.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_label_2.title))
+      end
 
-    it 'recreates missing ancestor group labels at project level and assigns them to the issuables' do
-      old_group_ancestor_label_1 = create(:group_label, group: old_group_ancestor)
-      old_group_ancestor_label_2 = create(:group_label, group: old_group_ancestor)
+      it 'recreates missing ancestor group labels at project level and assigns them to the issuables' do
+        old_group_ancestor_label_1 = create(:group_label, group: old_group_ancestor)
+        old_group_ancestor_label_2 = create(:group_label, group: old_group_ancestor)
 
-      labeled_issue = create(:labeled_issue, project: project, labels: [old_group_ancestor_label_1])
-      labeled_merge_request = create(:labeled_merge_request, source_project: project, labels: [old_group_ancestor_label_2])
+        labeled_issue = create(:labeled_issue, project: project, labels: [old_group_ancestor_label_1])
+        labeled_merge_request = create(:labeled_merge_request, source_project: project, labels: [old_group_ancestor_label_2])
 
-      expect { service.execute }.to change(project.labels, :count).by(2)
-      expect(labeled_issue.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_ancestor_label_1.title))
-      expect(labeled_merge_request.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_ancestor_label_2.title))
-    end
+        expect { service.execute }.to change(project.labels, :count).by(2)
+        expect(labeled_issue.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_ancestor_label_1.title))
+        expect(labeled_merge_request.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_ancestor_label_2.title))
+      end
 
-    it 'recreates label priorities related to the missing group labels' do
-      old_group_label = create(:group_label, group: old_group)
-      create(:labeled_issue, project: project, labels: [old_group_label])
-      create(:label_priority, project: project, label: old_group_label, priority: 1)
+      it 'recreates label priorities related to the missing group labels' do
+        old_group_label = create(:group_label, group: old_group)
+        create(:labeled_issue, project: project, labels: [old_group_label])
+        create(:label_priority, project: project, label: old_group_label, priority: 1)
 
-      service.execute
+        service.execute
 
-      new_project_label = project.labels.find_by(title: old_group_label.title)
-      expect(new_project_label.id).not_to eq old_group_label.id
-      expect(new_project_label.priorities).not_to be_empty
-    end
+        new_project_label = project.labels.find_by(title: old_group_label.title)
+        expect(new_project_label.id).not_to eq old_group_label.id
+        expect(new_project_label.priorities).not_to be_empty
+      end
 
-    it 'does not recreate missing group labels that are not applied to issues or merge requests' do
-      old_group_label = create(:group_label, group: old_group)
+      it 'does not recreate missing group labels that are not applied to issues or merge requests' do
+        old_group_label = create(:group_label, group: old_group)
 
-      service.execute
+        service.execute
 
-      expect(project.labels.where(title: old_group_label.title)).to be_empty
-    end
+        expect(project.labels.where(title: old_group_label.title)).to be_empty
+      end
 
-    it 'does not recreate missing group labels that already exist in the project group' do
-      old_group_label = create(:group_label, group: old_group)
-      labeled_issue = create(:labeled_issue, project: project, labels: [old_group_label])
+      it 'does not recreate missing group labels that already exist in the project group' do
+        old_group_label = create(:group_label, group: old_group)
+        labeled_issue = create(:labeled_issue, project: project, labels: [old_group_label])
 
-      new_group_label = create(:group_label, group: new_group, title: old_group_label.title)
+        new_group_label = create(:group_label, group: new_group, title: old_group_label.title)
 
-      service.execute
+        service.execute
 
-      expect(project.labels.where(title: old_group_label.title)).to be_empty
-      expect(labeled_issue.reload.labels).to contain_exactly(new_group_label)
-    end
+        expect(project.labels.where(title: old_group_label.title)).to be_empty
+        expect(labeled_issue.reload.labels).to contain_exactly(new_group_label)
+      end
 
-    it 'updates only label links in the given project' do
-      old_group_label = create(:group_label, group: old_group)
-      other_project = create(:project, group: old_group)
+      it 'updates only label links in the given project' do
+        old_group_label = create(:group_label, group: old_group)
+        other_project = create(:project, group: old_group)
 
-      labeled_issue = create(:labeled_issue, project: project, labels: [old_group_label])
-      other_project_labeled_issue = create(:labeled_issue, project: other_project, labels: [old_group_label])
+        labeled_issue = create(:labeled_issue, project: project, labels: [old_group_label])
+        other_project_labeled_issue = create(:labeled_issue, project: other_project, labels: [old_group_label])
 
-      service.execute
+        service.execute
 
-      expect(labeled_issue.reload.labels).not_to include(old_group_label)
-      expect(other_project_labeled_issue.reload.labels).to contain_exactly(old_group_label)
-    end
+        expect(labeled_issue.reload.labels).not_to include(old_group_label)
+        expect(other_project_labeled_issue.reload.labels).to contain_exactly(old_group_label)
+      end
 
-    context 'when moving within the same ancestor group' do
-      let(:other_subgroup) { create(:group, parent: old_group_ancestor) }
-      let(:project) { create(:project, :repository, group: other_subgroup) }
+      context 'when moving within the same ancestor group' do
+        let(:other_subgroup) { create(:group, parent: old_group_ancestor) }
+        let(:project) { create(:project, :repository, group: other_subgroup) }
 
-      it 'does not recreate ancestor group labels' do
-        old_group_ancestor_label_1 = create(:group_label, group: old_group_ancestor)
-        old_group_ancestor_label_2 = create(:group_label, group: old_group_ancestor)
+        it 'does not recreate ancestor group labels' do
+          old_group_ancestor_label_1 = create(:group_label, group: old_group_ancestor)
+          old_group_ancestor_label_2 = create(:group_label, group: old_group_ancestor)
 
-        labeled_issue = create(:labeled_issue, project: project, labels: [old_group_ancestor_label_1])
-        labeled_merge_request = create(:labeled_merge_request, source_project: project, labels: [old_group_ancestor_label_2])
+          labeled_issue = create(:labeled_issue, project: project, labels: [old_group_ancestor_label_1])
+          labeled_merge_request = create(:labeled_merge_request, source_project: project, labels: [old_group_ancestor_label_2])
 
-        expect { service.execute }.not_to change(project.labels, :count)
-        expect(labeled_issue.reload.labels).to contain_exactly(old_group_ancestor_label_1)
-        expect(labeled_merge_request.reload.labels).to contain_exactly(old_group_ancestor_label_2)
+          expect { service.execute }.not_to change(project.labels, :count)
+          expect(labeled_issue.reload.labels).to contain_exactly(old_group_ancestor_label_1)
+          expect(labeled_merge_request.reload.labels).to contain_exactly(old_group_ancestor_label_2)
+        end
       end
     end
   end
+
+  context 'with use_optimized_group_labels_query FF on' do
+    it_behaves_like 'transfer labels'
+  end
+
+  context 'with use_optimized_group_labels_query FF off' do
+    before do
+      stub_feature_flags(use_optimized_group_labels_query: false)
+    end
+
+    it_behaves_like 'transfer labels'
+  end
 end