Merge branch '30877-optimize-explore-snippets' into 'master'

Show only personal snippets on explore page

See merge request gitlab-org/gitlab!18092

app/controllers/explore/snippets_controller.rb

@@ -5,7 +5,7 @@ class Explore::SnippetsController < Explore::ApplicationController
   include Gitlab::NoteableMetadata
 
   def index
-    @snippets = SnippetsFinder.new(current_user)
+    @snippets = SnippetsFinder.new(current_user, explore: true)
       .execute
       .page(params[:page])
       .inc_author

app/finders/snippets_finder.rb

@@ -41,13 +41,14 @@
 class SnippetsFinder < UnionFinder
   include FinderMethods
 
-  attr_accessor :current_user, :project, :author, :scope
+  attr_accessor :current_user, :project, :author, :scope, :explore
 
   def initialize(current_user = nil, params = {})
     @current_user = current_user
     @project = params[:project]
     @author = params[:author]
     @scope = params[:scope].to_s
+    @explore = params[:explore]
 
     if project && author
       raise(
@@ -66,13 +67,23 @@ class SnippetsFinder < UnionFinder
   private
 
   def init_collection
-    if project
+    if explore
+      snippets_for_explore
+    elsif project
       snippets_for_a_single_project
     else
       snippets_for_multiple_projects
     end
   end
 
+  # Produces a query that retrieves snippets for the Explore page
+  #
+  # We only show personal snippets here because this page is meant for
+  # discovery, and project snippets are of limited interest here.
+  def snippets_for_explore
+    Snippet.public_to_user(current_user).only_personal_snippets
+  end
+
   # Produces a query that retrieves snippets from multiple projects.
   #
   # The resulting query will, depending on the user's permissions, include the
@@ -86,7 +97,7 @@ class SnippetsFinder < UnionFinder
   # Each collection is constructed in isolation, allowing for greater control
   # over the resulting SQL query.
   def snippets_for_multiple_projects
-    queries = [global_snippets]
+    queries = [personal_snippets]
 
     if Ability.allowed?(current_user, :read_cross_project)
       queries << snippets_of_visible_projects
@@ -100,8 +111,8 @@ class SnippetsFinder < UnionFinder
     Snippet.for_project_with_user(project, current_user)
   end
 
-  def global_snippets
-    snippets_for_author_or_visible_to_user.only_global_snippets
+  def personal_snippets
+    snippets_for_author_or_visible_to_user.only_personal_snippets
   end
 
   # Returns the snippets that the current user (logged in or not) can view.

app/models/snippet.rb

@@ -71,7 +71,7 @@ class Snippet < ApplicationRecord
     end
   end
 
-  def self.only_global_snippets
+  def self.only_personal_snippets
     where(project_id: nil)
   end
 

changelogs/unreleased/30877-optimize-explore-snippets.yml

+---
+title: Show only personal snippets on explore page
+merge_request: 18092
+author:
+type: performance

db/migrate/20191014084150_add_index_on_snippets_project_id_and_visibility_level.rb

+# frozen_string_literal: true
+
+class AddIndexOnSnippetsProjectIdAndVisibilityLevel < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :snippets, [:project_id, :visibility_level]
+  end
+
+  def down
+    remove_concurrent_index :snippets, [:project_id, :visibility_level]
+  end
+end

db/migrate/20191014132931_remove_index_on_snippets_project_id.rb

+# frozen_string_literal: true
+
+class RemoveIndexOnSnippetsProjectId < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index :snippets, [:project_id]
+  end
+
+  def down
+    add_concurrent_index :snippets, [:project_id]
+  end
+end

db/schema.rb

@@ -3449,7 +3449,7 @@ ActiveRecord::Schema.define(version: 2019_10_16_220135) do
     t.index ["author_id"], name: "index_snippets_on_author_id"
     t.index ["content"], name: "index_snippets_on_content_trigram", opclass: :gin_trgm_ops, using: :gin
     t.index ["file_name"], name: "index_snippets_on_file_name_trigram", opclass: :gin_trgm_ops, using: :gin
-    t.index ["project_id"], name: "index_snippets_on_project_id"
+    t.index ["project_id", "visibility_level"], name: "index_snippets_on_project_id_and_visibility_level"
     t.index ["title"], name: "index_snippets_on_title_trigram", opclass: :gin_trgm_ops, using: :gin
     t.index ["updated_at"], name: "index_snippets_on_updated_at"
     t.index ["visibility_level"], name: "index_snippets_on_visibility_level"

ee/app/finders/ee/snippets_finder.rb

@@ -27,7 +27,7 @@ module EE
     #
     # When current_user is nil it returns only public personal snippets
     def snippets_of_authorized_projects_or_personal
-      queries = [restricted_global_snippets]
+      queries = [restricted_personal_snippets]
 
       if current_user && Ability.allowed?(current_user, :read_cross_project)
         queries << snippets_of_authorized_projects
@@ -36,14 +36,14 @@ module EE
       find_union(queries, ::Snippet)
     end
 
-    def restricted_global_snippets
+    def restricted_personal_snippets
       if author
         snippets_for_author
       elsif current_user
         current_user.snippets
       else
         ::Snippet.public_to_user
-      end.only_global_snippets
+      end.only_personal_snippets
     end
   end
 end

spec/finders/snippets_finder_spec.rb

@@ -17,16 +17,27 @@ describe SnippetsFinder do
   end
 
   describe '#execute' do
-    set(:user) { create(:user) }
-    set(:private_personal_snippet) { create(:personal_snippet, :private, author: user) }
-    set(:internal_personal_snippet) { create(:personal_snippet, :internal, author: user) }
-    set(:public_personal_snippet) { create(:personal_snippet, :public, author: user) }
+    let_it_be(:user) { create(:user) }
+    let_it_be(:admin) { create(:admin) }
+    let_it_be(:group) { create(:group, :public) }
+    let_it_be(:project) { create(:project, :public, group: group) }
+
+    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(: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) }
 
     context 'filter by scope' do
       it "returns all snippets for 'all' scope" do
         snippets = described_class.new(user, scope: :all).execute
 
-        expect(snippets).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet)
+        expect(snippets).to contain_exactly(
+          private_personal_snippet, internal_personal_snippet, public_personal_snippet,
+          internal_project_snippet, public_project_snippet
+        )
       end
 
       it "returns all snippets for 'are_private' scope" do
@@ -38,13 +49,13 @@ describe SnippetsFinder do
       it "returns all snippets for 'are_internal' scope" do
         snippets = described_class.new(user, scope: :are_internal).execute
 
-        expect(snippets).to contain_exactly(internal_personal_snippet)
+        expect(snippets).to contain_exactly(internal_personal_snippet, internal_project_snippet)
       end
 
-      it "returns all snippets for 'are_private' scope" do
+      it "returns all snippets for 'are_public' scope" do
         snippets = described_class.new(user, scope: :are_public).execute
 
-        expect(snippets).to contain_exactly(public_personal_snippet)
+        expect(snippets).to contain_exactly(public_personal_snippet, public_project_snippet)
       end
     end
 
@@ -86,7 +97,6 @@ describe SnippetsFinder do
       end
 
       it 'returns all snippets for an admin' do
-        admin = create(:user, :admin)
         snippets = described_class.new(admin, author: user).execute
 
         expect(snippets).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet)
@@ -94,12 +104,6 @@ describe SnippetsFinder do
     end
 
     context 'project snippets' do
-      let(:group) { create(:group, :public) }
-      let(:project) { create(:project, :public, group: group) }
-      let!(:private_project_snippet) { create(:project_snippet, :private, project: project) }
-      let!(:internal_project_snippet) { create(:project_snippet, :internal, project: project) }
-      let!(:public_project_snippet) { create(:project_snippet, :public, project: project) }
-
       it 'returns public personal and project snippets for unauthorized user' do
         snippets = described_class.new(nil, project: project).execute
 
@@ -147,7 +151,6 @@ describe SnippetsFinder do
       end
 
       it 'returns all snippets for an admin' do
-        admin = create(:user, :admin)
         snippets = described_class.new(admin, project: project).execute
 
         expect(snippets).to contain_exactly(private_project_snippet, internal_project_snippet, public_project_snippet)
@@ -174,6 +177,30 @@ describe SnippetsFinder do
       end
     end
 
+    context 'explore snippets' do
+      it 'returns only public personal snippets for unauthenticated users' do
+        snippets = described_class.new(nil, explore: true).execute
+
+        expect(snippets).to contain_exactly(public_personal_snippet)
+      end
+
+      it 'also returns internal personal snippets for authenticated users' do
+        snippets = described_class.new(user, explore: true).execute
+
+        expect(snippets).to contain_exactly(
+          internal_personal_snippet, public_personal_snippet
+        )
+      end
+
+      it 'returns all personal snippets for admins' do
+        snippets = described_class.new(admin, explore: true).execute
+
+        expect(snippets).to contain_exactly(
+          private_personal_snippet, internal_personal_snippet, public_personal_snippet
+        )
+      end
+    end
+
     context 'when the user cannot read cross project' do
       before do
         allow(Ability).to receive(:allowed?).and_call_original

spec/models/snippet_spec.rb

@@ -183,12 +183,12 @@ describe Snippet do
     end
   end
 
-  describe '.only_global_snippets' do
+  describe '.only_personal_snippets' do
     it 'returns snippets not associated with any projects' do
       create(:project_snippet)
 
       snippet = create(:snippet)
-      snippets = described_class.only_global_snippets
+      snippets = described_class.only_personal_snippets
 
       expect(snippets).to eq([snippet])
     end