Merge branch 'sh-optimize-locks-check-ce' into 'master'

Eliminate N+1 queries in LFS file lock checks during a push

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

5 files changed, 44 insertions, 1 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index e5fa1c4db7b..0d777515536 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -25,6 +25,7 @@ class Project < ActiveRecord::Base
   include FastDestroyAll::Helpers
   include WithUploads
   include BatchDestroyDependentAssociations
+  extend Gitlab::Cache::RequestCache
 
   extend Gitlab::ConfigHelper
 
@@ -2013,6 +2014,11 @@ class Project < ActiveRecord::Base
     @gitlab_deploy_token ||= deploy_tokens.gitlab_deploy_token
   end
 
+  def any_lfs_file_locks?
+    lfs_file_locks.any?
+  end
+  request_cache(:any_lfs_file_locks?) { self.id }
+
   private
 
   def storage
diff --git a/changelogs/unreleased/sh-optimize-locks-check-ce.yml b/changelogs/unreleased/sh-optimize-locks-check-ce.yml
new file mode 100644
index 00000000000..933ec9b79bf
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-locks-check-ce.yml
@@ -0,0 +1,5 @@
+---
+title: Eliminate N+1 queries in LFS file locks checks during a push
+merge_request:
+author:
+type: performance
diff --git a/lib/gitlab/checks/commit_check.rb b/lib/gitlab/checks/commit_check.rb
index 43a52b493bb..22310e313ac 100644
--- a/lib/gitlab/checks/commit_check.rb
+++ b/lib/gitlab/checks/commit_check.rb
@@ -37,7 +37,7 @@ module Gitlab
 
       def validate_lfs_file_locks?
         strong_memoize(:validate_lfs_file_locks) do
-          project.lfs_enabled? && project.lfs_file_locks.any? && newrev && oldrev
+          project.lfs_enabled? && newrev && oldrev && project.any_lfs_file_locks?
         end
       end
 
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 0d5f6a0b576..ff32025253a 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -934,6 +934,22 @@ describe Gitlab::GitAccess do
         expect(project.repository).to receive(:clean_stale_repository_files).and_call_original
         expect { push_access_check }.not_to raise_error
       end
+
+      it 'avoids N+1 queries', :request_store do
+        # Run this once to establish a baseline. Cached queries should get
+        # cached, so that when we introduce another change we shouldn't see
+        # additional queries.
+        access.check('git-receive-pack', changes)
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          access.check('git-receive-pack', changes)
+        end
+
+        changes = ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature']
+
+        # There is still an N+1 query with protected branches
+        expect { access.check('git-receive-pack', changes) }.not_to exceed_query_limit(control_count).with_threshold(1)
+      end
     end
   end
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index bc9cce6b0c3..a2f8fac2f38 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2339,6 +2339,22 @@ describe Project do
     end
   end
 
+  describe '#any_lfs_file_locks?', :request_store do
+    set(:project) { create(:project) }
+
+    it 'returns false when there are no LFS file locks' do
+      expect(project.any_lfs_file_locks?).to be_falsey
+    end
+
+    it 'returns a cached true when there are LFS file locks' do
+      create(:lfs_file_lock, project: project)
+
+      expect(project.lfs_file_locks).to receive(:any?).once.and_call_original
+
+      2.times { expect(project.any_lfs_file_locks?).to be_truthy }
+    end
+  end
+
   describe '#protected_for?' do
     let(:project) { create(:project) }