From b26e5546c3a523742b39a0b5b0e376367ea4c649 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Tue, 21 Aug 2018 12:56:01 +0100
Subject: [PATCH] Only load issue notes once when getting related MRs

As we always call both methods from the controller - and elsewhere we call the
more general method - and one uses all notes and the other uses system notes,
then we should just load the notes and their authors once, and filter on the
Ruby side.
---
 .../issues/referenced_merge_requests_service.rb   | 15 +++++++++++----
 .../referenced_merge_requests_service_spec.rb     |  6 ++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/app/services/issues/referenced_merge_requests_service.rb b/app/services/issues/referenced_merge_requests_service.rb
index c64ac806ac6..40d78502697 100644
--- a/app/services/issues/referenced_merge_requests_service.rb
+++ b/app/services/issues/referenced_merge_requests_service.rb
@@ -14,7 +14,7 @@ module Issues
     end
 
     def referenced_merge_requests(issue)
-      merge_requests = extract_merge_requests(issue, issue.notes)
+      merge_requests = extract_merge_requests(issue)
 
       cross_project_filter = -> (merge_requests) do
         merge_requests.select { |mr| mr.target_project == project }
@@ -32,7 +32,7 @@ module Issues
     def closed_by_merge_requests(issue)
       return [] unless issue.open?
 
-      merge_requests = extract_merge_requests(issue, issue.notes.system).select(&:open?)
+      merge_requests = extract_merge_requests(issue, filter: :system).select(&:open?)
 
       return [] if merge_requests.empty?
 
@@ -42,16 +42,23 @@ module Issues
 
     private
 
-    def extract_merge_requests(issue, notes)
+    def extract_merge_requests(issue, filter: nil)
       ext = issue.all_references(current_user)
+      notes = issue_notes(issue)
+      notes = notes.select(&filter) if filter
 
-      notes.includes(:author).each do |note|
+      notes.each do |note|
         note.all_references(current_user, extractor: ext)
       end
 
       ext.merge_requests
     end
 
+    def issue_notes(issue)
+      @issue_notes ||= {}
+      @issue_notes[issue] ||= issue.notes.includes(:author)
+    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 d57bcf55af1..61d1612829f 100644
--- a/spec/services/issues/referenced_merge_requests_service_spec.rb
+++ b/spec/services/issues/referenced_merge_requests_service_spec.rb
@@ -65,6 +65,12 @@ describe Issues::ReferencedMergeRequestsService do
         expect { service.execute(issue).each(&pipeline_routes) }
           .not_to exceed_query_limit(control_count)
       end
+
+      it 'only loads issue notes once' do
+        expect(issue).to receive(:notes).once.and_call_original
+
+        service.execute(issue)
+      end
     end
   end