diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index a34a82b7ba6..23909bd2d39 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/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 diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 0d4266f0899..dc9e6f71152 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index e3fa3736808..5095d7fd445 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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] } diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index c8dd2275730..9848497f258 100644 --- a/app/finders/issuable_finder.rb +++ b/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 diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 681615dbf3e..717abf2082d 100644 --- a/app/helpers/issuables_helper.rb +++ b/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) diff --git a/app/views/projects/issues/_issues.html.haml b/app/views/projects/issues/_issues.html.haml index 34d5a3e1831..6fb5aa45166 100644 --- a/app/views/projects/issues/_issues.html.haml +++ b/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 diff --git a/app/views/projects/merge_requests/_merge_requests.html.haml b/app/views/projects/merge_requests/_merge_requests.html.haml index 4e97f74dd6a..bd6f1c05949 100644 --- a/app/views/projects/merge_requests/_merge_requests.html.haml +++ b/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 diff --git a/changelogs/unreleased/mr-index-page-performance.yml b/changelogs/unreleased/mr-index-page-performance.yml new file mode 100644 index 00000000000..df5f44c04fa --- /dev/null +++ b/changelogs/unreleased/mr-index-page-performance.yml @@ -0,0 +1,5 @@ +--- +title: Re-use issue/MR counts for the pagination system +merge_request: +author: +type: other diff --git a/lib/gitlab/issuables_count_for_state.rb b/lib/gitlab/issuables_count_for_state.rb new file mode 100644 index 00000000000..505810964bc --- /dev/null +++ b/lib/gitlab/issuables_count_for_state.rb @@ -0,0 +1,50 @@ +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 diff --git a/spec/controllers/concerns/issuable_collections_spec.rb b/spec/controllers/concerns/issuable_collections_spec.rb new file mode 100644 index 00000000000..c9687af4dd2 --- /dev/null +++ b/spec/controllers/concerns/issuable_collections_spec.rb @@ -0,0 +1,82 @@ +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 diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 0e80df94e18..47b173dea0a 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/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) } diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index b54155a6704..95f445e7905 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/issuables_count_for_state_spec.rb b/spec/lib/gitlab/issuables_count_for_state_spec.rb new file mode 100644 index 00000000000..c262fdfcb61 --- /dev/null +++ b/spec/lib/gitlab/issuables_count_for_state_spec.rb @@ -0,0 +1,37 @@ +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