Merge branch 'fj-add-filters-to-snippets-finder' into 'master'

Add new filters to SnippetsFinder

See merge request gitlab-org/gitlab!20767

app/finders/snippets_finder.rb

@@ -40,15 +40,14 @@
 # Any other value will be ignored.
 class SnippetsFinder < UnionFinder
   include FinderMethods
+  include Gitlab::Utils::StrongMemoize
 
-  attr_accessor :current_user, :project, :author, :scope, :explore
+  attr_accessor :current_user, :params
+  delegate :explore, :only_personal, :only_project, :scope, to: :params
 
   def initialize(current_user = nil, params = {})
     @current_user = current_user
-    @project = params[:project]
-    @author = params[:author]
-    @scope = params[:scope].to_s
-    @explore = params[:explore]
+    @params = OpenStruct.new(params)
 
     if project && author
       raise(
@@ -60,8 +59,15 @@ class SnippetsFinder < UnionFinder
   end
 
   def execute
-    base = init_collection
-    base.with_optional_visibility(visibility_from_scope).fresh
+    # The snippet query can be expensive, therefore if the
+    # author or project params have been passed and they don't
+    # exist, it's better to return
+    return Snippet.none if author.nil? && params[:author].present?
+    return Snippet.none if project.nil? && params[:project].present?
+
+    items = init_collection
+    items = by_ids(items)
+    items.with_optional_visibility(visibility_from_scope).fresh
   end
 
   private
@@ -69,10 +75,12 @@ class SnippetsFinder < UnionFinder
   def init_collection
     if explore
       snippets_for_explore
+    elsif only_personal
+      personal_snippets
     elsif project
       snippets_for_a_single_project
     else
-      snippets_for_multiple_projects
+      snippets_for_personal_and_multiple_projects
     end
   end
 
@@ -96,8 +104,9 @@ class SnippetsFinder < UnionFinder
   #
   # Each collection is constructed in isolation, allowing for greater control
   # over the resulting SQL query.
-  def snippets_for_multiple_projects
-    queries = [personal_snippets]
+  def snippets_for_personal_and_multiple_projects
+    queries = []
+    queries << personal_snippets unless only_project
 
     if Ability.allowed?(current_user, :read_cross_project)
       queries << snippets_of_visible_projects
@@ -158,7 +167,7 @@ class SnippetsFinder < UnionFinder
   end
 
   def visibility_from_scope
-    case scope
+    case scope.to_s
     when 'are_private'
       Snippet::PRIVATE
     when 'are_internal'
@@ -169,6 +178,28 @@ class SnippetsFinder < UnionFinder
       nil
     end
   end
+
+  def by_ids(items)
+    return items unless params[:ids].present?
+
+    items.id_in(params[:ids])
+  end
+
+  def author
+    strong_memoize(:author) do
+      next unless params[:author].present?
+
+      params[:author].is_a?(User) ? params[:author] : User.find_by_id(params[:author])
+    end
+  end
+
+  def project
+    strong_memoize(:project) do
+      next unless params[:project].present?
+
+      params[:project].is_a?(Project) ? params[:project] : Project.find_by_id(params[:project])
+    end
+  end
 end
 
 SnippetsFinder.prepend_if_ee('EE::SnippetsFinder')

changelogs/unreleased/fj-add-filters-to-snippets-finder.yml

+---
+title: Add more filters to SnippetsFinder
+merge_request: 20767
+author:
+type: changed

ee/app/finders/ee/snippets_finder.rb

@@ -27,10 +27,11 @@ module EE
     #
     # When current_user is nil it returns only public personal snippets
     def snippets_of_authorized_projects_or_personal
-      queries = [restricted_personal_snippets]
+      queries = []
+      queries << restricted_personal_snippets unless only_project
 
       if current_user && Ability.allowed?(current_user, :read_cross_project)
-        queries << snippets_of_authorized_projects
+        queries << snippets_of_authorized_projects unless only_personal
       end
 
       find_union(queries, ::Snippet)

ee/spec/finders/snippets_finder_spec.rb

@@ -152,5 +152,35 @@ describe SnippetsFinder do
         end
       end
     end
+
+    context 'when only_personal is passed' do
+      let(:finder_params) { { authorized_and_user_personal: true, only_personal: true } }
+
+      it 'returns only personal snippets' do
+        group.add_maintainer(user)
+
+        expect(subject)
+          .to contain_exactly(
+            public_personal_snippet,
+            internal_personal_snippet,
+            private_personal_snippet
+          )
+      end
+    end
+
+    context 'when only_project is passed' do
+      let(:finder_params) { { authorized_and_user_personal: true, only_project: true } }
+
+      it 'returns only project snippets' do
+        group.add_maintainer(user)
+
+        expect(subject)
+          .to contain_exactly(
+            public_project_snippet,
+            internal_project_snippet,
+            private_project_snippet
+          )
+      end
+    end
   end
 end

spec/finders/snippets_finder_spec.rb

@@ -60,10 +60,20 @@ describe SnippetsFinder do
     end
 
     context 'filter by author' do
-      it 'returns all public and internal snippets' do
-        snippets = described_class.new(create(:user), author: user).execute
+      context 'when the author is a User object' do
+        it 'returns all public and internal snippets' do
+          snippets = described_class.new(create(:user), author: user).execute
 
-        expect(snippets).to contain_exactly(internal_personal_snippet, public_personal_snippet)
+          expect(snippets).to contain_exactly(internal_personal_snippet, public_personal_snippet)
+        end
+      end
+
+      context 'when the author is the User id' do
+        it 'returns all public and internal snippets' do
+          snippets = described_class.new(create(:user), author: user.id).execute
+
+          expect(snippets).to contain_exactly(internal_personal_snippet, public_personal_snippet)
+        end
       end
 
       it 'returns internal snippets' do
@@ -101,13 +111,33 @@ describe SnippetsFinder do
 
         expect(snippets).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet)
       end
+
+      context 'when author is not valid' do
+        it 'returns quickly' do
+          finder = described_class.new(admin, author: 1234)
+
+          expect(finder).not_to receive(:init_collection)
+          expect(Snippet).to receive(:none).and_call_original
+          expect(finder.execute).to be_empty
+        end
+      end
     end
 
-    context 'project snippets' do
-      it 'returns public personal and project snippets for unauthorized user' do
-        snippets = described_class.new(nil, project: project).execute
+    context 'filter by project' do
+      context 'when project is a Project object' do
+        it 'returns public personal and project snippets for unauthorized user' do
+          snippets = described_class.new(nil, project: project).execute
 
-        expect(snippets).to contain_exactly(public_project_snippet)
+          expect(snippets).to contain_exactly(public_project_snippet)
+        end
+      end
+
+      context 'when project is a Project id' do
+        it 'returns public personal and project snippets for unauthorized user' do
+          snippets = described_class.new(nil, project: project.id).execute
+
+          expect(snippets).to contain_exactly(public_project_snippet)
+        end
       end
 
       it 'returns public and internal snippets for non project members' do
@@ -175,6 +205,49 @@ describe SnippetsFinder do
             )
         end
       end
+
+      context 'when project is not valid' do
+        it 'returns quickly' do
+          finder = described_class.new(admin, project: 1234)
+
+          expect(finder).not_to receive(:init_collection)
+          expect(Snippet).to receive(:none).and_call_original
+          expect(finder.execute).to be_empty
+        end
+      end
+    end
+
+    context 'filter by snippet type' do
+      context 'when filtering by only_personal snippet' do
+        it 'returns only personal snippet' do
+          snippets = described_class.new(admin, only_personal: true).execute
+
+          expect(snippets).to contain_exactly(private_personal_snippet,
+                                              internal_personal_snippet,
+                                              public_personal_snippet)
+        end
+      end
+
+      context 'when filtering by only_project snippet' do
+        it 'returns only project snippet' do
+          snippets = described_class.new(admin, only_project: true).execute
+
+          expect(snippets).to contain_exactly(private_project_snippet,
+                                              internal_project_snippet,
+                                              public_project_snippet)
+        end
+      end
+    end
+
+    context 'filtering by ids' do
+      it 'returns only personal snippet' do
+        snippets = described_class.new(
+          admin, ids: [private_personal_snippet.id,
+                       internal_personal_snippet.id]
+        ).execute
+
+        expect(snippets).to contain_exactly(private_personal_snippet, internal_personal_snippet)
+      end
     end
 
     context 'explore snippets' do