Show only personal snippets on explore page

The "Explore snippets" page loads very slowly with a large number of
snippets, due to the complexity of the authorization checks for project
snippets.

Since project snippets are of limited interest on the explore page, we
can restrict the query to personal snippets which are visible to the
current user.

We're also replacing the index on `snippets.project_id` with a combined
index on `(project_id, visibility_level)` to further speed up these
queries.

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