diff options
author | Thong Kuah <tkuah@gitlab.com> | 2019-09-11 07:48:21 +0300 |
---|---|---|
committer | Thong Kuah <tkuah@gitlab.com> | 2019-09-11 07:48:21 +0300 |
commit | 28292d516abb33aeaf7e5bfaf94679d1317bd284 (patch) | |
tree | 0acb8ae2d6904b8d779230df7a170f2cd0c2b6bf /app | |
parent | bb7bbcf7b667197e71f696cc17d8c08677629e8d (diff) | |
parent | f1926b321deb8b922dead991fb4d8bea42699f9f (diff) |
Merge branch '65988-optimize-snippet-listings' into 'master'
Optimize queries for snippet listings
See merge request gitlab-org/gitlab-ce!32576
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/concerns/issuable_collections.rb | 28 | ||||
-rw-r--r-- | app/controllers/concerns/paginated_collection.rb | 19 | ||||
-rw-r--r-- | app/controllers/dashboard/snippets_controller.rb | 17 | ||||
-rw-r--r-- | app/controllers/dashboard/todos_controller.rb | 32 | ||||
-rw-r--r-- | app/controllers/explore/snippets_controller.rb | 13 | ||||
-rw-r--r-- | app/controllers/projects/snippets_controller.rb | 19 | ||||
-rw-r--r-- | app/controllers/snippets_controller.rb | 10 | ||||
-rw-r--r-- | app/controllers/users_controller.rb | 12 | ||||
-rw-r--r-- | app/models/concerns/noteable.rb | 4 | ||||
-rw-r--r-- | app/models/snippet.rb | 1 | ||||
-rw-r--r-- | app/views/shared/snippets/_list.html.haml | 7 | ||||
-rw-r--r-- | app/views/shared/snippets/_snippet.html.haml | 6 |
12 files changed, 97 insertions, 71 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) |