Merge branch 'mk/fix-n-plus-1-queries-in-uploads-check-rake-task' into 'master'11-0-stable-patch-2

Fix N+1 queries in uploads check rake task

Closes #47924

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

3 files changed, 39 insertions, 9 deletions

diff --git a/doc/development/query_recorder.md b/doc/development/query_recorder.md
index 61e5e1afede..2167ed57428 100644
--- a/doc/development/query_recorder.md
+++ b/doc/development/query_recorder.md
@@ -28,6 +28,7 @@ By default, QueryRecorder will ignore cached queries in the count. However, it m
 all queries to avoid introducing an N+1 query that may be masked by the statement cache. To do this,
 pass the `skip_cached` variable to `QueryRecorder` and use the `exceed_all_query_limit` matcher:
 
+```
 it "avoids N+1 database queries" do
   control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { visit_some_page }.count
   create_list(:issue, 5)
diff --git a/lib/gitlab/verify/uploads.rb b/lib/gitlab/verify/uploads.rb
index 01f09ab8df7..73fc43cb590 100644
--- a/lib/gitlab/verify/uploads.rb
+++ b/lib/gitlab/verify/uploads.rb
@@ -12,7 +12,7 @@ module Gitlab
       private
 
       def all_relation
-        Upload.all
+        Upload.all.preload(:model)
       end
 
       def local?(upload)
diff --git a/spec/lib/gitlab/verify/uploads_spec.rb b/spec/lib/gitlab/verify/uploads_spec.rb
index 296866d3319..38c30fab1ba 100644
--- a/spec/lib/gitlab/verify/uploads_spec.rb
+++ b/spec/lib/gitlab/verify/uploads_spec.rb
@@ -47,20 +47,49 @@ describe Gitlab::Verify::Uploads do
       before do
         stub_uploads_object_storage(AvatarUploader)
         upload.update!(store: ObjectStorage::Store::REMOTE)
-        expect(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file)
       end
 
-      it 'passes uploads in object storage that exist' do
-        expect(file).to receive(:exists?).and_return(true)
+      describe 'returned hash object' do
+        before do
+          expect(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file)
+        end
 
-        expect(failures).to eq({})
+        it 'passes uploads in object storage that exist' do
+          expect(file).to receive(:exists?).and_return(true)
+
+          expect(failures).to eq({})
+        end
+
+        it 'fails uploads in object storage that do not exist' do
+          expect(file).to receive(:exists?).and_return(false)
+
+          expect(failures.keys).to contain_exactly(upload)
+          expect(failure).to include('Remote object does not exist')
+        end
       end
 
-      it 'fails uploads in object storage that do not exist' do
-        expect(file).to receive(:exists?).and_return(false)
+      describe 'performance' do
+        before do
+          allow(file).to receive(:exists?)
+          allow(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file)
+        end
+
+        it "avoids N+1 queries" do
+          control_count = ActiveRecord::QueryRecorder.new { perform_task }
+
+          # Create additional uploads in object storage
+          projects = create_list(:project, 3, :with_avatar)
+          uploads = projects.flat_map(&:uploads)
+          uploads.each do |upload|
+            upload.update!(store: ObjectStorage::Store::REMOTE)
+          end
+
+          expect { perform_task }.not_to exceed_query_limit(control_count)
+        end
 
-        expect(failures.keys).to contain_exactly(upload)
-        expect(failure).to include('Remote object does not exist')
+        def perform_task
+          described_class.new(batch_size: 100).run_batches { }
+        end
       end
     end
   end