Merge branch 'optimized-issuable-label-search-pt1' into 'master'

Optimized issuable label search

See merge request gitlab-org/gitlab!34503

app/finders/issuable_finder.rb

@@ -37,6 +37,7 @@ class IssuableFinder
   include FinderMethods
   include CreatedAtFilter
   include Gitlab::Utils::StrongMemoize
+  prepend OptimizedIssuableLabelFilter
 
   requires_cross_project_access unless: -> { params.project? }
 

app/finders/labels_finder.rb

@@ -172,7 +172,14 @@ class LabelsFinder < UnionFinder
                   ProjectsFinder.new(params: { non_archived: true }, current_user: current_user).execute # rubocop: disable CodeReuse/Finder
                 end
 
-    @projects = @projects.in_namespace(group.id) if group?
+    if group?
+      @projects = if params[:include_subgroups]
+                    @projects.in_namespace(group.self_and_descendants.select(:id))
+                  else
+                    @projects.in_namespace(group.id)
+                  end
+    end
+
     @projects = @projects.where(id: params[:project_ids]) if projects?
     @projects = @projects.reorder(nil)
 

app/models/concerns/optimized_issuable_label_filter.rb

+# frozen_string_literal: true
+
+module OptimizedIssuableLabelFilter
+  def by_label(items)
+    return items unless params.labels?
+
+    return super if Feature.disabled?(:optimized_issuable_label_filter)
+
+    target_model = items.model
+
+    if params.filter_by_no_label?
+      items.where('NOT EXISTS (?)', optimized_any_label_query(target_model))
+    elsif params.filter_by_any_label?
+      items.where('EXISTS (?)', optimized_any_label_query(target_model))
+    else
+      issuables_with_selected_labels(items, target_model)
+    end
+  end
+
+  # Taken from IssuableFinder
+  def count_by_state
+    return super if root_namespace.nil?
+    return super if Feature.disabled?(:optimized_issuable_label_filter)
+
+    count_params = params.merge(state: nil, sort: nil, force_cte: true)
+    finder = self.class.new(current_user, count_params)
+
+    state_counts = finder
+      .execute
+      .reorder(nil)
+      .group(:state_id)
+      .count
+
+    counts = state_counts.transform_keys { |key| count_key(key) }
+
+    counts[:all] = counts.values.sum
+    counts.with_indifferent_access
+  end
+
+  private
+
+  def issuables_with_selected_labels(items, target_model)
+    if root_namespace
+      all_label_ids = find_label_ids(root_namespace)
+      # Found less labels in the DB than we were searching for. Return nothing.
+      return items.none if all_label_ids.size != params.label_names.size
+
+      all_label_ids.each do |label_ids|
+        items = items.where('EXISTS (?)', optimized_label_query_by_label_ids(target_model, label_ids))
+      end
+    else
+      params.label_names.each do |label_name|
+        items = items.where('EXISTS (?)', optimized_label_query_by_label_name(target_model, label_name))
+      end
+    end
+
+    items
+  end
+
+  def find_label_ids(root_namespace)
+    finder_params = {
+      include_subgroups: true,
+      include_ancestor_groups: true,
+      include_descendant_groups: true,
+      group: root_namespace,
+      title: params.label_names
+    }
+
+    LabelsFinder
+      .new(nil, finder_params)
+      .execute(skip_authorization: true)
+      .pluck(:title, :id)
+      .group_by(&:first)
+      .values
+      .map { |labels| labels.map(&:last) }
+  end
+
+  def root_namespace
+    strong_memoize(:root_namespace) do
+      (params.project || params.group)&.root_ancestor
+    end
+  end
+
+  def optimized_any_label_query(target_model)
+    LabelLink
+      .where(target_type: target_model.name)
+      .where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
+      .limit(1)
+  end
+
+  def optimized_label_query_by_label_ids(target_model, label_ids)
+    LabelLink
+      .where(target_type: target_model.name)
+      .where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
+      .where(label_id: label_ids)
+      .limit(1)
+  end
+
+  def optimized_label_query_by_label_name(target_model, label_name)
+    LabelLink
+      .joins(:label)
+      .where(target_type: target_model.name)
+      .where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
+      .where(labels: { name: label_name })
+      .limit(1)
+  end
+end

changelogs/unreleased/optimized-issuable-label-search-pt1.yml

+---
+title: Add indexes to `label_links` database table
+merge_request: 34503
+author:
+type: other

config/feature_flags/development/optimized_issuable_label_filter.yml

+---
+name: optimized_issuable_label_filter
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34503
+rollout_issue_url: 
+group: group::analytics
+type: development
+default_enabled: false

db/migrate/20200629134747_add_extra_index_to_label_links.rb

+# frozen_string_literal: true
+
+class AddExtraIndexToLabelLinks < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_COVERING_ALL_COLUMNS = 'index_on_label_links_all_columns'
+  INDEX_TO_REPLACE = 'index_label_links_on_label_id'
+  NEW_INDEX = 'index_label_links_on_label_id_and_target_type'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :label_links, [:target_id, :label_id, :target_type], name: INDEX_COVERING_ALL_COLUMNS
+
+    add_concurrent_index :label_links, [:label_id, :target_type], name: NEW_INDEX
+    remove_concurrent_index_by_name(:label_links, INDEX_TO_REPLACE)
+  end
+
+  def down
+    remove_concurrent_index_by_name(:label_links, INDEX_COVERING_ALL_COLUMNS)
+
+    add_concurrent_index(:label_links, :label_id, name: INDEX_TO_REPLACE)
+    remove_concurrent_index_by_name(:label_links, NEW_INDEX)
+  end
+end

db/schema_migrations/20200629134747

+9cd0e15dd2c5e70e53fc154a47a76ec066c741b5f6d148972b96d23888f0fcd4
\ No newline at end of file

db/structure.sql

@@ -20123,7 +20123,7 @@ CREATE INDEX index_keys_on_user_id ON public.keys USING btree (user_id);
 
 CREATE UNIQUE INDEX index_kubernetes_namespaces_on_cluster_project_environment_id ON public.clusters_kubernetes_namespaces USING btree (cluster_id, project_id, environment_id);
 
-CREATE INDEX index_label_links_on_label_id ON public.label_links USING btree (label_id);
+CREATE INDEX index_label_links_on_label_id_and_target_type ON public.label_links USING btree (label_id, target_type);
 
 CREATE INDEX index_label_links_on_target_id_and_target_type ON public.label_links USING btree (target_id, target_type);
 
@@ -20409,6 +20409,8 @@ CREATE INDEX index_on_identities_lower_extern_uid_and_provider ON public.identit
 
 CREATE UNIQUE INDEX index_on_instance_statistics_recorded_at_and_identifier ON public.analytics_instance_statistics_measurements USING btree (identifier, recorded_at);
 
+CREATE INDEX index_on_label_links_all_columns ON public.label_links USING btree (target_id, label_id, target_type);
+
 CREATE INDEX index_on_users_name_lower ON public.users USING btree (lower((name)::text));
 
 CREATE INDEX index_open_project_tracker_data_on_service_id ON public.open_project_tracker_data USING btree (service_id);

ee/spec/controllers/groups/epics_controller_spec.rb

@@ -192,7 +192,7 @@ RSpec.describe Groups::EpicsController do
         end
 
         context 'using label_name filter' do
-          let(:label) { create(:label) }
+          let(:label) { create(:group_label, group: group) }
           let!(:labeled_epic) { create(:labeled_epic, group: group, labels: [label]) }
 
           it 'returns all epics with given label' do

ee/spec/finders/epics_finder_spec.rb

@@ -107,7 +107,7 @@ RSpec.describe EpicsFinder do
         end
 
         context 'by label' do
-          let_it_be(:label) { create(:label) }
+          let_it_be(:label) { create(:group_label, group: group) }
           let_it_be(:labeled_epic) { create(:labeled_epic, group: group, labels: [label]) }
 
           it 'returns all epics with given label' do
@@ -461,8 +461,8 @@ RSpec.describe EpicsFinder do
 
         context 'when using group cte for search' do
           context 'and two labels more search string are present' do
-            let_it_be(:label1) { create(:label) }
-            let_it_be(:label2) { create(:label) }
+            let_it_be(:label1) { create(:group_label, group: group) }
+            let_it_be(:label2) { create(:group_label, group: group) }
             let_it_be(:labeled_epic) { create(:labeled_epic, group: group, title: 'filtered epic', labels: [label1, label2]) }
 
             it 'returns correct epics' do
@@ -559,8 +559,8 @@ RSpec.describe EpicsFinder do
         end
 
         context 'with negated labels' do
-          let_it_be(:label) { create(:label) }
-          let_it_be(:label2) { create(:label) }
+          let_it_be(:label) { create(:group_label, group: group) }
+          let_it_be(:label2) { create(:group_label, group: group) }
           let_it_be(:negated_epic) { create(:labeled_epic, group: group, labels: [label]) }
           let_it_be(:negated_epic2) { create(:labeled_epic, group: group, labels: [label2]) }
           let_it_be(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
@@ -630,8 +630,8 @@ RSpec.describe EpicsFinder do
   end
 
   describe '#row_count' do
-    let_it_be(:label) { create(:label) }
-    let_it_be(:label2) { create(:label) }
+    let_it_be(:label) { create(:group_label, group: group) }
+    let_it_be(:label2) { create(:group_label, group: group) }
     let_it_be(:labeled_epic) { create(:labeled_epic, group: group, labels: [label]) }
     let_it_be(:labeled_epic2) { create(:labeled_epic, group: group, labels: [label, label2]) }
 

ee/spec/requests/api/epics_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe API::Epics do
   let_it_be(:user) { create(:user) }
   let(:group) { create(:group) }
   let(:project) { create(:project, :public, group: group) }
-  let_it_be(:label) { create(:label) }
+  let(:label) { create(:group_label, group: group) }
   let!(:epic) { create(:labeled_epic, group: group, labels: [label]) }
   let(:params) { nil }
 

spec/finders/issues_finder_spec.rb

@@ -330,6 +330,7 @@ RSpec.describe IssuesFinder do
         end
       end
 
+      shared_examples ':label_name parameter' do
         context 'filtering by label' do
           let(:params) { { label_name: label.title } }
 
@@ -426,6 +427,44 @@ RSpec.describe IssuesFinder do
           end
         end
 
+        context 'when the same label exists on project and group levels' do
+          let(:issue1) { create(:issue, project: project1) }
+          let(:issue2) { create(:issue, project: project1) }
+
+          # Skipping validation to reproduce a "real-word" scenario.
+          # We still have legacy labels on PRD that have the same title on the group and project levels, example: `bug`
+          let(:project_label) { build(:label, title: 'somelabel', project: project1).tap { |r| r.save!(validate: false) } }
+          let(:group_label) { create(:group_label, title: 'somelabel', group: project1.group) }
+
+          let(:params) { { label_name: 'somelabel' } }
+
+          before do
+            create(:label_link, label: group_label, target: issue1)
+            create(:label_link, label: project_label, target: issue2)
+          end
+
+          it 'finds both issue records' do
+            expect(issues).to contain_exactly(issue1, issue2)
+          end
+        end
+      end
+
+      context 'when `optimized_issuable_label_filter` feature flag is off' do
+        before do
+          stub_feature_flags(optimized_issuable_label_filter: false)
+        end
+
+        it_behaves_like ':label_name parameter'
+      end
+
+      context 'when `optimized_issuable_label_filter` feature flag is on' do
+        before do
+          stub_feature_flags(optimized_issuable_label_filter: true)
+        end
+
+        it_behaves_like ':label_name parameter'
+      end
+
       context 'filtering by issue term' do
         let(:params) { { search: 'git' } }
 

spec/finders/merge_requests_finder_spec.rb

@@ -167,6 +167,7 @@ RSpec.describe MergeRequestsFinder do
         end
       end
 
+      shared_examples ':label_name parameter' do
         describe ':label_name parameter' do
           let(:common_labels) { create_list(:label, 3) }
           let(:distinct_labels) { create_list(:label, 3) }
@@ -200,6 +201,23 @@ RSpec.describe MergeRequestsFinder do
             expect(all_common).to match_array(merge_requests)
           end
         end
+      end
+
+      context 'when `optimized_issuable_label_filter` feature flag is off' do
+        before do
+          stub_feature_flags(optimized_issuable_label_filter: false)
+        end
+
+        it_behaves_like ':label_name parameter'
+      end
+
+      context 'when `optimized_issuable_label_filter` feature flag is on' do
+        before do
+          stub_feature_flags(optimized_issuable_label_filter: true)
+        end
+
+        it_behaves_like ':label_name parameter'
+      end
 
       it 'filters by source project id' do
         params = { source_project_id: merge_request2.source_project_id }

spec/graphql/resolvers/merge_requests_resolver_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe Resolvers::MergeRequestsResolver do
   let_it_be(:merge_request_3) { create(:merge_request, :unique_branches, **common_attrs) }
   let_it_be(:merge_request_4) { create(:merge_request, :unique_branches, :locked, **common_attrs) }
   let_it_be(:merge_request_5) { create(:merge_request, :simple, :locked, **common_attrs) }
-  let_it_be(:merge_request_6) { create(:labeled_merge_request, :unique_branches, labels: create_list(:label, 2), **common_attrs) }
+  let_it_be(:merge_request_6) { create(:labeled_merge_request, :unique_branches, labels: create_list(:label, 2, project: project), **common_attrs) }
   let_it_be(:merge_request_with_milestone) { create(:merge_request, :unique_branches, **common_attrs, milestone: milestone) }
   let_it_be(:other_project) { create(:project, :repository) }
   let_it_be(:other_merge_request) { create(:merge_request, source_project: other_project, target_project: other_project) }

spec/requests/api/graphql/project/merge_requests_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe 'getting merge request listings nested in a project' do
   let_it_be(:project) { create(:project, :repository, :public) }
   let_it_be(:current_user) { create(:user) }
 
-  let_it_be(:label) { create(:label) }
+  let_it_be(:label) { create(:label, project: project) }
   let_it_be(:merge_request_a) { create(:labeled_merge_request, :unique_branches, source_project: project, labels: [label]) }
   let_it_be(:merge_request_b) { create(:merge_request, :closed, :unique_branches, source_project: project) }
   let_it_be(:merge_request_c) { create(:labeled_merge_request, :closed, :unique_branches, source_project: project, labels: [label]) }

spec/requests/api/issues/get_group_issues_spec.rb

@@ -402,6 +402,14 @@ RSpec.describe API::Issues do
         expect_paginated_array_response([group_closed_issue.id, group_issue.id])
       end
 
+      shared_examples 'labels parameter' do
+        it 'returns an array of labeled group issues' do
+          get api(base_url, user), params: { labels: group_label.title }
+
+          expect_paginated_array_response(group_issue.id)
+          expect(json_response.first['labels']).to eq([group_label.title])
+        end
+
         it 'returns an array of labeled group issues' do
           get api(base_url, user), params: { labels: group_label.title }
 
@@ -428,18 +436,6 @@ RSpec.describe API::Issues do
           expect_paginated_array_response([])
         end
 
-      it 'returns issues matching given search string for title' do
-        get api(base_url, user), params: { search: group_issue.title }
-
-        expect_paginated_array_response(group_issue.id)
-      end
-
-      it 'returns issues matching given search string for description' do
-        get api(base_url, user), params: { search: group_issue.description }
-
-        expect_paginated_array_response(group_issue.id)
-      end
-
         context 'with labeled issues' do
           let(:group_issue2) { create :issue, project: group_project }
           let(:label_b) { create(:label, title: 'foo', project: group_project) }
@@ -460,6 +456,35 @@ RSpec.describe API::Issues do
 
           it_behaves_like 'labeled issues with labels and label_name params'
         end
+      end
+
+      context 'when `optimized_issuable_label_filter` feature flag is off' do
+        before do
+          stub_feature_flags(optimized_issuable_label_filter: false)
+        end
+
+        it_behaves_like 'labels parameter'
+      end
+
+      context 'when `optimized_issuable_label_filter` feature flag is on' do
+        before do
+          stub_feature_flags(optimized_issuable_label_filter: true)
+        end
+
+        it_behaves_like 'labels parameter'
+      end
+
+      it 'returns issues matching given search string for title' do
+        get api(base_url, user), params: { search: group_issue.title }
+
+        expect_paginated_array_response(group_issue.id)
+      end
+
+      it 'returns issues matching given search string for description' do
+        get api(base_url, user), params: { search: group_issue.description }
+
+        expect_paginated_array_response(group_issue.id)
+      end
 
       context 'with archived projects' do
         let_it_be(:archived_issue) do