From 3f7555bec895f46e787c4942ad4f6feb2aa40cd7 Mon Sep 17 00:00:00 2001 From: Alexey Dudarev <skel@nebius.com> Date: Wed, 11 Sep 2024 17:07:19 +0200 Subject: [PATCH] Fix Zoekt global code search There is no Code tab in search navigation now. There are no results with manual scope=blobs now. This fix: - uses :zoekt_cross_namespace_search for navigation, - correctly overrides allowed scopes, - adds and updates tests. Changelog: fixed EE: true --- .../concerns/search/zoekt_searchable.rb | 2 +- ee/app/services/ee/search/global_service.rb | 11 +++-- ee/app/services/ee/search/group_service.rb | 3 +- ee/lib/ee/search/navigation.rb | 14 ++++-- ee/spec/lib/search/navigation_spec.rb | 48 ++++++++++++++----- .../services/search/global_service_spec.rb | 35 ++++++++++++++ ee/spec/services/search/group_service_spec.rb | 14 +++++- .../services/search/project_service_spec.rb | 2 +- 8 files changed, 105 insertions(+), 24 deletions(-) diff --git a/ee/app/services/concerns/search/zoekt_searchable.rb b/ee/app/services/concerns/search/zoekt_searchable.rb index 3acb1becc86a48d2..15fb1b5ba2fb3ba8 100644 --- a/ee/app/services/concerns/search/zoekt_searchable.rb +++ b/ee/app/services/concerns/search/zoekt_searchable.rb @@ -17,7 +17,7 @@ def zoekt_searchable_scope end def zoekt_searchable_scope? - scope == 'blobs' && zoekt_searchable_scope.try(:search_code_with_zoekt?) + zoekt_searchable_scope.try(:search_code_with_zoekt?) end def root_ancestor diff --git a/ee/app/services/ee/search/global_service.rb b/ee/app/services/ee/search/global_service.rb index acf3df4734d8ec90..213973acdefac0b3 100644 --- a/ee/app/services/ee/search/global_service.rb +++ b/ee/app/services/ee/search/global_service.rb @@ -21,7 +21,7 @@ def elasticsearch_results override :zoekt_searchable_scope? def zoekt_searchable_scope? - scope == 'blobs' && ::Feature.enabled?(:zoekt_cross_namespace_search, current_user) + ::Feature.enabled?(:zoekt_cross_namespace_search, current_user) end override :root_ancestor @@ -70,10 +70,13 @@ def elastic_projects override :allowed_scopes def allowed_scopes - return super unless use_elasticsearch? - strong_memoize(:ee_allowed_scopes) do - super + %w[blobs commits epics notes wiki_blobs] + scopes = super + + scopes += %w[blobs commits epics notes wiki_blobs] if use_elasticsearch? + scopes += %w[blobs] if use_zoekt? + + scopes.uniq end end end diff --git a/ee/app/services/ee/search/group_service.rb b/ee/app/services/ee/search/group_service.rb index ade7615576145c4b..93f74531a837c23d 100644 --- a/ee/app/services/ee/search/group_service.rb +++ b/ee/app/services/ee/search/group_service.rb @@ -58,10 +58,9 @@ def allowed_scopes scopes -= %w[epics] unless group.licensed_feature_available?(:epics) else scopes += %w[epics] # In basic search we have epics enabled for groups - scopes += %w[blobs] if ::Search::Zoekt.search?(group) && ::Search::Zoekt.enabled_for_user?(current_user) end - scopes + scopes.uniq end end end diff --git a/ee/lib/ee/search/navigation.rb b/ee/lib/ee/search/navigation.rb index c1debe47348e38fe..deff54e64cc4de17 100644 --- a/ee/lib/ee/search/navigation.rb +++ b/ee/lib/ee/search/navigation.rb @@ -21,14 +21,22 @@ def show_code_search_tab? return true if super return false unless project.nil? + global_search_code_tab_enabled = ::Feature.enabled?(:global_search_code_tab, user, type: :ops) + global_search_with_zoekt_enabled = ::Feature.enabled?(:zoekt_cross_namespace_search, user, type: :ops) + + zoekt_enabled_for_user = zoekt_enabled? && ::Search::Zoekt.enabled_for_user?(user) + if show_elasticsearch_tabs? return true if group.present? - return ::Feature.enabled?(:global_search_code_tab, user, type: :ops) + return global_search_code_tab_enabled + elsif zoekt_enabled_for_user + return ::Search::Zoekt.search?(group) if group.present? + + return global_search_code_tab_enabled && global_search_with_zoekt_enabled end - group.present? && zoekt_enabled? && - ::Search::Zoekt.search?(group) && ::Search::Zoekt.enabled_for_user?(user) + false end override :show_wiki_search_tab? diff --git a/ee/spec/lib/search/navigation_spec.rb b/ee/spec/lib/search/navigation_spec.rb index 8fec789f88a6e159..b2ac27ed9b5bbcb8 100644 --- a/ee/spec/lib/search/navigation_spec.rb +++ b/ee/spec/lib/search/navigation_spec.rb @@ -219,23 +219,49 @@ let(:project) { nil } let(:group) { nil } - where(:feature_flag, :show_elasticsearch_tabs, :zoekt_enabled, :condition) do - false | false | false | false - false | false | true | false - true | true | false | true - true | true | true | true - true | false | false | false - true | false | true | false - true | false | true | false - false | true | false | false - false | true | true | false + where(:global_search_code_tab_enabled, :global_search_with_zoekt_enabled, :show_elasticsearch_tabs, + :zoekt_enabled, :zoekt_enabled_for_user, :condition) do + false | false | false | false | false | false + false | false | false | false | true | false + false | false | false | true | false | false + false | false | false | true | true | false + false | false | true | false | false | false + false | false | true | false | true | false + false | false | true | true | false | false + false | false | true | true | true | false + false | true | false | false | false | false + false | true | false | false | true | false + false | true | false | true | false | false + false | true | false | true | true | false + false | true | true | false | false | false + false | true | true | false | true | false + false | true | true | true | false | false + false | true | true | true | true | false + true | false | false | false | false | false + true | false | false | false | true | false + true | false | false | true | false | false + true | false | false | true | true | false + true | false | true | false | false | true + true | false | true | false | true | true + true | false | true | true | false | true + true | false | true | true | true | true + true | true | false | false | false | false + true | true | false | false | true | false + true | true | false | true | false | false + true | true | false | true | true | true + true | true | true | false | false | true + true | true | true | false | true | true + true | true | true | true | false | true + true | true | true | true | true | true end with_them do let(:options) { { show_elasticsearch_tabs: show_elasticsearch_tabs, zoekt_enabled: zoekt_enabled } } before do - stub_feature_flags(global_search_code_tab: feature_flag) + stub_feature_flags(global_search_code_tab: global_search_code_tab_enabled) + stub_feature_flags(zoekt_cross_namespace_search: global_search_with_zoekt_enabled) + allow(::Search::Zoekt).to receive(:enabled_for_user?).and_return(zoekt_enabled_for_user) end it 'data item condition is set correctly' do diff --git a/ee/spec/services/search/global_service_spec.rb b/ee/spec/services/search/global_service_spec.rb index 9bbdb8b194675905..53d81846b82323db 100644 --- a/ee/spec/services/search/global_service_spec.rb +++ b/ee/spec/services/search/global_service_spec.rb @@ -422,6 +422,41 @@ end end end + + context 'for blobs scope' do + context 'when elasticearch_search is disabled and zoekt is disabled' do + before do + stub_ee_application_setting(elasticsearch_search: false) + allow(::Search::Zoekt).to receive(:enabled_for_user?).and_return(false) + end + + it 'does not include blobs scope' do + expect(described_class.new(user, {}).allowed_scopes).not_to include('blobs') + end + end + + context 'when elasticsearch_search is enabled and zoekt is disabled' do + before do + stub_ee_application_setting(elasticsearch_search: true) + allow(::Search::Zoekt).to receive(:enabled_for_user?).and_return(false) + end + + it 'includes blobs scope' do + expect(described_class.new(user, {}).allowed_scopes).to include('blobs') + end + end + + context 'when elasticsearch_search is disabled and zoekt is enabled' do + before do + stub_ee_application_setting(elasticsearch_search: false) + allow(::Search::Zoekt).to receive(:enabled_for_user?).and_return(true) + end + + it 'includes blobs scope' do + expect(described_class.new(user, {}).allowed_scopes).to include('blobs') + end + end + end end describe '#elastic_projects' do diff --git a/ee/spec/services/search/group_service_spec.rb b/ee/spec/services/search/group_service_spec.rb index 662294c6bfe128b1..e6fb7c0f77ab3889 100644 --- a/ee/spec/services/search/group_service_spec.rb +++ b/ee/spec/services/search/group_service_spec.rb @@ -213,7 +213,7 @@ let(:scope) { 'issues' } it 'does not search with Zoekt' do - expect(service.use_zoekt?).to eq(false) + expect(service.search_type).not_to eq('zoekt') expect(service.execute).not_to be_kind_of(::Search::Zoekt::SearchResults) end end @@ -653,8 +653,9 @@ describe '#allowed_scopes' do let_it_be(:group) { create(:group) } + let(:service) { described_class.new(user, group, {}) } - subject(:allowed_scopes) { described_class.new(user, group, {}).allowed_scopes } + subject(:allowed_scopes) { service.allowed_scopes } context 'for blobs scope' do context 'when elasticearch_search is disabled and zoekt is disabled' do @@ -680,10 +681,19 @@ stub_ee_application_setting(elasticsearch_search: false) allow(::Search::Zoekt).to receive(:enabled_for_user?).and_return(true) allow(::Search::Zoekt).to receive(:search?).with(group).and_return(true) + allow(service).to receive(:zoekt_node_available_for_search?).and_return(true) end it { is_expected.to include('blobs') } + context 'and zoekt node is not available' do + before do + allow(service).to receive(:zoekt_node_available_for_search?).and_return(false) + end + + it { is_expected.not_to include('blobs') } + end + context 'but the group does is not enabled for zoekt' do before do allow(::Search::Zoekt).to receive(:search?).with(group).and_return(false) diff --git a/ee/spec/services/search/project_service_spec.rb b/ee/spec/services/search/project_service_spec.rb index c572ebc69d64ffd9..383b39531448e608 100644 --- a/ee/spec/services/search/project_service_spec.rb +++ b/ee/spec/services/search/project_service_spec.rb @@ -167,7 +167,7 @@ let(:scope) { 'issues' } it 'does not search with Zoekt' do - expect(service.use_zoekt?).to eq(false) + expect(service.search_type).not_to eq('zoekt') expect(service.execute).not_to be_kind_of(::Search::Zoekt::SearchResults) end end -- GitLab