From 60755fbc406bd25ab526339899f97a2b27aeb272 Mon Sep 17 00:00:00 2001
From: Markus Koller <mkoller@gitlab.com>
Date: Mon, 2 Sep 2019 13:12:20 +0200
Subject: Optimize queries for snippet listings

- Avoid N+1 queries for authors and comment counts
- Avoid an additional snippet existence query
---
 app/controllers/dashboard/snippets_controller.rb   | 14 +++++----
 app/controllers/explore/snippets_controller.rb     | 10 +++++--
 app/controllers/projects/snippets_controller.rb    | 16 ++++++-----
 app/controllers/snippets_controller.rb             |  7 ++++-
 app/controllers/users_controller.rb                | 12 ++++----
 app/models/concerns/noteable.rb                    |  4 +++
 app/models/snippet.rb                              |  1 +
 app/views/shared/snippets/_list.html.haml          |  7 ++---
 app/views/shared/snippets/_snippet.html.haml       |  6 ++--
 .../unreleased/65988-optimize-snippet-listings.yml |  5 ++++
 lib/gitlab/issuable_metadata.rb                    |  4 +--
 lib/gitlab/noteable_metadata.rb                    | 33 ++++++++++++++++++++++
 spec/lib/gitlab/noteable_metadata_spec.rb          | 29 +++++++++++++++++++
 13 files changed, 118 insertions(+), 30 deletions(-)
 create mode 100644 changelogs/unreleased/65988-optimize-snippet-listings.yml
 create mode 100644 lib/gitlab/noteable_metadata.rb
 create mode 100644 spec/lib/gitlab/noteable_metadata_spec.rb

diff --git a/app/controllers/dashboard/snippets_controller.rb b/app/controllers/dashboard/snippets_controller.rb
index 161c22046f9..382f54c6c00 100644
--- a/app/controllers/dashboard/snippets_controller.rb
+++ b/app/controllers/dashboard/snippets_controller.rb
@@ -1,14 +1,16 @@
 # 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
diff --git a/app/controllers/explore/snippets_controller.rb b/app/controllers/explore/snippets_controller.rb
index 76ed142c939..6192b594593 100644
--- a/app/controllers/explore/snippets_controller.rb
+++ b/app/controllers/explore/snippets_controller.rb
@@ -1,8 +1,14 @@
 # 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
diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb
index 59f948959d6..c84e9fbc2d7 100644
--- a/app/controllers/projects/snippets_controller.rb
+++ b/app/controllers/projects/snippets_controller.rb
@@ -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
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb
index 869655e9550..c1e05b72b37 100644
--- a/app/controllers/snippets_controller.rb
+++ b/app/controllers/snippets_controller.rb
@@ -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 @@ 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
+
+      @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/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
-- 
cgit v1.2.1