Merge branch 'dm-notes-for-commit-id' into 'master'

Use Commit#notes and Note.for_commit_id when possible to make sure we use all indexes available to us

Closes #34509

See merge request gitlab-org/gitlab-ce!15253

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)
 

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

app/controllers/projects/merge_requests_controller.rb

@@ -81,8 +81,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

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!

app/models/external_issue.rb

@@ -47,4 +47,8 @@ class ExternalIssue
 
     id
   end
+
+  def notes
+    Note.none
+  end
 end

app/models/merge_request.rb

@@ -576,7 +576,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

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
 

app/views/projects/commits/_commit.html.haml

 - 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

changelogs/unreleased/dm-notes-for-commit-id.yml

+---
+title: Improve performance of commits list by fully using DB index when getting commit
+  note counts
+merge_request:
+author:
+type: performance

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

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