Commit ef2252c3 authored by Markus Koller's avatar Markus Koller
Browse files

Optimize queries for snippet listings

- Avoid N+1 queries for authors and comment counts
- Avoid an additional snippet existence query
parent 3e1a2a49b9ed
# frozen_string_literal: true
class Dashboard::SnippetsController < Dashboard::ApplicationController
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
@noteable_meta_data = noteable_meta_data(@snippets, 'Snippet')
end
end
# frozen_string_literal: true
class Explore::SnippetsController < Explore::ApplicationController
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
@noteable_meta_data = noteable_meta_data(@snippets, 'Snippet')
end
end
......@@ -6,6 +6,7 @@ class Projects::SnippetsController < Projects::ApplicationController
include SpammableActions
include SnippetsActions
include RendersBlob
include Gitlab::NoteableMetadata
skip_before_action :verify_authenticity_token,
if: -> { action_name == 'show' && js_request? }
......@@ -28,15 +29,16 @@ 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])
@snippets = SnippetsFinder.new(current_user, project: @project, scope: params[:scope])
.execute
.page(params[:page])
.inc_author
if @snippets.out_of_range? && @snippets.total_pages != 0
redirect_to project_snippets_path(@project, page: @snippets.total_pages)
return redirect_to project_snippets_path(@project, page: @snippets.total_pages)
end
@noteable_meta_data = noteable_meta_data(@snippets, 'Snippet')
end
def new
......
......@@ -7,6 +7,7 @@ class SnippetsController < ApplicationController
include SnippetsActions
include RendersBlob
include PreviewMarkdown
include Gitlab::NoteableMetadata
skip_before_action :verify_authenticity_token,
if: -> { action_name == 'show' && js_request? }
......@@ -32,7 +33,11 @@ def index
@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
@noteable_meta_data = noteable_meta_data(@snippets, 'Snippet')
render 'index'
else
......
......@@ -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 @@ def load_groups
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)
......
......@@ -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
......
......@@ -55,6 +55,7 @@ def content_html_invalidated?
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
......
- 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")
- 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)
......
---
title: Optimize queries for snippet listings
merge_request: 32576
author:
type: performance
......@@ -19,7 +19,7 @@ def issuable_meta_data(issuable_collection, collection_type, user = nil)
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 @@ def issuable_meta_data(issuable_collection, collection_type, user = nil)
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(
......
# 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
# 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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment