diff options
| -rw-r--r-- | app/models/concerns/avatarable.rb | 47 | ||||
| -rw-r--r-- | app/models/concerns/with_uploads.rb | 4 | ||||
| -rw-r--r-- | app/models/note.rb | 4 | ||||
| -rw-r--r-- | app/models/personal_snippet.rb | 1 | ||||
| -rw-r--r-- | app/uploaders/object_storage.rb | 4 | ||||
| -rw-r--r-- | changelogs/unreleased/fix-avatars-n-plus-one.yml | 5 | ||||
| -rw-r--r-- | spec/uploaders/object_storage_spec.rb | 22 | ||||
| -rw-r--r-- | spec/uploaders/workers/object_storage/background_move_worker_spec.rb | 6 |
8 files changed, 89 insertions, 4 deletions
diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb index 13246a774e3..095897b08e3 100644 --- a/app/models/concerns/avatarable.rb +++ b/app/models/concerns/avatarable.rb @@ -4,11 +4,14 @@ module Avatarable included do prepend ShadowMethods include ObjectStorage::BackgroundMove + include Gitlab::Utils::StrongMemoize validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } validates :avatar, file_size: { maximum: 200.kilobytes.to_i } mount_uploader :avatar, AvatarUploader + + after_initialize :add_avatar_to_batch end module ShadowMethods @@ -18,6 +21,17 @@ module Avatarable avatar_path(only_path: args.fetch(:only_path, true)) || super end + + def retrieve_upload(identifier, paths) + upload = retrieve_upload_from_batch(identifier) + + # This fallback is needed when deleting an upload, because we may have + # already been removed from the DB. We have to check an explicit `#nil?` + # because it's a BatchLoader instance. + upload = super if upload.nil? + + upload + end end def avatar_type @@ -52,4 +66,37 @@ module Avatarable url_base + avatar.local_url end + + # Path that is persisted in the tracking Upload model. Used to fetch the + # upload from the model. + def upload_paths(identifier) + avatar_mounter.blank_uploader.store_dirs.map { |store, path| File.join(path, identifier) } + end + + private + + def retrieve_upload_from_batch(identifier) + BatchLoader.for(identifier: identifier, model: self).batch(key: self.class) do |upload_params, loader, args| + model_class = args[:key] + paths = upload_params.flat_map do |params| + params[:model].upload_paths(params[:identifier]) + end + + Upload.where(uploader: AvatarUploader, path: paths).find_each do |upload| + model = model_class.instantiate('id' => upload.model_id) + + loader.call({ model: model, identifier: File.basename(upload.path) }, upload) + end + end + end + + def add_avatar_to_batch + return unless avatar_mounter + + avatar_mounter.read_identifiers.each { |identifier| retrieve_upload_from_batch(identifier) } + end + + def avatar_mounter + strong_memoize(:avatar_mounter) { _mounter(:avatar) } + end end diff --git a/app/models/concerns/with_uploads.rb b/app/models/concerns/with_uploads.rb index e7cfffb775b..4245d083a49 100644 --- a/app/models/concerns/with_uploads.rb +++ b/app/models/concerns/with_uploads.rb @@ -36,4 +36,8 @@ module WithUploads upload.destroy end end + + def retrieve_upload(_identifier, paths) + uploads.find_by(path: paths) + end end diff --git a/app/models/note.rb b/app/models/note.rb index 02f7a9b1e4f..41c04ae0571 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -435,6 +435,10 @@ class Note < ActiveRecord::Base super.merge(noteable: noteable) end + def retrieve_upload(_identifier, paths) + Upload.find_by(model: self, path: paths) + end + private def keep_around_commit diff --git a/app/models/personal_snippet.rb b/app/models/personal_snippet.rb index 82c1c4de3a0..355624fd552 100644 --- a/app/models/personal_snippet.rb +++ b/app/models/personal_snippet.rb @@ -1,2 +1,3 @@ class PersonalSnippet < Snippet + include WithUploads end diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 3bb2e1ea63a..5aa1bc7227c 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -33,7 +33,7 @@ module ObjectStorage unless current_upload_satisfies?(paths, model) # the upload we already have isn't right, find the correct one - self.upload = uploads.find_by(model: model, path: paths) + self.upload = model&.retrieve_upload(identifier, paths) end super @@ -46,7 +46,7 @@ module ObjectStorage end def upload=(upload) - return unless upload + return if upload.nil? self.object_store = upload.store super diff --git a/changelogs/unreleased/fix-avatars-n-plus-one.yml b/changelogs/unreleased/fix-avatars-n-plus-one.yml new file mode 100644 index 00000000000..c5b42071f2b --- /dev/null +++ b/changelogs/unreleased/fix-avatars-n-plus-one.yml @@ -0,0 +1,5 @@ +--- +title: Fix an N+1 when loading user avatars +merge_request: +author: +type: performance diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb index 01166865e88..cda911032b2 100644 --- a/spec/uploaders/object_storage_spec.rb +++ b/spec/uploaders/object_storage_spec.rb @@ -739,4 +739,26 @@ describe ObjectStorage do end end end + + describe '#retrieve_from_store!' do + [:group, :project, :user].each do |model| + context "for #{model}s" do + let(:models) { create_list(model, 3, :with_avatar).map(&:reload) } + let(:avatars) { models.map(&:avatar) } + + it 'batches fetching uploads from the database' do + # Ensure that these are all created and fully loaded before we start + # running queries for avatars + models + + expect { avatars }.not_to exceed_query_limit(1) + end + + it 'fetches a unique upload for each model' do + expect(avatars.map(&:url).uniq).to eq(avatars.map(&:url)) + expect(avatars.map(&:upload).uniq).to eq(avatars.map(&:upload)) + end + end + end + end end diff --git a/spec/uploaders/workers/object_storage/background_move_worker_spec.rb b/spec/uploaders/workers/object_storage/background_move_worker_spec.rb index b34f427fd8a..95813d15e52 100644 --- a/spec/uploaders/workers/object_storage/background_move_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/background_move_worker_spec.rb @@ -125,8 +125,10 @@ describe ObjectStorage::BackgroundMoveWorker do it "migrates file to remote storage" do perform + project.reload + BatchLoader::Executor.clear_current - expect(project.reload.avatar.file_storage?).to be_falsey + expect(project.avatar).not_to be_file_storage end end @@ -137,7 +139,7 @@ describe ObjectStorage::BackgroundMoveWorker do it "migrates file to remote storage" do perform - expect(project.reload.avatar.file_storage?).to be_falsey + expect(project.reload.avatar).not_to be_file_storage end end end |