Merge branch 'tchu-fix-label_name-global-level' into 'master'

Fix bug with label_name search filter

See merge request !163353



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

ee/lib/search/elastic/filters.rb

@@ -602,13 +602,28 @@ def filter_ids_by_feature(project_ids, user, feature)
         end
 
         def find_labels_by_name(names, options)
+          raise ArgumentError, 'search_scope is a required option' unless options.key?(:search_scope)
+
           return [] unless names.any?
 
+          search_level = options[:search_scope].to_sym
           project_ids = options[:project_ids]
           group_ids = options[:group_ids]
+
           finder_params = { name: names }
-          finder_params[:project_ids] = project_ids if project_ids&.any? && project_ids != :any
-          finder_params[:group_id] = group_ids.first if group_ids&.any?
+
+          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_scope option provided'
+          end
 
           labels = LabelsFinder.new(nil, finder_params).execute(skip_authorization: true)
           labels.each_with_object(Hash.new { |h, k| h[k] = [] }) do |label, hash|

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

@@ -101,11 +101,11 @@
     describe 'named queries' do
       using RSpec::Parameterized::TableSyntax
 
-      where(:projects, :groups) do
-        [] | []
-        [ref(:project)] | []
-        [] | [ref(:group)]
-        [ref(:project)] | [ref(:group)]
+      where(:search_level, :projects, :groups) do
+        :global   | []              | []
+        :project  | [ref(:project)] | []
+        :group    | []              | [ref(:group)]
+        :project  | [ref(:project)] | [ref(:group)]
       end
 
       with_them do
@@ -116,6 +116,7 @@
         let(:base_options) do
           {
             current_user: user,
+            search_scope: search_level,
             project_ids: project_ids,
             group_ids: group_ids,
             public_and_internal_projects: false,

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

@@ -362,6 +362,8 @@
     let_it_be(:project) { create(:project, group: 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) }
+    let_it_be(:project_label_2) { create(:label, project: project_2, title: label_title) }
     let_it_be(:group_label) { create(:group_label, group: group, title: label_title) }
 
     subject(:by_label_ids) { described_class.by_label_ids(query_hash: query_hash, options: options) }
@@ -397,7 +399,7 @@
       end
 
       context 'when options[:aggregation] is true' do
-        let(:options) { { labels: [project_label.id], aggregation: true } }
+        let(:options) { { labels: [project.id], aggregation: true } }
 
         it_behaves_like 'does not modify the query_hash'
       end
@@ -410,47 +412,163 @@
     end
 
     context 'when options[:label_name] is provided' do
-      let(:options) { { label_name: [label_title] } }
+      let(:label_name) { [label_title] }
+      let(:aggregations) { false }
+      let(:count_only) { false }
+      let(:group_ids) { nil }
+      let(:project_ids) { nil }
+      let(:options) do
+        {
+          label_name: label_name, search_scope: search_level, count_only: count_only, aggregations: aggregations,
+          group_ids: group_ids, project_ids: project_ids
+        }
+      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)
-              }
-            }]
-          }
-        ]
+      context 'when search_level invalid' do
+        let(:search_level) { :not_supported }
 
-        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
+        it 'raises an exception' do
+          expect { by_label_ids }.to raise_exception(ArgumentError)
+        end
       end
 
-      context 'when multiple label names are provided' do
-        let_it_be(:another_label) { create(:label, project: project, title: 'Another label') }
-        let(:options) { { label_name: [label_title, another_label.title] } }
+      context 'when search_level is not provided' do
+        let(:options) { { label_name: label_name } }
 
-        it 'adds the label_ids filter to query_hash' do
-          expected_filter = [
-            bool: {
-              must: contain_exactly(
-                {
-                  terms: {
-                    _name: 'filters:label_ids',
-                    label_ids: contain_exactly(group_label.id, project_label.id)
+        it 'raises an exception' do
+          expect { by_label_ids }.to raise_exception(ArgumentError)
+        end
+      end
+
+      context 'for global search' do
+        let(:search_level) { :global }
+
+        context 'when multiple label names are provided' do
+          let_it_be(:another_label) { create(:label, project: project, title: 'Another label') }
+          let(:label_name) { [label_title, another_label.title] }
+
+          it 'adds the label_ids filter to query_hash' do
+            expected_filter = [
+              bool: {
+                must: contain_exactly(
+                  {
+                    terms: {
+                      _name: 'filters:label_ids',
+                      label_ids: contain_exactly(group_label.id, project_label.id, project_label_2.id)
+                    }
+                  },
+                  {
+                    terms: {
+                      _name: 'filters:label_ids',
+                      label_ids: contain_exactly(another_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
+
+        context 'when options[:group_ids] is provided' do
+          let(:group_ids) { [group.id] }
+
+          it 'adds the label_ids filter to query_hash with no group filtering' do
+            expected_filter = [
+              bool: {
+                must: [{
                   terms: {
                     _name: 'filters:label_ids',
-                    label_ids: contain_exactly(another_label.id)
+                    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
+
+          context 'when options[:project_ids] is provided' do
+            let(:project_ids) { [project.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
+        end
+
+        context 'when options[:project_ids] is provided' do
+          using RSpec::Parameterized::TableSyntax
+
+          let(:project_ids) { projects == :any ? projects : [projects.id] }
+
+          where(:projects) do
+            [:any, ref(:project), ref(:project_2)]
+          end
+
+          with_them do
+            it 'adds the label_ids filter to query_hash with no project filtering' 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
+        end
+
+        context 'when options[:count_only] is true' do
+          let(:count_only) { true }
+
+          it_behaves_like 'does not modify the query_hash'
+        end
+
+        context 'when options[:aggregation] is true' do
+          let(:aggregation) { true }
+
+          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)
+                }
+              }]
             }
           ]
 
@@ -461,8 +579,53 @@
         end
       end
 
-      context 'when options[:group_ids] is provided' do
-        let(:options) { { label_name: [label_title], group_ids: [group.id] } }
+      context 'for group search' do
+        let(:search_level) { :group }
+        let(:group_ids) { [group.id] }
+        let(:project_ids) { nil }
+
+        context 'when multiple label names are provided' do
+          let_it_be(:another_label) { create(:label, project: project, title: 'Another label') }
+          let(:label_name) { [label_title, another_label.title] }
+
+          it 'adds the label_ids filter to query_hash' do
+            expected_filter = [
+              bool: {
+                must: contain_exactly(
+                  {
+                    terms: {
+                      _name: 'filters:label_ids',
+                      label_ids: contain_exactly(group_label.id, project_label.id, project_label_2.id)
+                    }
+                  },
+                  {
+                    terms: {
+                      _name: 'filters:label_ids',
+                      label_ids: contain_exactly(another_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
+
+        context 'when options[:count_only] is true' do
+          let(:count_only) { true }
+
+          it_behaves_like 'does not modify the query_hash'
+        end
+
+        context 'when options[:aggregation] is true' do
+          let(:aggregation) { true }
+
+          it_behaves_like 'does not modify the query_hash'
+        end
 
         it 'adds the label_ids filter to query_hash' do
           expected_filter = [
@@ -470,7 +633,7 @@
               must: [{
                 terms: {
                   _name: 'filters:label_ids',
-                  label_ids: contain_exactly(group_label.id, project_label.id)
+                  label_ids: contain_exactly(group_label.id, project_label.id, project_label_2.id)
                 }
               }]
             }
@@ -481,13 +644,48 @@
           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 options[:project_ids] is provided' do
-          let(:options) do
-            { label_name: [label_title], group_id: [group.id], project_ids: [project.id] }
-          end
+      context 'for project search' do
+        let(:search_level) { :project }
+        let(:group_ids) { nil }
+        let(:project_ids) { [project.id] }
+
+        context 'when multiple label names are provided' do
+          let_it_be(:another_label) { create(:label, project: project, title: 'Another label') }
+          let(:label_name) { [label_title, another_label.title] }
 
           it 'adds the label_ids filter to query_hash' do
+            expected_filter = [
+              bool: {
+                must: contain_exactly(
+                  {
+                    terms: {
+                      _name: 'filters:label_ids',
+                      label_ids: contain_exactly(group_label.id, project_label.id)
+                    }
+                  },
+                  {
+                    terms: {
+                      _name: 'filters:label_ids',
+                      label_ids: contain_exactly(another_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
+
+        context 'when options[:group_ids] is provided' do
+          let(:group_ids) { [group.id] }
+
+          it 'adds the label_ids filter to query_hash with no group filtering' do
             expected_filter = [
               bool: {
                 must: [{
@@ -505,10 +703,18 @@
             expect(by_label_ids.dig(:query, :bool, :should)).to be_empty
           end
         end
-      end
 
-      context 'when options[:project_ids] is provided' do
-        let(:options) { { label_name: [label_title], project_ids: [project.id] } }
+        context 'when options[:count_only] is true' do
+          let(:count_only) { true }
+
+          it_behaves_like 'does not modify the query_hash'
+        end
+
+        context 'when options[:aggregation] is true' do
+          let(:aggregation) { true }
+
+          it_behaves_like 'does not modify the query_hash'
+        end
 
         it 'adds the label_ids filter to query_hash' do
           expected_filter = [
@@ -528,22 +734,10 @@
           expect(by_label_ids.dig(:query, :bool, :should)).to be_empty
         end
       end
-
-      context 'when options[:count_only] is true' do
-        let(:options) { { label_name: [label_title], count_only: true } }
-
-        it_behaves_like 'does not modify the query_hash'
-      end
-
-      context 'when options[:aggregation] is true' do
-        let(:options) { { label_name: [label_title], aggregation: true } }
-
-        it_behaves_like 'does not modify the query_hash'
-      end
     end
 
     context 'when options[:labels] and options[:label_name] are provided' do
-      let(:options) { { labels: [project_label.id], label_name: [label_title] } }
+      let(:options) { { labels: [project_label.id], label_name: [label_title], search_scope: :global } }
 
       it 'uses label_name option and adds the label_ids filter to query_hash' do
         expected_filter = [
@@ -551,7 +745,7 @@
             must: [{
               terms: {
                 _name: 'filters:label_ids',
-                label_ids: contain_exactly(group_label.id, project_label.id)
+                label_ids: contain_exactly(group_label.id, project_label.id, project_label_2.id)
               }
             }]
           }

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

@@ -7,6 +7,7 @@
   let(:base_options) do
     {
       current_user: user,
+      search_scope: 'global',
       project_ids: project_ids,
       group_ids: [],
       public_and_internal_projects: false

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

@@ -7,6 +7,7 @@
   let(:base_options) do
     {
       current_user: user,
+      search_scope: 'global',
       project_ids: project_ids,
       group_ids: [],
       public_and_internal_projects: true