Merge branch '65988-optimize-snippet-listings' into 'master'

Optimize queries for snippet listings

See merge request gitlab-org/gitlab-ce!32576

app/controllers/concerns/issuable_collections.rb

@@ -2,6 +2,7 @@
 
 module IssuableCollections
   extend ActiveSupport::Concern
+  include PaginatedCollection
   include SortingHelper
   include SortingPreference
   include Gitlab::IssuableMetadata
@@ -17,8 +18,11 @@ module IssuableCollections
   def set_issuables_index
     @issuables = issuables_collection
 
-    set_pagination
-    return if redirect_out_of_range(@total_pages)
+    unless pagination_disabled?
+      set_pagination
+
+      return if redirect_out_of_range(@issuables, @total_pages)
+    end
 
     if params[:label_name].present? && @project
       labels_params = { project_id: @project.id, title: params[:label_name] }
@@ -38,12 +42,10 @@ module IssuableCollections
   end
 
   def set_pagination
-    return if pagination_disabled?
-
     @issuables          = @issuables.page(params[:page])
     @issuables          = per_page_for_relative_position if params[:sort] == 'relative_position'
     @issuable_meta_data = issuable_meta_data(@issuables, collection_type, current_user)
-    @total_pages        = issuable_page_count
+    @total_pages        = issuable_page_count(@issuables)
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
@@ -57,20 +59,8 @@ module IssuableCollections
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
-  def redirect_out_of_range(total_pages)
-    return false if total_pages.nil? || total_pages.zero?
-
-    out_of_range = @issuables.current_page > total_pages # rubocop:disable Gitlab/ModuleWithInstanceVariables
-
-    if out_of_range
-      redirect_to(url_for(safe_params.merge(page: total_pages, only_path: true)))
-    end
-
-    out_of_range
-  end
-
-  def issuable_page_count
-    page_count_for_relation(@issuables, finder.row_count) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  def issuable_page_count(relation)
+    page_count_for_relation(relation, finder.row_count)
   end
 
   def page_count_for_relation(relation, row_count)

app/controllers/concerns/paginated_collection.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module PaginatedCollection
+  extend ActiveSupport::Concern
+
+  private
+
+  def redirect_out_of_range(collection, total_pages = collection.total_pages)
+    return false if total_pages.zero?
+
+    out_of_range = collection.current_page > total_pages
+
+    if out_of_range
+      redirect_to(url_for(safe_params.merge(page: total_pages, only_path: true)))
+    end
+
+    out_of_range
+  end
+end

app/controllers/dashboard/snippets_controller.rb

@@ -1,14 +1,19 @@
 # frozen_string_literal: true
 
 class Dashboard::SnippetsController < Dashboard::ApplicationController
+  include PaginatedCollection
+  include Gitlab::NoteableMetadata
+
   skip_cross_project_access_check :index
 
   def index
-    @snippets = SnippetsFinder.new(
-      current_user,
-      author: current_user,
-      scope: params[:scope]
-    ).execute
-    @snippets = @snippets.page(params[:page])
+    @snippets = SnippetsFinder.new(current_user, author: current_user, scope: params[:scope])
+      .execute
+      .page(params[:page])
+      .inc_author
+
+    return if redirect_out_of_range(@snippets)
+
+    @noteable_meta_data = noteable_meta_data(@snippets, 'Snippet')
   end
 end

app/controllers/dashboard/todos_controller.rb

@@ -2,6 +2,7 @@
 
 class Dashboard::TodosController < Dashboard::ApplicationController
   include ActionView::Helpers::NumberHelper
+  include PaginatedCollection
 
   before_action :authorize_read_project!, only: :index
   before_action :authorize_read_group!, only: :index
@@ -12,7 +13,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     @todos = @todos.page(params[:page])
     @todos = @todos.with_entity_associations
 
-    return if redirect_out_of_range(@todos)
+    return if redirect_out_of_range(@todos, todos_page_count(@todos))
   end
 
   def destroy
@@ -82,28 +83,15 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     }
   end
 
+  def todos_page_count(todos)
+    if todo_params.except(:sort, :page).empty? # rubocop: disable CodeReuse/ActiveRecord
+      (current_user.todos_pending_count.to_f / todos.limit_value).ceil
+    else
+      todos.total_pages
+    end
+  end
+
   def todo_params
     params.permit(:action_id, :author_id, :project_id, :type, :sort, :state, :group_id)
   end
-
-  # rubocop: disable CodeReuse/ActiveRecord
-  def redirect_out_of_range(todos)
-    total_pages =
-      if todo_params.except(:sort, :page).empty?
-        (current_user.todos_pending_count.to_f / todos.limit_value).ceil
-      else
-        todos.total_pages
-      end
-
-    return false if total_pages.zero?
-
-    out_of_range = todos.current_page > total_pages
-
-    if out_of_range
-      redirect_to url_for(safe_params.merge(page: total_pages, only_path: true))
-    end
-
-    out_of_range
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
 end

app/controllers/explore/snippets_controller.rb

@@ -1,8 +1,17 @@
 # frozen_string_literal: true
 
 class Explore::SnippetsController < Explore::ApplicationController
+  include PaginatedCollection
+  include Gitlab::NoteableMetadata
+
   def index
-    @snippets = SnippetsFinder.new(current_user).execute
-    @snippets = @snippets.page(params[:page])
+    @snippets = SnippetsFinder.new(current_user)
+      .execute
+      .page(params[:page])
+      .inc_author
+
+    return if redirect_out_of_range(@snippets)
+
+    @noteable_meta_data = noteable_meta_data(@snippets, 'Snippet')
   end
 end

app/controllers/projects/snippets_controller.rb

@@ -6,6 +6,8 @@ class Projects::SnippetsController < Projects::ApplicationController
   include SpammableActions
   include SnippetsActions
   include RendersBlob
+  include PaginatedCollection
+  include Gitlab::NoteableMetadata
 
   skip_before_action :verify_authenticity_token,
     if: -> { action_name == 'show' && js_request? }
@@ -28,15 +30,14 @@ class Projects::SnippetsController < Projects::ApplicationController
   respond_to :html
 
   def index
-    @snippets = SnippetsFinder.new(
-      current_user,
-      project: @project,
-      scope: params[:scope]
-    ).execute
-    @snippets = @snippets.page(params[:page])
-    if @snippets.out_of_range? && @snippets.total_pages != 0
-      redirect_to project_snippets_path(@project, page: @snippets.total_pages)
-    end
+    @snippets = SnippetsFinder.new(current_user, project: @project, scope: params[:scope])
+      .execute
+      .page(params[:page])
+      .inc_author
+
+    return if redirect_out_of_range(@snippets)
+
+    @noteable_meta_data = noteable_meta_data(@snippets, 'Snippet')
   end
 
   def new

app/controllers/snippets_controller.rb

@@ -7,6 +7,8 @@ class SnippetsController < ApplicationController
   include SnippetsActions
   include RendersBlob
   include PreviewMarkdown
+  include PaginatedCollection
+  include Gitlab::NoteableMetadata
 
   skip_before_action :verify_authenticity_token,
     if: -> { action_name == 'show' && js_request? }
@@ -32,7 +34,13 @@ class SnippetsController < ApplicationController
       @user = UserFinder.new(params[:username]).find_by_username!
 
       @snippets = SnippetsFinder.new(current_user, author: @user, scope: params[:scope])
-        .execute.page(params[:page])
+        .execute
+        .page(params[:page])
+        .inc_author
+
+      return if redirect_out_of_range(@snippets)
+
+      @noteable_meta_data = noteable_meta_data(@snippets, 'Snippet')
 
       render 'index'
     else

app/controllers/users_controller.rb

@@ -4,6 +4,7 @@ class UsersController < ApplicationController
   include RoutableActions
   include RendersMemberAccess
   include ControllerWithCrossProjectAccessCheck
+  include Gitlab::NoteableMetadata
 
   requires_cross_project_access show: false,
                                 groups: false,
@@ -165,11 +166,12 @@ class UsersController < ApplicationController
   end
 
   def load_snippets
-    @snippets = SnippetsFinder.new(
-      current_user,
-      author: user,
-      scope: params[:scope]
-    ).execute.page(params[:page])
+    @snippets = SnippetsFinder.new(current_user, author: user, scope: params[:scope])
+      .execute
+      .page(params[:page])
+      .inc_author
+
+    @noteable_meta_data = noteable_meta_data(@snippets, 'Snippet')
   end
 
   def build_canonical_path(user)

app/models/concerns/noteable.rb

@@ -3,6 +3,10 @@
 module Noteable
   extend ActiveSupport::Concern
 
+  # This object is used to gather noteable meta data for list displays
+  # avoiding n+1 queries and improving performance.
+  NoteableMeta = Struct.new(:user_notes_count)
+
   class_methods do
     # `Noteable` class names that support replying to individual notes.
     def replyable_types

app/models/snippet.rb

@@ -55,6 +55,7 @@ class Snippet < ApplicationRecord
   scope :are_public, -> { where(visibility_level: Snippet::PUBLIC) }
   scope :public_and_internal, -> { where(visibility_level: [Snippet::PUBLIC, Snippet::INTERNAL]) }
   scope :fresh, -> { order("created_at DESC") }
+  scope :inc_author, -> { includes(:author) }
   scope :inc_relations_for_view, -> { includes(author: :status) }
 
   participant :author

app/views/shared/snippets/_list.html.haml

@@ -1,12 +1,11 @@
 - remote = local_assigns.fetch(:remote, false)
 - link_project = local_assigns.fetch(:link_project, false)
 
-- if @snippets.exists?
+- if @snippets.to_a.empty?
+  .nothing-here-block= s_("SnippetsEmptyState|No snippets found")
+- else
   .snippets-list-holder
     %ul.content-list
       = render partial: 'shared/snippets/snippet', collection: @snippets, locals: { link_project: link_project }
 
     = paginate @snippets, theme: 'gitlab', remote: remote
-
-- else
-  .nothing-here-block= s_("SnippetsEmptyState|No snippets found")

app/views/shared/snippets/_snippet.html.haml

@@ -1,4 +1,5 @@
 - link_project = local_assigns.fetch(:link_project, false)
+- notes_count = @noteable_meta_data[snippet.id].user_notes_count
 
 %li.snippet-row
   = image_tag avatar_icon_for_user(snippet.author), class: "avatar s40 d-none d-sm-block", alt: ''
@@ -12,10 +13,9 @@
 
     %ul.controls
       %li
-        - note_count = snippet.notes.user.count
-        = link_to reliable_snippet_path(snippet, anchor: 'notes'), class: ('no-comments' if note_count.zero?) do
+        = link_to reliable_snippet_path(snippet, anchor: 'notes'), class: ('no-comments' if notes_count.zero?) do
           = icon('comments')
-          = note_count
+          = notes_count
       %li
         %span.sr-only
           = visibility_level_label(snippet.visibility_level)

changelogs/unreleased/65988-optimize-snippet-listings.yml

@@ -0,0 +1,5 @@
+---
+title: Optimize queries for snippet listings
+merge_request: 32576
+author:
+type: performance

lib/gitlab/issuable_metadata.rb

@@ -19,7 +19,7 @@ module Gitlab
 
       return {} if issuable_ids.empty?
 
-      issuable_note_count = ::Note.count_for_collection(issuable_ids, collection_type)
+      issuable_notes_count = ::Note.count_for_collection(issuable_ids, collection_type)
       issuable_votes_count = ::AwardEmoji.votes_for_collection(issuable_ids, collection_type)
       issuable_merge_requests_count =
         if collection_type == 'Issue'
@@ -31,7 +31,7 @@ module Gitlab
       issuable_ids.each_with_object({}) do |id, issuable_meta|
         downvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.downvote? }
         upvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.upvote? }
-        notes = issuable_note_count.find { |notes| notes.noteable_id == id }
+        notes = issuable_notes_count.find { |notes| notes.noteable_id == id }
         merge_requests = issuable_merge_requests_count.find { |mr| mr.first == id }
 
         issuable_meta[id] = ::Issuable::IssuableMeta.new(

lib/gitlab/noteable_metadata.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module NoteableMetadata
+    def noteable_meta_data(noteable_collection, collection_type)
+      # ActiveRecord uses Object#extend for null relations.
+      if !(noteable_collection.singleton_class < ActiveRecord::NullRelation) &&
+          noteable_collection.respond_to?(:limit_value) &&
+          noteable_collection.limit_value.nil?
+
+        raise 'Collection must have a limit applied for preloading meta-data'
+      end
+
+      # map has to be used here since using pluck or select will
+      # throw an error when ordering noteables which inserts
+      # a new order into the collection.
+      # We cannot use reorder to not mess up the paginated collection.
+      noteable_ids = noteable_collection.map(&:id)
+
+      return {} if noteable_ids.empty?
+
+      noteable_notes_count = ::Note.count_for_collection(noteable_ids, collection_type)
+
+      noteable_ids.each_with_object({}) do |id, noteable_meta|
+        notes = noteable_notes_count.find { |notes| notes.noteable_id == id }
+
+        noteable_meta[id] = ::Noteable::NoteableMeta.new(
+          notes.try(:count).to_i
+        )
+      end
+    end
+  end
+end

spec/controllers/dashboard/snippets_controller_spec.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Dashboard::SnippetsController do
+  let(:user) { create(:user) }
+
+  before do
+    sign_in(user)
+  end
+
+  describe 'GET #index' do
+    it_behaves_like 'paginated collection' do
+      let(:collection) { Snippet.all }
+
+      before do
+        create(:personal_snippet, :public, author: user)
+      end
+    end
+  end
+end

spec/controllers/dashboard/todos_controller_spec.rb

@@ -82,35 +82,15 @@ describe Dashboard::TodosController do
       end
     end
 
-    context 'when using pagination' do
-      let(:last_page) { user.todos.page.total_pages }
+    it_behaves_like 'paginated collection' do
       let!(:issues) { create_list(:issue, 3, project: project, assignees: [user]) }
+      let(:collection) { user.todos }
 
       before do
         issues.each { |issue| todo_service.new_issue(issue, user) }
         allow(Kaminari.config).to receive(:default_per_page).and_return(2)
       end
 
-      it 'redirects to last_page if page number is larger than number of pages' do
-        get :index, params: { page: (last_page + 1).to_param }
-
-        expect(response).to redirect_to(dashboard_todos_path(page: last_page))
-      end
-
-      it 'goes to the correct page' do
-        get :index, params: { page: last_page }
-
-        expect(assigns(:todos).current_page).to eq(last_page)
-        expect(response).to have_gitlab_http_status(200)
-      end
-
-      it 'does not redirect to external sites when provided a host field' do
-        external_host = "www.example.com"
-        get :index, params: { page: (last_page + 1).to_param, host: external_host }
-
-        expect(response).to redirect_to(dashboard_todos_path(page: last_page))
-      end
-
       context 'when providing no filters' do
         it 'does not perform a query to get the page count, but gets that from the user' do
           allow(controller).to receive(:current_user).and_return(user)

spec/controllers/explore/snippets_controller_spec.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Explore::SnippetsController do
+  describe 'GET #index' do
+    it_behaves_like 'paginated collection' do
+      let(:collection) { Snippet.all }
+
+      before do
+        create(:personal_snippet, :public)
+      end
+    end
+  end
+end

spec/controllers/projects/issues_controller_spec.rb

@@ -71,9 +71,16 @@ describe Projects::IssuesController do
       end
     end
 
-    context 'with page param' do
-      let(:last_page) { project.issues.page.total_pages }
+    it_behaves_like 'paginated collection' do
       let!(:issue_list) { create_list(:issue, 2, project: project) }
+      let(:collection) { project.issues }
+      let(:params) do
+        {
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          state: 'opened'
+        }
+      end
 
       before do
         sign_in(user)
@@ -81,51 +88,10 @@ describe Projects::IssuesController do
         allow(Kaminari.config).to receive(:default_per_page).and_return(1)
       end
 
-      it 'redirects to last_page if page number is larger than number of pages' do
-        get :index,
-          params: {
-            namespace_id: project.namespace.to_param,
-            project_id: project,
-            page: (last_page + 1).to_param
-          }
-
-        expect(response).to redirect_to(namespace_project_issues_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope]))
-      end
-
-      it 'redirects to specified page' do
-        get :index,
-          params: {
-            namespace_id: project.namespace.to_param,
-            project_id: project,
-            page: last_page.to_param
-          }
-
-        expect(assigns(:issues).current_page).to eq(last_page)
-        expect(response).to have_gitlab_http_status(200)
-      end
-
-      it 'does not redirect to external sites when provided a host field' do
-        external_host = "www.example.com"
-        get :index,
-          params: {
-            namespace_id: project.namespace.to_param,
-            project_id: project,
-            page: (last_page + 1).to_param,
-            host: external_host
-          }
-
-        expect(response).to redirect_to(namespace_project_issues_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope]))
-      end
-
       it 'does not use pagination if disabled' do
         allow(controller).to receive(:pagination_disabled?).and_return(true)
 
-        get :index,
-          params: {
-            namespace_id: project.namespace.to_param,
-            project_id: project,
-            page: (last_page + 1).to_param
-          }
+        get :index, params: params.merge(page: last_page + 1)
 
         expect(response).to have_gitlab_http_status(200)
         expect(assigns(:issues).size).to eq(2)

spec/controllers/projects/snippets_controller_spec.rb

@@ -13,31 +13,17 @@ describe Projects::SnippetsController do
   end
 
   describe 'GET #index' do
-    context 'when page param' do
-      let(:last_page) { project.snippets.page.total_pages }
-      let!(:project_snippet) { create(:project_snippet, :public, project: project, author: user) }
-
-      it 'redirects to last_page if page number is larger than number of pages' do
-        get :index,
-          params: {
-            namespace_id: project.namespace,
-            project_id: project,
-            page: (last_page + 1).to_param
-          }
-
-        expect(response).to redirect_to(namespace_project_snippets_path(page: last_page))
+    it_behaves_like 'paginated collection' do
+      let(:collection) { project.snippets }
+      let(:params) do
+        {
+          namespace_id: project.namespace,
+          project_id: project
+        }
       end
 
-      it 'redirects to specified page' do
-        get :index,
-          params: {
-            namespace_id: project.namespace,
-            project_id: project,
-            page: last_page.to_param
-          }
-
-        expect(assigns(:snippets).current_page).to eq(last_page)
-        expect(response).to have_gitlab_http_status(200)
+      before do
+        create(:project_snippet, :public, project: project, author: user)
       end
     end
 

spec/controllers/snippets_controller_spec.rb

@@ -9,6 +9,15 @@ describe SnippetsController do
     let(:user) { create(:user) }
 
     context 'when username parameter is present' do
+      it_behaves_like 'paginated collection' do
+        let(:collection) { Snippet.all }
+        let(:params) { { username: user.username } }
+
+        before do
+          create(:personal_snippet, :public, author: user)
+        end
+      end
+
       it 'renders snippets of a user when username is present' do
         get :index, params: { username: user.username }
 

spec/lib/gitlab/noteable_metadata_spec.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::NoteableMetadata do
+  subject { Class.new { include Gitlab::NoteableMetadata }.new }
+
+  it 'returns an empty Hash if an empty collection is provided' do
+    expect(subject.noteable_meta_data(Snippet.none, 'Snippet')).to eq({})
+  end
+
+  it 'raises an error when given a collection with no limit' do
+    expect { subject.noteable_meta_data(Snippet.all, 'Snippet') }.to raise_error(/must have a limit/)
+  end
+
+  context 'snippets' do
+    let!(:snippet) { create(:personal_snippet) }
+    let!(:other_snippet) { create(:personal_snippet) }
+    let!(:note) { create(:note, noteable: snippet) }
+
+    it 'aggregates stats on snippets' do
+      data = subject.noteable_meta_data(Snippet.all.limit(10), 'Snippet')
+
+      expect(data.count).to eq(2)
+      expect(data[snippet.id].user_notes_count).to eq(1)
+      expect(data[other_snippet.id].user_notes_count).to eq(0)
+    end
+  end
+end

spec/support/shared_examples/controllers/paginated_collection_shared_examples.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+shared_examples 'paginated collection' do
+  let(:collection) { nil }
+  let(:last_page) { collection.page.total_pages }
+  let(:action) { :index }
+  let(:params) { {} }
+
+  it 'renders a page number that is not ouf of range' do
+    get action, params: params.merge(page: last_page)
+
+    expect(response).to have_gitlab_http_status(200)
+  end
+
+  it 'redirects to last_page if page number is larger than number of pages' do
+    get action, params: params.merge(page: last_page + 1)
+
+    expect(response).to redirect_to(params.merge(page: last_page))
+  end
+
+  it 'does not redirect to external sites when provided a host field' do
+    external_host = 'www.example.com'
+
+    get action, params: params.merge(page: last_page + 1, host: external_host)
+
+    expect(response).to redirect_to(params.merge(page: last_page))
+  end
+end