Merge branch 'mr-index-page-performance' into 'master'

Re-use issue/MR counts for the pagination system

Closes #27168

See merge request !13805

app/controllers/concerns/issuable_collections.rb

@@ -36,6 +36,34 @@ module IssuableCollections
     @merge_requests_finder ||= issuable_finder_for(MergeRequestsFinder)
   end
 
+  def redirect_out_of_range(relation, total_pages)
+    return false if total_pages.zero?
+
+    out_of_range = relation.current_page > total_pages
+
+    if out_of_range
+      redirect_to(url_for(params.merge(page: total_pages, only_path: true)))
+    end
+
+    out_of_range
+  end
+
+  def issues_page_count(relation)
+    page_count_for_relation(relation, issues_finder.row_count)
+  end
+
+  def merge_requests_page_count(relation)
+    page_count_for_relation(relation, merge_requests_finder.row_count)
+  end
+
+  def page_count_for_relation(relation, row_count)
+    limit = relation.limit_value.to_f
+
+    return 1 if limit.zero?
+
+    (row_count.to_f / limit).ceil
+  end
+
   def issuable_finder_for(finder_class)
     finder_class.new(current_user, filter_params)
   end

app/controllers/projects/issues_controller.rb

@@ -27,10 +27,9 @@ class Projects::IssuesController < Projects::ApplicationController
     @issues             = issues_collection
     @issues             = @issues.page(params[:page])
     @issuable_meta_data = issuable_meta_data(@issues, @collection_type)
+    @total_pages        = issues_page_count(@issues)
 
-    if @issues.out_of_range? && @issues.total_pages != 0
-      return redirect_to url_for(params.merge(page: @issues.total_pages, only_path: true))
-    end
+    return if redirect_out_of_range(@issues, @total_pages)
 
     if params[:label_name].present?
       @labels = LabelsFinder.new(current_user, project_id: @project.id, title: params[:label_name]).execute

app/controllers/projects/merge_requests_controller.rb

@@ -18,10 +18,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @merge_requests     = @merge_requests.page(params[:page])
     @merge_requests     = @merge_requests.preload(merge_request_diff: :merge_request)
     @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type)
+    @total_pages        = merge_requests_page_count(@merge_requests)
 
-    if @merge_requests.out_of_range? && @merge_requests.total_pages != 0
-      return redirect_to url_for(params.merge(page: @merge_requests.total_pages, only_path: true))
-    end
+    return if redirect_out_of_range(@merge_requests, @total_pages)
 
     if params[:label_name].present?
       labels_params = { project_id: @project.id, title: params[:label_name] }

app/finders/issuable_finder.rb

@@ -61,6 +61,10 @@ class IssuableFinder
     execute.find_by(*params)
   end
 
+  def row_count
+    Gitlab::IssuablesCountForState.new(self).for_state_or_opened(params[:state])
+  end
+
   # We often get counts for each state by running a query per state, and
   # counting those results. This is typically slower than running one query
   # (even if that query is slower than any of the individual state queries) and

app/helpers/issuables_helper.rb

@@ -240,7 +240,8 @@ module IssuablesHelper
 
   def issuables_count_for_state(issuable_type, state)
     finder = public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend
-    finder.count_by_state[state]
+
+    Gitlab::IssuablesCountForState.new(finder)[state]
   end
 
   def close_issuable_url(issuable)

app/views/projects/issues/_issues.html.haml

@@ -4,4 +4,4 @@
     = render 'shared/empty_states/issues'
 
 - if @issues.present?
-  = paginate @issues, theme: "gitlab"
+  = paginate @issues, theme: "gitlab", total_pages: @total_pages

app/views/projects/merge_requests/_merge_requests.html.haml

@@ -5,4 +5,4 @@
     = render 'shared/empty_states/merge_requests'
 
 - if @merge_requests.present?
-  = paginate @merge_requests, theme: "gitlab"
+  = paginate @merge_requests, theme: "gitlab", total_pages: @total_pages

changelogs/unreleased/mr-index-page-performance.yml

+---
+title: Re-use issue/MR counts for the pagination system
+merge_request:
+author:
+type: other

lib/gitlab/issuables_count_for_state.rb

+module Gitlab
+  # Class for counting and caching the number of issuables per state.
+  class IssuablesCountForState
+    # The name of the RequestStore cache key.
+    CACHE_KEY = :issuables_count_for_state
+
+    # The state values that can be safely casted to a Symbol.
+    STATES = %w[opened closed merged all].freeze
+
+    # finder - The finder class to use for retrieving the issuables.
+    def initialize(finder)
+      @finder = finder
+      @cache =
+        if RequestStore.active?
+          RequestStore[CACHE_KEY] ||= initialize_cache
+        else
+          initialize_cache
+        end
+    end
+
+    def for_state_or_opened(state = nil)
+      self[state || :opened]
+    end
+
+    # Returns the count for the given state.
+    #
+    # state - The name of the state as either a String or a Symbol.
+    #
+    # Returns an Integer.
+    def [](state)
+      state = state.to_sym if cast_state_to_symbol?(state)
+
+      cache_for_finder[state] || 0
+    end
+
+    private
+
+    def cache_for_finder
+      @cache[@finder]
+    end
+
+    def cast_state_to_symbol?(state)
+      state.is_a?(String) && STATES.include?(state)
+    end
+
+    def initialize_cache
+      Hash.new { |hash, finder| hash[finder] = finder.count_by_state }
+    end
+  end
+end

spec/controllers/concerns/issuable_collections_spec.rb

+require 'spec_helper'
+
+describe IssuableCollections do
+  let(:user) { create(:user) }
+
+  let(:controller) do
+    klass = Class.new do
+      def self.helper_method(name); end
+
+      include IssuableCollections
+    end
+
+    controller = klass.new
+
+    allow(controller).to receive(:params).and_return(state: 'opened')
+
+    controller
+  end
+
+  describe '#redirect_out_of_range' do
+    before do
+      allow(controller).to receive(:url_for)
+    end
+
+    it 'returns true and redirects if the offset is out of range' do
+      relation = double(:relation, current_page: 10)
+
+      expect(controller).to receive(:redirect_to)
+      expect(controller.send(:redirect_out_of_range, relation, 2)).to eq(true)
+    end
+
+    it 'returns false if the offset is not out of range' do
+      relation = double(:relation, current_page: 1)
+
+      expect(controller).not_to receive(:redirect_to)
+      expect(controller.send(:redirect_out_of_range, relation, 2)).to eq(false)
+    end
+  end
+
+  describe '#issues_page_count' do
+    it 'returns the number of issue pages' do
+      project = create(:project, :public)
+
+      create(:issue, project: project)
+
+      finder = IssuesFinder.new(user)
+      issues = finder.execute
+
+      allow(controller).to receive(:issues_finder)
+        .and_return(finder)
+
+      expect(controller.send(:issues_page_count, issues)).to eq(1)
+    end
+  end
+
+  describe '#merge_requests_page_count' do
+    it 'returns the number of merge request pages' do
+      project = create(:project, :public)
+
+      create(:merge_request, source_project: project, target_project: project)
+
+      finder = MergeRequestsFinder.new(user)
+      merge_requests = finder.execute
+
+      allow(controller).to receive(:merge_requests_finder)
+        .and_return(finder)
+
+      pages = controller.send(:merge_requests_page_count, merge_requests)
+
+      expect(pages).to eq(1)
+    end
+  end
+
+  describe '#page_count_for_relation' do
+    it 'returns the number of pages' do
+      relation = double(:relation, limit_value: 20)
+      pages = controller.send(:page_count_for_relation, relation, 28)
+
+      expect(pages).to eq(2)
+    end
+  end
+end

spec/finders/issues_finder_spec.rb

@@ -15,8 +15,8 @@ describe IssuesFinder do
   set(:award_emoji3) { create(:award_emoji, name: 'thumbsdown', user: user, awardable: issue3) }
 
   describe '#execute' do
-    set(:closed_issue) { create(:issue, author: user2, assignees: [user2], project: project2, state: 'closed') }
-    set(:label_link) { create(:label_link, label: label, target: issue2) }
+    let!(:closed_issue) { create(:issue, author: user2, assignees: [user2], project: project2, state: 'closed') }
+    let!(:label_link) { create(:label_link, label: label, target: issue2) }
     let(:search_user) { user }
     let(:params) { {} }
     let(:issues) { described_class.new(search_user, params.reverse_merge(scope: scope, state: 'opened')).execute }
@@ -347,6 +347,20 @@ describe IssuesFinder do
     end
   end
 
+  describe '#row_count', :request_store do
+    it 'returns the number of rows for the default state' do
+      finder = described_class.new(user)
+
+      expect(finder.row_count).to eq(3)
+    end
+
+    it 'returns the number of rows for a given state' do
+      finder = described_class.new(user, state: 'closed')
+
+      expect(finder.row_count).to be_zero
+    end
+  end
+
   describe '#with_confidentiality_access_check' do
     let(:guest) { create(:user) }
     set(:authorized_user) { create(:user) }

spec/finders/merge_requests_finder_spec.rb

@@ -108,4 +108,18 @@ describe MergeRequestsFinder do
       end
     end
   end
+
+  describe '#row_count', :request_store do
+    it 'returns the number of rows for the default state' do
+      finder = described_class.new(user)
+
+      expect(finder.row_count).to eq(3)
+    end
+
+    it 'returns the number of rows for a given state' do
+      finder = described_class.new(user, state: 'closed')
+
+      expect(finder.row_count).to eq(1)
+    end
+  end
 end

spec/lib/gitlab/issuables_count_for_state_spec.rb

+require 'spec_helper'
+
+describe Gitlab::IssuablesCountForState do
+  let(:finder) do
+    double(:finder, count_by_state: { opened: 2, closed: 1 })
+  end
+
+  let(:counter) { described_class.new(finder) }
+
+  describe '#for_state_or_opened' do
+    it 'returns the number of issuables for the given state' do
+      expect(counter.for_state_or_opened(:closed)).to eq(1)
+    end
+
+    it 'returns the number of open issuables when no state is given' do
+      expect(counter.for_state_or_opened).to eq(2)
+    end
+
+    it 'returns the number of open issuables when a nil value is given' do
+      expect(counter.for_state_or_opened(nil)).to eq(2)
+    end
+  end
+
+  describe '#[]' do
+    it 'returns the number of issuables for the given state' do
+      expect(counter[:closed]).to eq(1)
+    end
+
+    it 'casts valid states from Strings to Symbols' do
+      expect(counter['closed']).to eq(1)
+    end
+
+    it 'returns 0 when using an invalid state name as a String' do
+      expect(counter['kittens']).to be_zero
+    end
+  end
+end