diff options
23 files changed, 262 insertions, 162 deletions
diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 8ea77b994de..88044cf7557 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/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) diff --git a/app/controllers/concerns/paginated_collection.rb b/app/controllers/concerns/paginated_collection.rb new file mode 100644 index 00000000000..be84215a9e2 --- /dev/null +++ b/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 diff --git a/app/controllers/dashboard/snippets_controller.rb b/app/controllers/dashboard/snippets_controller.rb index 161c22046f9..6feade3df03 100644 --- a/app/controllers/dashboard/snippets_controller.rb +++ b/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 diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index 8f6fcb362d2..940d1482611 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/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 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)) + 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 - out_of_range + def todo_params + params.permit(:action_id, :author_id, :project_id, :type, :sort, :state, :group_id) end - # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/controllers/explore/snippets_controller.rb b/app/controllers/explore/snippets_controller.rb index 76ed142c939..d4c6aae2ca8 100644 --- a/app/controllers/explore/snippets_controller.rb +++ b/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 diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index 59f948959d6..dbd11c8ddc8 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/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 diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 869655e9550..5805d068e21 100644 --- a/app/controllers/snippets_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 91e0efcf45f..e38d4073de3 100644 --- a/app/controllers/users_controller.rb +++ b/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) diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index 6a44bc7c401..b3e4df730b4 100644 --- a/app/models/concerns/noteable.rb +++ b/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 diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 00931457344..b2fca65b9e0 100644 --- a/app/models/snippet.rb +++ b/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 diff --git a/app/views/shared/snippets/_list.html.haml b/app/views/shared/snippets/_list.html.haml index 5d2152eb411..766f48fff3d 100644 --- a/app/views/shared/snippets/_list.html.haml +++ b/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") diff --git a/app/views/shared/snippets/_snippet.html.haml b/app/views/shared/snippets/_snippet.html.haml index 42af97bc6af..0ef626868a2 100644 --- a/app/views/shared/snippets/_snippet.html.haml +++ b/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) diff --git a/changelogs/unreleased/65988-optimize-snippet-listings.yml b/changelogs/unreleased/65988-optimize-snippet-listings.yml new file mode 100644 index 00000000000..186a83cf9f3 --- /dev/null +++ b/changelogs/unreleased/65988-optimize-snippet-listings.yml @@ -0,0 +1,5 @@ +--- +title: Optimize queries for snippet listings +merge_request: 32576 +author: +type: performance diff --git a/lib/gitlab/issuable_metadata.rb b/lib/gitlab/issuable_metadata.rb index be73bcd5506..6f760751b0f 100644 --- a/lib/gitlab/issuable_metadata.rb +++ b/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( diff --git a/lib/gitlab/noteable_metadata.rb b/lib/gitlab/noteable_metadata.rb new file mode 100644 index 00000000000..f3f8933b81f --- /dev/null +++ b/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 diff --git a/spec/controllers/dashboard/snippets_controller_spec.rb b/spec/controllers/dashboard/snippets_controller_spec.rb new file mode 100644 index 00000000000..2d839094d34 --- /dev/null +++ b/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 diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb index 9a3fbfaac51..c5af04f72ee 100644 --- a/spec/controllers/dashboard/todos_controller_spec.rb +++ b/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) diff --git a/spec/controllers/explore/snippets_controller_spec.rb b/spec/controllers/explore/snippets_controller_spec.rb new file mode 100644 index 00000000000..fa659c6df7f --- /dev/null +++ b/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 diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 608131dcbc8..367bd641f5d 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/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) diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb index 9b5d7317c11..b13534b9088 100644 --- a/spec/controllers/projects/snippets_controller_spec.rb +++ b/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 diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index b0092bc8994..1b3a8965342 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/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 } diff --git a/spec/lib/gitlab/noteable_metadata_spec.rb b/spec/lib/gitlab/noteable_metadata_spec.rb new file mode 100644 index 00000000000..b12a1825f04 --- /dev/null +++ b/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 diff --git a/spec/support/shared_examples/controllers/paginated_collection_shared_examples.rb b/spec/support/shared_examples/controllers/paginated_collection_shared_examples.rb new file mode 100644 index 00000000000..bd84bd1093f --- /dev/null +++ b/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 |