Merge branch 'reduce-query-count-for-mergerequestscontroller-show' into 'master'

Fix N+1 in `MergeRequest#merge_request_diff_for`

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

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