Fix authors N+1 in Issues::ReferencedMergeRequestsService

`#referenced_merge_requests` preloaded too many associations. Award emoji, for
instance, are completely unnecessary here.

`#closed_by_merge_requests` had the opposite problem: `#all_references` needs
each item's author, and these weren't preloaded.

2 files changed, 37 insertions, 15 deletions

diff --git a/app/services/issues/referenced_merge_requests_service.rb b/app/services/issues/referenced_merge_requests_service.rb
index 9cce89636f5..40375a0c861 100644
--- a/app/services/issues/referenced_merge_requests_service.rb
+++ b/app/services/issues/referenced_merge_requests_service.rb
@@ -10,13 +10,7 @@ module Issues
     end
 
     def referenced_merge_requests(issue)
-      ext = issue.all_references(current_user)
-
-      issue.notes_with_associations.each do |object|
-        object.all_references(current_user, extractor: ext)
-      end
-
-      merge_requests = ext.merge_requests.sort_by(&:iid)
+      merge_requests = extract_merge_requests(issue, issue.notes)
 
       cross_project_filter = -> (merge_requests) do
         merge_requests.select { |mr| mr.target_project == project }
@@ -34,13 +28,7 @@ module Issues
     def closed_by_merge_requests(issue)
       return [] unless issue.open?
 
-      ext = issue.all_references(current_user)
-
-      issue.notes.system.each do |note|
-        note.all_references(current_user, extractor: ext)
-      end
-
-      merge_requests = ext.merge_requests.select(&:open?)
+      merge_requests = extract_merge_requests(issue, issue.notes.system).select(&:open?)
 
       return [] if merge_requests.empty?
 
@@ -50,6 +38,16 @@ module Issues
 
     private
 
+    def extract_merge_requests(issue, notes)
+      ext = issue.all_references(current_user)
+
+      notes.includes(:author).each do |note|
+        note.all_references(current_user, extractor: ext)
+      end
+
+      ext.merge_requests
+    end
+
     def sort_by_iid(merge_requests)
       Gitlab::IssuableSorter.sort(project, merge_requests) { |mr| mr.iid.to_s }
     end
diff --git a/spec/services/issues/referenced_merge_requests_service_spec.rb b/spec/services/issues/referenced_merge_requests_service_spec.rb
index a10893e502f..0afc8c300f9 100644
--- a/spec/services/issues/referenced_merge_requests_service_spec.rb
+++ b/spec/services/issues/referenced_merge_requests_service_spec.rb
@@ -5,7 +5,7 @@ require 'spec_helper.rb'
 describe Issues::ReferencedMergeRequestsService do
   def create_referencing_mr(attributes = {})
     create(:merge_request, attributes).tap do |merge_request|
-      create(:note, :system, project: project, noteable: issue, note: merge_request.to_reference(full: true))
+      create(:note, :system, project: project, noteable: issue, author: user, note: merge_request.to_reference(full: true))
     end
   end
 
@@ -54,6 +54,18 @@ describe Issues::ReferencedMergeRequestsService do
       expect(service.referenced_merge_requests(issue)).not_to include(closing_mr_other_project)
       expect(service.referenced_merge_requests(issue)).not_to include(referencing_mr_other_project)
     end
+
+    context 'performance' do
+      it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do
+        service.referenced_merge_requests(issue) # warm cache
+        control_count = ActiveRecord::QueryRecorder.new { service.referenced_merge_requests(issue) }.count
+
+        create(:note, project: project, noteable: issue, author: create(:user))
+        service.referenced_merge_requests(issue) # warm cache
+
+        expect { service.referenced_merge_requests(issue) }.not_to exceed_query_limit(control_count)
+      end
+    end
   end
 
   describe '#closed_by_merge_requests' do
@@ -68,5 +80,17 @@ describe Issues::ReferencedMergeRequestsService do
     it 'returns an empty array when the current issue is closed already' do
       expect(service.closed_by_merge_requests(closed_issue)).to eq([])
     end
+
+    context 'performance' do
+      it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do
+        service.closed_by_merge_requests(issue) # warm cache
+        control_count = ActiveRecord::QueryRecorder.new { service.closed_by_merge_requests(issue) }.count
+
+        create(:note, :system, project: project, noteable: issue, author: create(:user))
+        service.closed_by_merge_requests(issue) # warm cache
+
+        expect { service.closed_by_merge_requests(issue) }.not_to exceed_query_limit(control_count)
+      end
+    end
   end
 end