Merge branch 'fj-26123-narrow-snippet-search-scope-in-com' into 'master'

Narrow snippet search scope in GitLab.com

Closes #26123

See merge request gitlab-org/gitlab!17625

app/finders/snippets_finder.rb

@@ -59,18 +59,20 @@ class SnippetsFinder < UnionFinder
   end
 
   def execute
-    base =
+    base = init_collection
+    base.with_optional_visibility(visibility_from_scope).fresh
+  end
+
+  private
+
+  def init_collection
     if project
       snippets_for_a_single_project
     else
       snippets_for_multiple_projects
     end
-
-    base.with_optional_visibility(visibility_from_scope).fresh
   end
 
-  private
-
   # Produces a query that retrieves snippets from multiple projects.
   #
   # The resulting query will, depending on the user's permissions, include the
@@ -115,7 +117,7 @@ class SnippetsFinder < UnionFinder
   # This method requires that `current_user` returns a `User` instead of `nil`,
   # and is optimised for this specific scenario.
   def snippets_of_authorized_projects
-    base = author ? snippets_for_author : Snippet.all
+    base = author ? author.snippets : Snippet.all
 
     base
       .only_include_projects_with_snippets_enabled(include_private: true)
@@ -157,3 +159,5 @@ class SnippetsFinder < UnionFinder
     end
   end
 end
+
+SnippetsFinder.prepend_if_ee('EE::SnippetsFinder')

changelogs/unreleased/fj-26123-narrow-snippet-search-scope-in-com.yml

+---
+title: Narrow snippet search scope in GitLab.com
+merge_request: 17625
+author:
+type: performance

ee/app/finders/ee/snippets_finder.rb

+# frozen_string_literal: true
+
+module EE
+  module SnippetsFinder
+    extend ::Gitlab::Utils::Override
+
+    attr_reader :authorized_and_user_personal
+
+    def initialize(current_user = nil, params = {})
+      super
+
+      @authorized_and_user_personal = params[:authorized_and_user_personal]
+    end
+
+    private
+
+    override :init_collection
+    def init_collection
+      return snippets_of_authorized_projects_or_personal if authorized_and_user_personal.present?
+
+      super
+    end
+
+    # This method returns snippets from a more restrictive scope.
+    # When current_user is not nil we return the personal snippets
+    # authored by the user and also snippets from the authorized projects.
+    #
+    # When current_user is nil it returns only public personal snippets
+    def snippets_of_authorized_projects_or_personal
+      queries = [restricted_global_snippets]
+
+      if current_user && Ability.allowed?(current_user, :read_cross_project)
+        queries << snippets_of_authorized_projects
+      end
+
+      find_union(queries, ::Snippet)
+    end
+
+    def restricted_global_snippets
+      if author
+        snippets_for_author
+      elsif current_user
+        current_user.snippets
+      else
+        ::Snippet.public_to_user
+      end.only_global_snippets
+    end
+  end
+end

ee/app/helpers/ee/search_helper.rb

@@ -3,6 +3,7 @@ module EE
   module SearchHelper
     extend ::Gitlab::Utils::Override
 
+    override :search_filter_input_options
     def search_filter_input_options(type)
       options = super
       options[:data][:'multiple-assignees'] = 'true' if search_multiple_assignees?(type)
@@ -42,6 +43,23 @@ module EE
       end
     end
 
+    # This is a special case for snippet searches in .com.
+    # The scope used to gather the snippets is too wide and
+    # we have to process a lot of them, what leads to time outs.
+    # We're reducing the scope only in .com because the current
+    # one is still valid in smaller installations.
+    # https://gitlab.com/gitlab-org/gitlab/issues/26123
+    override :search_entries_info_template
+    def search_entries_info_template(collection)
+      return super unless gitlab_com_snippet_db_search?
+
+      if collection.total_pages > 1
+        s_("SearchResults|Showing %{from} - %{to} of %{count} %{scope} for \"%{term}\" in your personal and project snippets")
+      else
+        s_("SearchResults|Showing %{count} %{scope} for \"%{term}\" in your personal and project snippets")
+      end
+    end
+
     private
 
     def search_multiple_assignees?(type)
@@ -54,5 +72,13 @@ module EE
     def blob_project_id(blob_result)
       blob_result.dig('_source', 'join_field', 'parent')&.split('_')&.last.to_i
     end
+
+    def gitlab_com_snippet_db_search?
+      @current_user &&
+        @show_snippets &&
+        ::Gitlab.com? &&
+        ::Feature.enabled?(:restricted_snippet_scope_search, default_enabled: true) &&
+        ::Gitlab::CurrentSettings.search_using_elasticsearch?(scope: nil)
+    end
   end
 end

ee/lib/ee/gitlab/snippet_search_results.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module SnippetSearchResults
+      extend ::Gitlab::Utils::Override
+
+      # Special scope for .com
+      # https://gitlab.com/gitlab-org/gitlab/issues/26123
+      override :finder_params
+      def finder_params
+        return super unless ::Gitlab.com?
+        return super unless ::Feature.enabled?(:restricted_snippet_scope_search, default_enabled: true)
+
+        { authorized_and_user_personal: true }
+      end
+    end
+  end
+end

ee/spec/finders/snippets_finder_spec.rb

@@ -3,20 +3,154 @@
 require 'spec_helper'
 
 describe SnippetsFinder do
+  let_it_be(:group) { create(:group, :public) }
+  let_it_be(:project) { create(:project, :public, group: group) }
+  let_it_be(:private_project_snippet) { create(:project_snippet, :private, project: project) }
+  let_it_be(:internal_project_snippet) { create(:project_snippet, :internal, project: project) }
+  let_it_be(:public_project_snippet) { create(:project_snippet, :public, project: project) }
+
+  let(:finder_params) { {} }
+  let(:finder_user) {}
+
+  subject { described_class.new(finder_user, finder_params).execute }
+
   context 'filter by project' do
-    set(:user) { create(:user) }
-    set(:group) { create(:group, :public) }
-    set(:project) { create(:project, :public, group: group) }
-    set(:private_project_snippet) { create(:project_snippet, :private, project: project) }
-    set(:internal_project_snippet) { create(:project_snippet, :internal, project: project) }
-    set(:public_project_snippet) { create(:project_snippet, :public, project: project) }
+    let_it_be(:user) { create(:user, :auditor) }
+
+    let(:finder_params) { { project: project } }
+    let(:finder_user) { user }
 
     it 'returns all snippets for auditor users' do
-      user = create(:user, :auditor)
+      expect(subject).to match_array([private_project_snippet, internal_project_snippet, public_project_snippet])
+    end
+  end
 
-      snippets = described_class.new(user, project: project).execute
+  context 'filter by authorized snippet projects and authored personal' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:other_user) { create(:user) }
+    let_it_be(:other_project) { create(:project) }
+    let_it_be(:private_personal_snippet) { create(:personal_snippet, :private, author: user) }
+    let_it_be(:internal_personal_snippet) { create(:personal_snippet, :internal, author: user) }
+    let_it_be(:public_personal_snippet) { create(:personal_snippet, :public, author: user) }
+    let_it_be(:other_private_personal_snippet) { create(:personal_snippet, :private, author: other_user) }
+    let_it_be(:other_internal_personal_snippet) { create(:personal_snippet, :internal, author: other_user) }
+    let_it_be(:other_public_personal_snippet) { create(:personal_snippet, :public, author: other_user) }
+    let_it_be(:other_private_project_snippet) { create(:project_snippet, :private, project: other_project, author: other_user) }
+    let_it_be(:other_internal_project_snippet) { create(:project_snippet, :internal, project: other_project, author: other_user) }
+    let_it_be(:other_public_project_snippet) { create(:project_snippet, :public, project: other_project, author: other_user) }
+
+    let(:finder_params) { { authorized_and_user_personal: true } }
+    let(:finder_user) { user }
+
+    context 'when no user' do
+      let(:finder_user) {}
+
+      it 'returns only public personal snippets' do
+        expect(subject).to contain_exactly(public_personal_snippet, other_public_personal_snippet)
+      end
+    end
 
-      expect(snippets).to include(private_project_snippet, internal_project_snippet, public_project_snippet)
+    context 'when user is not a member of any project' do
+      it 'returns only user personal snippets' do
+        expect(subject).to match_array([public_personal_snippet, internal_personal_snippet, private_personal_snippet])
+      end
+    end
+
+    context 'when the user is a member of a project' do
+      [:guest, :reporter, :developer, :maintainer].each do |role|
+        it 'returns all the authorized project snippets and authored personal ones' do
+          project.add_role(user, role)
+
+          expect(subject)
+            .to contain_exactly(
+              public_personal_snippet,
+              internal_personal_snippet,
+              private_personal_snippet,
+              public_project_snippet,
+              internal_project_snippet,
+              private_project_snippet
+            )
+        end
+      end
+
+      [:guest, :reporter, :developer, :maintainer].each do |role|
+        it 'returns all the authorized project snippets and authored personal ones' do
+          project.add_role(user, role)
+          other_project.add_role(user, role)
+
+          expect(subject)
+            .to contain_exactly(
+              public_personal_snippet,
+              internal_personal_snippet,
+              private_personal_snippet,
+              public_project_snippet,
+              internal_project_snippet,
+              private_project_snippet,
+              other_private_project_snippet,
+              other_internal_project_snippet,
+              other_public_project_snippet
+            )
+        end
+      end
+
+      context 'when user cannot read_cross_project' do
+        before do
+          project.add_maintainer(user)
+          allow(Ability).to receive(:allowed?)
+                          .with(user, :read_cross_project)
+                          .and_return(false)
+        end
+
+        it 'returns only user personal snippets' do
+          expect(subject).to contain_exactly(public_personal_snippet, internal_personal_snippet, private_personal_snippet)
+        end
+      end
+    end
+
+    context 'when the user is a member of a group' do
+      [:guest, :reporter, :developer, :maintainer].each do |role|
+        it 'returns all the authorized project snippets and authored personal ones' do
+          group.add_user(user, role)
+
+          expect(subject)
+            .to contain_exactly(
+              public_personal_snippet,
+              internal_personal_snippet,
+              private_personal_snippet,
+              public_project_snippet,
+              internal_project_snippet,
+              private_project_snippet
+            )
+        end
+      end
+    end
+
+    context 'when param author is passed' do
+      let(:finder_params) { { author: other_user, authorized_and_user_personal: true } }
+
+      context 'when user is not a member of any project' do
+        it 'returns only the author visible personal snippets to the user' do
+          expect(subject).to contain_exactly(other_public_personal_snippet, other_internal_personal_snippet)
+        end
+      end
+
+      context 'when user is a member of a project' do
+        [:guest, :reporter, :developer, :maintainer].each do |role|
+          it 'returns all the authorized project and personal snippets authored by the author' do
+            project.add_role(user, role)
+            other_project.add_role(user, role)
+
+            expect(subject)
+              .to contain_exactly(
+                other_public_personal_snippet,
+                other_internal_personal_snippet,
+                other_internal_project_snippet,
+                other_public_project_snippet,
+                other_private_project_snippet
+              )
+          end
+        end
+      end
     end
   end
 end

ee/spec/helpers/search_helper_spec.rb

@@ -111,4 +111,68 @@ describe SearchHelper do
       expect(result_projects).to match_array(projects)
     end
   end
+
+  describe '#search_entries_info_template' do
+    let(:com_value) { true }
+    let(:flag_enabled) { true }
+    let(:elasticsearch_enabled) { true }
+    let(:show_snippets) { true }
+    let(:collection) { Kaminari.paginate_array([:foo]).page(1).per(10) }
+    let(:user) { create(:user) }
+    let(:message) { "Showing %{count} %{scope} for \"%{term}\"" }
+    let(:new_message) { message + " in your personal and project snippets" }
+
+    subject { search_entries_info_template(collection) }
+
+    before do
+      @show_snippets = show_snippets
+      @current_user = user
+
+      allow(Gitlab).to receive(:com?).and_return(com_value)
+      stub_feature_flags(restricted_snippet_scope_search: flag_enabled)
+      stub_ee_application_setting(search_using_elasticsearch: elasticsearch_enabled)
+    end
+
+    shared_examples 'returns old message' do
+      it do
+        expect(subject).to eq message
+      end
+    end
+
+    context 'when all requirements are met' do
+      it 'returns a custom message' do
+        expect(subject).to eq new_message
+      end
+    end
+
+    context 'when not in Gitlab.com' do
+      let(:com_value) { false }
+
+      it_behaves_like 'returns old message'
+    end
+
+    context 'when flag restricted_snippet_scope_search is not enabled' do
+      let(:flag_enabled) { false }
+
+      it_behaves_like 'returns old message'
+    end
+
+    context 'when elastic search is not enabled' do
+      let(:elasticsearch_enabled) { false }
+
+      it_behaves_like 'returns old message'
+    end
+
+    context 'when no user is present' do
+      let(:user) { nil }
+
+      it_behaves_like 'returns old message'
+    end
+
+    context 'when not searching for snippets' do
+      let(:show_snippets) { nil }
+
+      it_behaves_like 'returns old message'
+    end
+  end
 end

ee/spec/lib/ee/gitlab/snippet_search_results_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Gitlab::SnippetSearchResults do
+  let!(:snippet) { create(:snippet, content: 'foo', file_name: 'foo') }
+  let(:user) { snippet.author }
+  let(:com_value) { true }
+  let(:flag_enabled) { true }
+
+  subject { described_class.new(user, 'foo').objects('snippet_titles') }
+
+  before do
+    allow(Gitlab).to receive(:com?).and_return(com_value)
+    stub_feature_flags(restricted_snippet_scope_search: flag_enabled)
+  end
+
+  context 'when all requirements are met' do
+    it 'calls the finder with the restrictive scope' do
+      expect(SnippetsFinder).to receive(:new).with(user, authorized_and_user_personal: true).and_call_original
+
+      subject
+    end
+  end
+
+  context 'when not in Gitlab.com' do
+    let(:com_value) { false }
+
+    it 'calls the finder with the restrictive scope' do
+      expect(SnippetsFinder).to receive(:new).with(user, {}).and_call_original
+
+      subject
+    end
+  end
+
+  context 'when flag restricted_snippet_scope_search is not enabled' do
+    let(:flag_enabled) { false }
+
+    it 'calls the finder with the restrictive scope' do
+      expect(SnippetsFinder).to receive(:new).with(user, {}).and_call_original
+
+      subject
+    end
+  end
+end

lib/gitlab/snippet_search_results.rb

@@ -45,7 +45,7 @@ module Gitlab
 
     # rubocop: disable CodeReuse/ActiveRecord
     def snippets
-      SnippetsFinder.new(current_user)
+      SnippetsFinder.new(current_user, finder_params)
         .execute
         .includes(:author)
         .reorder(updated_at: :desc)
@@ -67,5 +67,11 @@ module Gitlab
     def paginated_objects(relation, page)
       relation.page(page).per(per_page)
     end
+
+    def finder_params
+      {}
+    end
   end
 end
+
+Gitlab::SnippetSearchResults.prepend_if_ee('::EE::Gitlab::SnippetSearchResults')

locale/gitlab.pot

@@ -14041,9 +14041,15 @@ msgstr ""
 msgid "SearchResults|Showing %{count} %{scope} for \"%{term}\""
 msgstr ""
 
+msgid "SearchResults|Showing %{count} %{scope} for \"%{term}\" in your personal and project snippets"
+msgstr ""
+
 msgid "SearchResults|Showing %{from} - %{to} of %{count} %{scope} for \"%{term}\""
 msgstr ""
 
+msgid "SearchResults|Showing %{from} - %{to} of %{count} %{scope} for \"%{term}\" in your personal and project snippets"
+msgstr ""
+
 msgid "SearchResults|We couldn't find any %{scope} matching %{term}"
 msgstr ""
 

spec/finders/snippets_finder_spec.rb

@@ -150,6 +150,26 @@ describe SnippetsFinder do
 
         expect(snippets).to contain_exactly(private_project_snippet, internal_project_snippet, public_project_snippet)
       end
+
+      context 'filter by author' do
+        let!(:other_user) { create(:user) }
+        let!(:other_private_project_snippet) { create(:project_snippet, :private, project: project, author: other_user) }
+        let!(:other_internal_project_snippet) { create(:project_snippet, :internal, project: project, author: other_user) }
+        let!(:other_public_project_snippet) { create(:project_snippet, :public, project: project, author: other_user) }
+
+        it 'returns all snippets for project members' do
+          project.add_developer(user)
+
+          snippets = described_class.new(user, author: other_user).execute
+
+          expect(snippets)
+            .to contain_exactly(
+              other_private_project_snippet,
+              other_internal_project_snippet,
+              other_public_project_snippet
+            )
+        end
+      end
     end
 
     context 'when the user cannot read cross project' do