Fix n+1 issue by not reloading fully loaded blobs

3 files changed, 40 insertions, 9 deletions

diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index 3cb4eb23981..2b0c2ca97c0 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -17,10 +17,8 @@ class Projects::CompareController < Projects::ApplicationController
 
   def show
     apply_diff_view_cookie!
-    # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37430
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
-      render
-    end
+
+    render
   end
 
   def diff_for_path
diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb
index b2fca2c16de..eabcf46cf58 100644
--- a/lib/gitlab/git/blob.rb
+++ b/lib/gitlab/git/blob.rb
@@ -238,9 +238,9 @@ module Gitlab
           self.__send__("#{key}=", options[key.to_sym]) # rubocop:disable GitlabSecurity/PublicSend
         end
 
-        @loaded_all_data = false
         # Retain the actual size before it is encoded
         @loaded_size = @data.bytesize if @data
+        @loaded_all_data = @loaded_size == size
       end
 
       def binary?
@@ -255,10 +255,15 @@ module Gitlab
       # memory as a Ruby string.
       def load_all_data!(repository)
         return if @data == '' # don't mess with submodule blobs
-        return @data if @loaded_all_data
 
-        Gitlab::GitalyClient.migrate(:git_blob_load_all_data) do |is_enabled|
-          @data = begin
+        # Even if we return early, recalculate wether this blob is binary in
+        # case a blob was initialized as text but the full data isn't
+        @binary = nil
+
+        return if @loaded_all_data
+
+        @data = Gitlab::GitalyClient.migrate(:git_blob_load_all_data) do |is_enabled|
+          begin
             if is_enabled
               repository.gitaly_blob_client.get_blob(oid: id, limit: -1).data
             else
@@ -269,7 +274,6 @@ module Gitlab
 
         @loaded_all_data = true
         @loaded_size = @data.bytesize
-        @binary = nil
       end
 
       def name
diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb
index a6341cd509b..67d898e787e 100644
--- a/spec/lib/gitlab/git/blob_spec.rb
+++ b/spec/lib/gitlab/git/blob_spec.rb
@@ -500,4 +500,33 @@ describe Gitlab::Git::Blob, seed_helper: true do
       end
     end
   end
+
+  describe '#load_all_data!' do
+    let(:full_data) { 'abcd' }
+    let(:blob) { Gitlab::Git::Blob.new(name: 'test', size: 4, data: 'abc') }
+
+    subject { blob.load_all_data!(repository) }
+
+    it 'loads missing data' do
+      expect(Gitlab::GitalyClient).to receive(:migrate)
+        .with(:git_blob_load_all_data).and_return(full_data)
+
+      subject
+
+      expect(blob.data).to eq(full_data)
+    end
+
+    context 'with a fully loaded blob' do
+      let(:blob) { Gitlab::Git::Blob.new(name: 'test', size: 4, data: full_data) }
+
+      it "doesn't perform any loading" do
+        expect(Gitlab::GitalyClient).not_to receive(:migrate)
+          .with(:git_blob_load_all_data)
+
+        subject
+
+        expect(blob.data).to eq(full_data)
+      end
+    end
+  end
 end