Use Commit#notes and Note.for_commit_id when possible to make sure we use all the indexes available to usdm-notes-for-commit-id

11 files changed, 16 insertions, 29 deletions

diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb
index d48284a4429..28920877635 100644
--- a/app/controllers/projects/commits_controller.rb
+++ b/app/controllers/projects/commits_controller.rb
@@ -10,9 +10,6 @@ class Projects::CommitsController < Projects::ApplicationController
   before_action :set_commits
 
   def show
-    @note_counts = project.notes.where(commit_id: @commits.map(&:id))
-      .group(:commit_id).count
-
     @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened
       .find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref)
 
diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb
index 129682f64aa..764a9c7111e 100644
--- a/app/controllers/projects/merge_requests/creations_controller.rb
+++ b/app/controllers/projects/merge_requests/creations_controller.rb
@@ -110,9 +110,6 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
     @commits = prepare_commits_for_rendering(@merge_request.commits)
     @commit = @merge_request.diff_head_commit
 
-    @note_counts = Note.where(commit_id: @commits.map(&:id))
-      .group(:commit_id).count
-
     @labels = LabelsFinder.new(current_user, project_id: @project.id).execute
 
     set_pipeline_variables
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index c86acae8fe4..c068f95b55a 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -102,8 +102,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     # Get commits from repository
     # or from cache if already merged
     @commits = prepare_commits_for_rendering(@merge_request.commits)
-    @note_counts = Note.where(commit_id: @commits.map(&:id))
-      .group(:commit_id).count
 
     render json: { html: view_to_html_string('projects/merge_requests/_commits') }
   end
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index ca65e81f27a..fcbe3d2b67b 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -409,7 +409,7 @@ module Ci
     end
 
     def notes
-      Note.for_commit_id(sha)
+      project.notes.for_commit_id(sha)
     end
 
     def process!
diff --git a/app/models/external_issue.rb b/app/models/external_issue.rb
index 0bf18e529f0..9ff56f229bc 100644
--- a/app/models/external_issue.rb
+++ b/app/models/external_issue.rb
@@ -47,4 +47,8 @@ class ExternalIssue
 
     id
   end
+
+  def notes
+    Note.none
+  end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index f80601f3484..919391d2e47 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -578,7 +578,7 @@ class MergeRequest < ActiveRecord::Base
     commit_notes = Note
       .except(:order)
       .where(project_id: [source_project_id, target_project_id])
-      .where(noteable_type: 'Commit', commit_id: commit_ids)
+      .for_commit_id(commit_ids)
 
     # We're using a UNION ALL here since this results in better performance
     # compared to using OR statements. We're using UNION ALL since the queries
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 69bd19c1977..e946218824c 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -481,17 +481,7 @@ module SystemNoteService
   #
   # Returns Boolean
   def cross_reference_exists?(noteable, mentioner)
-    # Initial scope should be system notes of this noteable type
-    notes = Note.system.where(noteable_type: noteable.class)
-
-    notes =
-      if noteable.is_a?(Commit)
-        # Commits have non-integer IDs, so they're stored in `commit_id`
-        notes.where(commit_id: noteable.id)
-      else
-        notes.where(noteable_id: noteable.id)
-      end
-
+    notes = noteable.notes.system
     notes_for_mentioner(mentioner, noteable, notes).exists?
   end
 
diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml
index a16ffb433a5..a66177f20e9 100644
--- a/app/views/projects/commits/_commit.html.haml
+++ b/app/views/projects/commits/_commit.html.haml
@@ -1,11 +1,6 @@
 - ref = local_assigns.fetch(:ref)
-- if @note_counts
-  - note_count = @note_counts.fetch(commit.id, 0)
-- else
-  - notes = commit.notes
-  - note_count = notes.user.count
 
-- cache_key = [project.full_path, commit.id, current_application_settings, note_count, @path.presence, current_controller?(:commits), I18n.locale]
+- cache_key = [project.full_path, commit.id, current_application_settings, @path.presence, current_controller?(:commits), I18n.locale]
 - cache_key.push(commit.status(ref)) if commit.status(ref)
 
 = cache(cache_key, expires_in: 1.day) do
diff --git a/changelogs/unreleased/dm-notes-for-commit-id.yml b/changelogs/unreleased/dm-notes-for-commit-id.yml
new file mode 100644
index 00000000000..5b83332d82f
--- /dev/null
+++ b/changelogs/unreleased/dm-notes-for-commit-id.yml
@@ -0,0 +1,6 @@
+---
+title: Improve performance of commits list by fully using DB index when getting commit
+  note counts
+merge_request:
+author:
+type: performance
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index 2685dc27252..2bc4039b019 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -117,7 +117,7 @@ module API
         commit = user_project.commit(params[:sha])
 
         not_found! 'Commit' unless commit
-        notes = user_project.notes.where(commit_id: commit.id).order(:created_at)
+        notes = commit.notes.order(:created_at)
 
         present paginate(notes), with: Entities::CommitNote
       end
diff --git a/lib/api/v3/commits.rb b/lib/api/v3/commits.rb
index ed206a6def0..be360fbfc0c 100644
--- a/lib/api/v3/commits.rb
+++ b/lib/api/v3/commits.rb
@@ -106,7 +106,7 @@ module API
           commit = user_project.commit(params[:sha])
 
           not_found! 'Commit' unless commit
-          notes = Note.where(commit_id: commit.id).order(:created_at)
+          notes = commit.notes.order(:created_at)
 
           present paginate(notes), with: ::API::Entities::CommitNote
         end