Commit c0616ba8 authored by Stan Hu's avatar Stan Hu

Merge branch 'mk/fix-n-plus-1-queries-in-uploads-check-rake-task' into 'master'

Fix N+1 queries in uploads check rake task

Closes #47924

See merge request gitlab-org/gitlab-ce!20012
parents e40aa397 ba56b344
...@@ -28,6 +28,7 @@ By default, QueryRecorder will ignore cached queries in the count. However, it m ...@@ -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, 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: pass the `skip_cached` variable to `QueryRecorder` and use the `exceed_all_query_limit` matcher:
```
it "avoids N+1 database queries" do it "avoids N+1 database queries" do
control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { visit_some_page }.count control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { visit_some_page }.count
create_list(:issue, 5) create_list(:issue, 5)
......
...@@ -12,7 +12,7 @@ module Gitlab ...@@ -12,7 +12,7 @@ module Gitlab
private private
def all_relation def all_relation
Upload.all Upload.all.preload(:model)
end end
def local?(upload) def local?(upload)
......
...@@ -47,20 +47,49 @@ describe Gitlab::Verify::Uploads do ...@@ -47,20 +47,49 @@ describe Gitlab::Verify::Uploads do
before do before do
stub_uploads_object_storage(AvatarUploader) stub_uploads_object_storage(AvatarUploader)
upload.update!(store: ObjectStorage::Store::REMOTE) upload.update!(store: ObjectStorage::Store::REMOTE)
expect(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file)
end end
it 'passes uploads in object storage that exist' do describe 'returned hash object' do
expect(file).to receive(:exists?).and_return(true) 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 end
it 'fails uploads in object storage that do not exist' do describe 'performance' do
expect(file).to receive(:exists?).and_return(false) 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) def perform_task
expect(failure).to include('Remote object does not exist') described_class.new(batch_size: 100).run_batches { }
end
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment