Fix N+1 in `MergeRequest#merge_request_diff_for`reduce-query-count-for-mergerequestscontroller-show

Previously, this would issue a query for each unique `diff_refs_or_sha`
passed. This was because we didn't want to load other MR diffs into memory, as
they had some very large columns.

Now they are actually very small, and it's more efficient to just load them all
at once and do the finding in Ruby.

3 files changed, 33 insertions, 10 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 7e6d89ec9c7..91d8be5559b 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -536,18 +536,25 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff(true)
   end
 
+  def viewable_diffs
+    @viewable_diffs ||= merge_request_diffs.viewable.to_a
+  end
+
   def merge_request_diff_for(diff_refs_or_sha)
-    @merge_request_diffs_by_diff_refs_or_sha ||= Hash.new do |h, diff_refs_or_sha|
-      diffs = merge_request_diffs.viewable
-      h[diff_refs_or_sha] =
-        if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs)
-          diffs.find_by_diff_refs(diff_refs_or_sha)
-        else
-          diffs.find_by(head_commit_sha: diff_refs_or_sha)
-        end
-    end
+    matcher =
+      if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs)
+        {
+          'start_commit_sha' => diff_refs_or_sha.start_sha,
+          'head_commit_sha' => diff_refs_or_sha.head_sha,
+          'base_commit_sha' => diff_refs_or_sha.base_sha
+        }
+      else
+        { 'head_commit_sha' => diff_refs_or_sha }
+      end
 
-    @merge_request_diffs_by_diff_refs_or_sha[diff_refs_or_sha]
+    viewable_diffs.find do |diff|
+      diff.attributes.slice(*matcher.keys) == matcher
+    end
   end
 
   def version_params_for(diff_refs)
diff --git a/changelogs/unreleased/reduce-query-count-for-mergerequestscontroller-show.yml b/changelogs/unreleased/reduce-query-count-for-mergerequestscontroller-show.yml
new file mode 100644
index 00000000000..1f793fe5e7c
--- /dev/null
+++ b/changelogs/unreleased/reduce-query-count-for-mergerequestscontroller-show.yml
@@ -0,0 +1,5 @@
+---
+title: Reduce number of queries when viewing a merge request
+merge_request:
+author:
+type: performance
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index ff5a6f63010..f73f44ca0ad 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1961,6 +1961,17 @@ describe MergeRequest do
         expect(subject.merge_request_diff_for(merge_request_diff3.head_commit_sha)).to eq(merge_request_diff3)
       end
     end
+
+    it 'runs a single query on the initial call, and none afterwards' do
+      expect { subject.merge_request_diff_for(merge_request_diff1.diff_refs) }
+        .not_to exceed_query_limit(1)
+
+      expect { subject.merge_request_diff_for(merge_request_diff2.diff_refs) }
+        .not_to exceed_query_limit(0)
+
+      expect { subject.merge_request_diff_for(merge_request_diff3.head_commit_sha) }
+        .not_to exceed_query_limit(0)
+    end
   end
 
   describe '#version_params_for' do