summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2019-04-02 12:52:28 +0100
committerSean McGivern <sean@gitlab.com>2019-04-04 11:32:42 +0100
commitec85debaf51067cc78d54188ec1eef94342d5a8b (patch)
treefa1a8a38dd233d479b3b7f2973dd92bdd465936d
parentf87b7fe3b386962c45e83486634352da544857fb (diff)
downloadgitlab-ce-ec85debaf51067cc78d54188ec1eef94342d5a8b.tar.gz
Speed up avatar URLs with object storage
With object storage enabled, calling `#filename` on an upload does this: 1. Call the `#filename` method on the CarrierWave object. 2. Generate the URL for that object. 3. If the uploader isn't public, do so by generating an authenticated URL, including signing that request. That's all correct behaviour, but for the case where we use `#filename`, it's typically to generate a GitLab URL. That URL doesn't need to be signed because we do our own auth. Signing the URLs can be very expensive, especially in batch (say, we need to get the avatar URLs for 150 users in one request). It's all unnecessary work. If we used the `RecordsUploads` concern, we have already recorded a `path` in the database. That `path` is actually generated from CarrierWave's `#filename` at upload time, so we don't need to recompute it - we can just use it and strip off the prefix if it's available. On a sample users autocomplete URL, at least 10% of the time before this change went to signing URLs. After this change, we spend no time in URL signing, and still get the correct results.
-rw-r--r--app/uploaders/records_uploads.rb4
-rw-r--r--changelogs/unreleased/stop-signing-avatar-paths.yml5
-rw-r--r--spec/uploaders/records_uploads_spec.rb9
3 files changed, 18 insertions, 0 deletions
diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb
index 9a243e07936..00b51f92b12 100644
--- a/app/uploaders/records_uploads.rb
+++ b/app/uploaders/records_uploads.rb
@@ -46,6 +46,10 @@ module RecordsUploads
File.join(store_dir, filename.to_s)
end
+ def filename
+ upload&.path ? File.basename(upload.path) : super
+ end
+
private
# rubocop: disable CodeReuse/ActiveRecord
diff --git a/changelogs/unreleased/stop-signing-avatar-paths.yml b/changelogs/unreleased/stop-signing-avatar-paths.yml
new file mode 100644
index 00000000000..2c2493f0f21
--- /dev/null
+++ b/changelogs/unreleased/stop-signing-avatar-paths.yml
@@ -0,0 +1,5 @@
+---
+title: Speed up generation of avatar URLs when using object storage
+merge_request:
+author:
+type: performance
diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb
index 3592a11360d..ab98976ec27 100644
--- a/spec/uploaders/records_uploads_spec.rb
+++ b/spec/uploaders/records_uploads_spec.rb
@@ -94,4 +94,13 @@ describe RecordsUploads do
expect { uploader.remove! }.to change { Upload.count }.from(1).to(0)
end
end
+
+ describe '#filename' do
+ it 'gets the filename from the path recorded in the database, not CarrierWave' do
+ uploader.store!(upload_fixture('rails_sample.jpg'))
+ expect_any_instance_of(GitlabUploader).not_to receive(:filename)
+
+ expect(uploader.filename).to eq('rails_sample.jpg')
+ end
+ end
end