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

Eliminate N+1 queries in path lock checks during a push

Closes #6465

See merge request gitlab-org/gitlab-ee!6159

app/models/project.rb

@@ -25,6 +25,7 @@ class Project < ActiveRecord::Base
   include FastDestroyAll::Helpers
   include WithUploads
   include BatchDestroyDependentAssociations
+  extend Gitlab::Cache::RequestCache
 
   # EE specific modules
   prepend EE::Project
@@ -2027,6 +2028,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

ee/app/models/ee/project.rb

@@ -6,6 +6,7 @@ module EE
   module Project
     extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
+    extend ::Gitlab::Cache::RequestCache
     include ::Gitlab::Utils::StrongMemoize
 
     prepended do
@@ -486,6 +487,11 @@ module EE
       end
     end
 
+    def any_path_locks?
+      path_locks.any?
+    end
+    request_cache(:any_path_locks?) { self.id }
+
     private
 
     def set_override_pull_mirror_available

ee/changelogs/unreleased/sh-optimize-locks-check.yml

+---
+title: Eliminate N+1 queries in path lock checks during a push
+merge_request:
+author:
+type: performance

ee/lib/ee/gitlab/checks/change_access.rb

@@ -190,7 +190,7 @@ module EE
         def validate_path_locks?
           strong_memoize(:validate_path_locks) do
             project.feature_available?(:file_locks) &&
-              project.path_locks.any? && newrev && oldrev &&
+              newrev && oldrev && project.any_path_locks? &&
               project.default_branch == branch_name # locks protect default branch only
           end
         end

ee/spec/models/project_spec.rb

@@ -1390,4 +1390,34 @@ describe Project do
       it { is_expected.to be_empty }
     end
   end
+
+  describe '#find_path_lock' do
+    let(:project) { create :project }
+    let(:path_lock) { create :path_lock, project: project }
+    let(:path) { path_lock.path }
+
+    it 'returns path_lock' do
+      expect(project.find_path_lock(path)).to eq(path_lock)
+    end
+
+    it 'returns nil' do
+      expect(project.find_path_lock('app/controllers')).to be_falsey
+    end
+  end
+
+  describe '#any_path_locks?', :request_store do
+    let(:project) { create :project }
+
+    it 'returns false when there are no path locks' do
+      expect(project.any_path_locks?).to be_falsey
+    end
+
+    it 'returns a cached true when there are path locks' do
+      create(:path_lock, project: project)
+
+      expect(project.path_locks).to receive(:any?).once.and_call_original
+
+      2.times { expect(project.any_path_locks?).to be_truthy }
+    end
+  end
 end

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
 

spec/lib/gitlab/git_access_spec.rb

@@ -1055,6 +1055,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
 

spec/models/project_spec.rb

@@ -2292,20 +2292,6 @@ describe Project do
     end
   end
 
-  describe '#find_path_lock' do
-    let(:project) { create :project }
-    let(:path_lock) { create :path_lock, project: project }
-    let(:path) { path_lock.path }
-
-    it 'returns path_lock' do
-      expect(project.find_path_lock(path)).to eq(path_lock)
-    end
-
-    it 'returns nil' do
-      expect(project.find_path_lock('app/controllers')).to be_falsey
-    end
-  end
-
   describe '#change_repository_storage' do
     let(:project) { create(:project, :repository) }
     let(:read_only_project) { create(:project, :repository, repository_read_only: true) }
@@ -2692,6 +2678,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) }