Fix n+1 issue by not reloading fully loaded blobs37430-projects-comparecontroller-show-is-calling-gitaly-n-1-times-per-request

3 files changed, 45 insertions, 14 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..f6aea1e6c79 100644
--- a/spec/lib/gitlab/git/blob_spec.rb
+++ b/spec/lib/gitlab/git/blob_spec.rb
@@ -140,12 +140,12 @@ describe Gitlab::Git::Blob, seed_helper: true do
     it { expect(bad_blob).to be_nil }
 
     context 'large file' do
-      it 'limits the size of a large file' do
-        blob_size = Gitlab::Git::Blob::MAX_DATA_DISPLAY_SIZE + 1
-        buffer = Array.new(blob_size, 0)
-        rugged_blob = Rugged::Blob.from_buffer(repository.rugged, buffer.join(''))
-        blob = Gitlab::Git::Blob.raw(repository, rugged_blob)
+      let(:blob_size) { Gitlab::Git::Blob::MAX_DATA_DISPLAY_SIZE + 1 }
+      let(:buffer) { Array.new(blob_size, 0) }
+      let(:rugged_blob) { Rugged::Blob.from_buffer(repository.rugged, buffer.join('')) }
+      let(:blob) { Gitlab::Git::Blob.raw(repository, rugged_blob) }
 
+      it 'limits the size of a large file' do
         expect(blob.size).to eq(blob_size)
         expect(blob.loaded_size).to eq(Gitlab::Git::Blob::MAX_DATA_DISPLAY_SIZE)
         expect(blob.data.length).to eq(Gitlab::Git::Blob::MAX_DATA_DISPLAY_SIZE)
@@ -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