Merge branch 'cherry-pick-add0da31' into '17-4-stable-ee'

Backport 17.4 Fix label filter by name for search

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/168099



Merged-by: Siddharth Dungarwal <sdungarwal@gitlab.com>
Approved-by: Siddharth Dungarwal <sdungarwal@gitlab.com>
Co-authored-by: Terri Chu <tchu@gitlab.com>

ee/lib/search/elastic/filters.rb

@@ -775,28 +775,53 @@ def ability_to_access_feature(feature)
         def find_labels_by_name(names, options)
           raise ArgumentError, 'search_level is a required option' unless options.key?(:search_level)
-          return [] unless names.any?
+          return [] if names.empty?
           search_level = options[:search_level].to_sym
-          project_ids = options[:project_ids]
-          group_ids = options[:group_ids]
-
           finder_params = { name: names }
-          case search_level
-          when :global
-          # no-op
-          # no group or project filtering applied
-          when :group
-            finder_params[:group_id] = group_ids.first if group_ids&.any?
-          when :project
-            finder_params[:group_id] = group_ids.first if group_ids&.any?
-            finder_params[:project_ids] = project_ids if project_ids != :any && project_ids&.any?
-          else
-            raise ArgumentError, 'Invalid search_level option provided'
+          labels = case search_level
+                   when :global
+                     find_global_labels(finder_params)
+                   when :group
+                     find_group_labels(finder_params, options[:group_ids])
+                   when :project
+                     find_project_labels(finder_params, options[:group_ids], options[:project_ids])
+                   else
+                     raise ArgumentError, 'Invalid search_level option provided'
+                   end
+
+          group_labels_by_name(labels)
+        end
+
+        def find_global_labels(finder_params)
+          LabelsFinder.new(nil, finder_params).execute(skip_authorization: true)
+        end
+
+        def find_group_labels(finder_params, group_ids)
+          return find_global_labels(finder_params) if group_ids.blank?
+
+          finder_params[:include_descendant_groups] = true
+          finder_params[:include_ancestor_groups] = true
+
+          group_ids.flat_map do |group_id|
+            LabelsFinder.new(nil, finder_params.merge(group_id: group_id)).execute(skip_authorization: true)
           end
-          labels = LabelsFinder.new(nil, finder_params).execute(skip_authorization: true)
+        end
+
+        def find_project_labels(finder_params, group_ids, project_ids)
+          # try group labels if no project_ids are provided or set to :any which means user has admin access
+          return find_group_labels(finder_params, group_ids) if project_ids.blank? || project_ids == :any
+
+          finder_params[:include_descendant_groups] = false
+          finder_params[:include_ancestor_groups] = true
+
+          project_ids.flat_map do |project_id|
+            LabelsFinder.new(nil, finder_params.merge(project_id: project_id)).execute(skip_authorization: true)
+          end
+        end
+        def group_labels_by_name(labels)
           labels.each_with_object(Hash.new { |h, k| h[k] = [] }) do |label, hash|
             hash[label.name] << label.id
           end

ee/spec/lib/elastic/latest/issue_class_proxy_spec.rb

@@ -2,18 +2,14 @@
 require 'spec_helper'
-RSpec.describe Elastic::Latest::IssueClassProxy, :elastic, :sidekiq_inline, feature_category: :global_search do
-  before do
-    stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
-  end
-
+RSpec.describe Elastic::Latest::IssueClassProxy, :elastic, feature_category: :global_search do
   subject(:proxy) { described_class.new(Issue, use_separate_indices: true) }
   let_it_be(:group) { create(:group) }
   let_it_be_with_reload(:project) { create(:project, :public, group: group) }
   let_it_be(:user) { create(:user, developer_of: project) }
-  let!(:label) { create(:label, project: project) }
-  let!(:issue) { create(:labeled_issue, title: 'test', project: project, labels: [label]) }
+  let_it_be(:label) { create(:label, project: project) }
+  let_it_be_with_reload(:issue) { create(:labeled_issue, title: 'test', project: project, labels: [label]) }
   let(:query) { 'test' }
   let(:options) do
@@ -27,35 +23,34 @@
     }
   end
-  describe '#issue_aggregations' do
-    before do
-      ensure_elasticsearch_index!
-    end
-    shared_examples 'returns aggregations' do
-      it 'filters by labels' do
-        result = proxy.issue_aggregations('test', options)
-
-        expect(result.first.name).to eq('labels')
-        expect(result.first.buckets.first.symbolize_keys).to match(
-          key: label.id.to_s,
-          count: 1,
-          title: label.title,
-          type: label.type,
-          color: label.color.to_s,
-          parent_full_name: label.project.full_name
-        )
-      end
-    end
-    it_behaves_like 'returns aggregations'
+  before do
+    stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
+    Elastic::ProcessInitialBookkeepingService.backfill_projects!(project)
+    ensure_elasticsearch_index!
+  end
+  describe '#issue_aggregations' do
+    it 'filters by labels' do
+      result = proxy.issue_aggregations('test', options)
+
+      expect(result.first.name).to eq('labels')
+      expect(result.first.buckets.first.symbolize_keys).to match(
+        key: label.id.to_s,
+        count: 1,
+        title: label.title,
+        type: label.type,
+        color: label.color.to_s,
+        parent_full_name: label.project.full_name
+      )
+    end
   end
   describe '#elastic_search' do
     let(:result) { proxy.elastic_search(query, options: options) }
     describe 'search on basis of hidden attribute' do
-      context 'when author of the issue is banned' do
+      context 'when author of the issue is banned', :sidekiq_inline do
         before do
           issue.author.ban
           ensure_elasticsearch_index!
@@ -78,10 +73,6 @@
       end
       context 'when author of the issue is active' do
-        before do
-          ensure_elasticsearch_index!
-        end
-
         it 'current_user is an admin user then user can see the issue' do
           allow(user).to receive(:can_admin_all_resources?).and_return(true)
           expect(elasticsearch_hit_ids(result)).to include issue.id

ee/spec/lib/search/elastic/filters_spec.rb

@@ -355,7 +355,8 @@
   describe '.by_label_ids' do
     let_it_be(:label_title) { 'My label' }
     let_it_be(:group) { create(:group) }
-    let_it_be(:project) { create(:project, group: group) }
+    let_it_be(:sub_group) { create(:group, parent: group) }
+    let_it_be(:project) { create(:project, group: sub_group) }
     # project label must be defined first or the title cannot match
     let_it_be(:project_label) { create(:label, project: project, title: label_title) }
     let_it_be(:project_2) { create(:project, group: group) }
@@ -577,7 +578,7 @@
       context 'for group search' do
         let(:search_level) { :group }
-        let(:group_ids) { [group.id] }
+        let(:group_ids) { [sub_group.id] }
         let(:project_ids) { nil }
         context 'when multiple label names are provided' do
@@ -591,7 +592,7 @@
                   {
                     terms: {
                       _name: 'filters:label_ids',
-                      label_ids: contain_exactly(group_label.id, project_label.id, project_label_2.id)
+                      label_ids: contain_exactly(group_label.id, project_label.id)
                     }
                   },
                   {
@@ -623,22 +624,46 @@
           it_behaves_like 'does not modify the query_hash'
         end
-        it 'adds the label_ids filter to query_hash' do
-          expected_filter = [
-            bool: {
-              must: [{
-                terms: {
-                  _name: 'filters:label_ids',
-                  label_ids: contain_exactly(group_label.id, project_label.id, project_label_2.id)
-                }
-              }]
-            }
-          ]
-          expect(by_label_ids.dig(:query, :bool, :filter)).to match(expected_filter)
-          expect(by_label_ids.dig(:query, :bool, :must)).to be_empty
-          expect(by_label_ids.dig(:query, :bool, :must_not)).to be_empty
-          expect(by_label_ids.dig(:query, :bool, :should)).to be_empty
+        context 'when top level group is searched' do
+          let(:group_ids) { [group.id] }
+          it 'adds the label_ids filter to query_hash' do
+            expected_filter = [
+              bool: {
+                must: [{
+                  terms: {
+                    _name: 'filters:label_ids',
+                    label_ids: contain_exactly(group_label.id, project_label.id, project_label_2.id)
+                  }
+                }]
+              }
+            ]
+
+            expect(by_label_ids.dig(:query, :bool, :filter)).to match(expected_filter)
+            expect(by_label_ids.dig(:query, :bool, :must)).to be_empty
+            expect(by_label_ids.dig(:query, :bool, :must_not)).to be_empty
+            expect(by_label_ids.dig(:query, :bool, :should)).to be_empty
+          end
+        end
+
+        context 'when subgroup is searched' do
+          it 'adds the label_ids filter to query_hash' do
+            expected_filter = [
+              bool: {
+                must: [{
+                  terms: {
+                    _name: 'filters:label_ids',
+                    label_ids: contain_exactly(group_label.id, project_label.id)
+                  }
+                }]
+              }
+            ]
+
+            expect(by_label_ids.dig(:query, :bool, :filter)).to match(expected_filter)
+            expect(by_label_ids.dig(:query, :bool, :must)).to be_empty
+            expect(by_label_ids.dig(:query, :bool, :must_not)).to be_empty
+            expect(by_label_ids.dig(:query, :bool, :should)).to be_empty
+          end
         end
       end