diff options
author | Stan Hu <stanhu@gmail.com> | 2018-06-20 22:31:48 +0000 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-06-20 22:31:48 +0000 |
commit | c0616ba8aaf9cbb46cc7476e5d4b08d0e98863ca (patch) | |
tree | 877f2cd57437288b150951aef8105d51d78d4238 | |
parent | e40aa397700f997b320d9e6299ab3b15b75c77a8 (diff) | |
parent | ba56b3447098890d313f943781b5413c566e14cd (diff) | |
download | gitlab-ce-c0616ba8aaf9cbb46cc7476e5d4b08d0e98863ca.tar.gz |
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
-rw-r--r-- | doc/development/query_recorder.md | 1 | ||||
-rw-r--r-- | lib/gitlab/verify/uploads.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/verify/uploads_spec.rb | 45 |
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 |