From af75c443f0d670fc6cd6ebbdecd5c365bdab0311 Mon Sep 17 00:00:00 2001 From: Terri Chu Date: Wed, 17 Sep 2025 21:03:43 -0400 Subject: [PATCH 1/3] Remove highlight in query for notes Only when count or noteable_type are used in the search. Count does not require highlighting and noteable_type searches are used for searches notes during issues scope searches Changelog: changed EE: true --- ee/lib/elastic/latest/note_class_proxy.rb | 8 ---- ee/lib/gitlab/elastic/search_results.rb | 2 +- ee/lib/search/elastic/filters.rb | 8 +++- .../elastic/latest/note_class_proxy_spec.rb | 12 +++--- ee/spec/lib/search/elastic/filters_spec.rb | 39 ++++++++++++++++++- 5 files changed, 50 insertions(+), 19 deletions(-) diff --git a/ee/lib/elastic/latest/note_class_proxy.rb b/ee/lib/elastic/latest/note_class_proxy.rb index 70bf1fb44cc211..7326eb3d58a4d1 100644 --- a/ee/lib/elastic/latest/note_class_proxy.rb +++ b/ee/lib/elastic/latest/note_class_proxy.rb @@ -31,8 +31,6 @@ def elastic_search(query, options: {}) query_hash = ::Search::Elastic::Filters.by_noteable_type(query_hash:, options:) end - query_hash[:highlight] = highlight_options(options[:in]) if include_highlight?(options) - search(query_hash, options) end @@ -58,12 +56,6 @@ def preload_indexing_data(relation) attr_reader :noteable_type_to_feature - def include_highlight?(options) - return false if options[:count_only] || options[:noteable_type] - - true - end - def get_authorization_filter(query_hash, options) if use_new_auth?(options[:current_user]) options[:features] = noteable_type_to_feature.values diff --git a/ee/lib/gitlab/elastic/search_results.rb b/ee/lib/gitlab/elastic/search_results.rb index b9238fce5e8303..440e8d31e8f8e3 100644 --- a/ee/lib/gitlab/elastic/search_results.rb +++ b/ee/lib/gitlab/elastic/search_results.rb @@ -453,7 +453,7 @@ def notes(count_only: false) def related_ids_for_notes(noteable_type) strong_memoize_with(:related_ids_for_notes, noteable_type) do - options = scope_options(:notes).merge(count_only: false, noteable_type: noteable_type) + options = scope_options(:notes).merge(count_only: false, noteable_type: noteable_type, related_ids_only: true) notes_response = Note.elastic_search(query, options: options).response notes_response['hits']['hits'].filter_map { |hit| hit['_source']['noteable_id'] }.uniq diff --git a/ee/lib/search/elastic/filters.rb b/ee/lib/search/elastic/filters.rb index 6ea65edb51c3ea..144b0f8f47d43e 100644 --- a/ee/lib/search/elastic/filters.rb +++ b/ee/lib/search/elastic/filters.rb @@ -1031,8 +1031,12 @@ def by_noteable_type(query_hash:, options:) end context.name(:filters) do - query_hash[:_source] = ['noteable_id'] - query_hash[:size] = options.fetch(:related_size, DEFAULT_RELATED_SIZE) + if options[:related_ids_only] + query_hash[:_source] = ['noteable_id'] + # basic_query_hash automatically adds highlight + query_hash.delete(:highlight) + query_hash[:size] = options.fetch(:related_size, DEFAULT_RELATED_SIZE) + end add_filter(query_hash, :query, :bool, :filter) do { diff --git a/ee/spec/lib/elastic/latest/note_class_proxy_spec.rb b/ee/spec/lib/elastic/latest/note_class_proxy_spec.rb index 0740c40964f53f..4f57971f8d76ae 100644 --- a/ee/spec/lib/elastic/latest/note_class_proxy_spec.rb +++ b/ee/spec/lib/elastic/latest/note_class_proxy_spec.rb @@ -79,8 +79,8 @@ end end - context 'when options[:noteable_type] is set' do - let(:options) { base_options.merge(noteable_type: 'Issue') } + context 'when options[:noteable_type] and options[:related_ids_only] set' do + let(:options) { base_options.merge(noteable_type: 'Issue', related_ids_only: true) } it 'filters by noteable_type and returns noteable_id only in the _source array' do hits = result.response['hits']['hits'] @@ -120,8 +120,8 @@ end end - context 'when options[:noteable_type] is set' do - let(:options) { base_options.merge(noteable_type: 'Issue') } + context 'when options[:noteable_type] and options[:related_ids_only] are set' do + let(:options) { base_options.merge(noteable_type: 'Issue', related_ids_only: true) } it 'filters by noteable_type and returns noteable_id only in the _source array' do expect(result.response['hits']['hits'].first['_source'].keys).to contain_exactly('noteable_id') @@ -158,8 +158,8 @@ end end - context 'when options[:noteable_type] is set' do - let(:options) { base_options.merge(noteable_type: 'Issue') } + context 'when options[:noteable_type] and options[:related_ids_only] are set' do + let(:options) { base_options.merge(noteable_type: 'Issue', related_ids_only: true) } it 'filters by noteable_type and returns noteable_id only in the _source array' do expect(result.response['hits']['hits'].first['_source'].keys).to contain_exactly('noteable_id') diff --git a/ee/spec/lib/search/elastic/filters_spec.rb b/ee/spec/lib/search/elastic/filters_spec.rb index 428f2ea9ed0f03..887ffc20e0c429 100644 --- a/ee/spec/lib/search/elastic/filters_spec.rb +++ b/ee/spec/lib/search/elastic/filters_spec.rb @@ -3618,7 +3618,20 @@ def compare_query_bool_fields(actual, expected) end describe '.by_noteable_type' do - let(:query_hash) { { query: { bool: { filter: [] } } } } + # use a query (including highlight) like what NoteClassProxy uses + let(:query_hash) do + { query: + { bool: + { must: [], + should: [], + filter: [] } }, + highlight: { + fields: { note: {} }, + number_of_fragments: 0, + pre_tags: ["gitlabelasticsearch→"], + post_tags: ["←gitlabelasticsearch"] + } } + end context 'when search_level is global' do it 'returns the original query_hash without modifications' do @@ -3651,9 +3664,31 @@ def compare_query_bool_fields(actual, expected) end end - context 'when valid noteable_type is provided' do + context 'when only valid noteable_type is provided' do let(:options) { { search_level: 'project', noteable_type: 'Issue' } } + it 'keeps the highlight if set' do + described_class.by_noteable_type(query_hash: query_hash, options: options) + + expect(query_hash).to have_key(:highlight) + end + + it 'does not set _source to only include noteable_id' do + described_class.by_noteable_type(query_hash: query_hash, options: options) + + expect(query_hash).not_to have_key(:_source) + end + + it 'does not sets size' do + described_class.by_noteable_type(query_hash: query_hash, options: options) + + expect(query_hash).not_to have_key(:size) + end + end + + context 'when valid noteable_type and related_ids_only is provided' do + let(:options) { { search_level: 'project', noteable_type: 'Issue', related_ids_only: true } } + it 'does not provide highlight' do described_class.by_noteable_type(query_hash: query_hash, options: options) -- GitLab From 132c35c4d0e1c93b1a6cc3d9d1dd2d3b33cce69a Mon Sep 17 00:00:00 2001 From: Terri Chu Date: Fri, 19 Sep 2025 09:53:15 -0400 Subject: [PATCH 2/3] Apply suggestions from review --- .../elastic/latest/note_class_proxy_spec.rb | 2 +- ee/spec/lib/search/elastic/filters_spec.rb | 24 +++++++++++-------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/ee/spec/lib/elastic/latest/note_class_proxy_spec.rb b/ee/spec/lib/elastic/latest/note_class_proxy_spec.rb index 4f57971f8d76ae..d2a3cb7bbc11ab 100644 --- a/ee/spec/lib/elastic/latest/note_class_proxy_spec.rb +++ b/ee/spec/lib/elastic/latest/note_class_proxy_spec.rb @@ -79,7 +79,7 @@ end end - context 'when options[:noteable_type] and options[:related_ids_only] set' do + context 'when options[:noteable_type] and options[:related_ids_only] are set' do let(:options) { base_options.merge(noteable_type: 'Issue', related_ids_only: true) } it 'filters by noteable_type and returns noteable_id only in the _source array' do diff --git a/ee/spec/lib/search/elastic/filters_spec.rb b/ee/spec/lib/search/elastic/filters_spec.rb index 887ffc20e0c429..ec3cdd795646e3 100644 --- a/ee/spec/lib/search/elastic/filters_spec.rb +++ b/ee/spec/lib/search/elastic/filters_spec.rb @@ -3618,19 +3618,23 @@ def compare_query_bool_fields(actual, expected) end describe '.by_noteable_type' do - # use a query (including highlight) like what NoteClassProxy uses - let(:query_hash) do - { query: - { bool: - { must: [], + { + query: { + bool: { + must: [], should: [], - filter: [] } }, + filter: [] + } + }, highlight: { - fields: { note: {} }, + fields: { + note: {} + }, number_of_fragments: 0, - pre_tags: ["gitlabelasticsearch→"], - post_tags: ["←gitlabelasticsearch"] - } } + pre_tags: [Elastic::Latest::GitClassProxy::HIGHLIGHT_START_TAG], + post_tags: [Elastic::Latest::GitClassProxy::HIGHLIGHT_END_TAG] + } + } end context 'when search_level is global' do -- GitLab From 0521b886c583f617279c593fa22e17748ae367a1 Mon Sep 17 00:00:00 2001 From: Terri Chu Date: Fri, 19 Sep 2025 12:01:06 -0400 Subject: [PATCH 3/3] fixup! Apply suggestions from review --- ee/spec/lib/search/elastic/filters_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ee/spec/lib/search/elastic/filters_spec.rb b/ee/spec/lib/search/elastic/filters_spec.rb index ec3cdd795646e3..a61aa61313d1d2 100644 --- a/ee/spec/lib/search/elastic/filters_spec.rb +++ b/ee/spec/lib/search/elastic/filters_spec.rb @@ -3618,6 +3618,8 @@ def compare_query_bool_fields(actual, expected) end describe '.by_noteable_type' do + # use a query (including highlight) like what NoteClassProxy uses + let(:query_hash) do { query: { bool: { -- GitLab