Merge branch 'fix-avatars-n-plus-one' into 'master'

Fix an N+1 in avatar URLs

Closes #43114

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

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

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

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

app/models/personal_snippet.rb

 class PersonalSnippet < Snippet
+  include WithUploads
 end

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

changelogs/unreleased/fix-avatars-n-plus-one.yml

+---
+title: Fix an N+1 when loading user avatars
+merge_request:
+author:
+type: performance

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

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
-            expect(project.reload.avatar.file_storage?).to be_falsey
+            project.reload
+            BatchLoader::Executor.clear_current
+            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