Fix an N+1 in avatar URLs

This is tricky: the query was being run in
`ObjectStorage::Extension::RecordsUploads#retrieve_from_store!`, but we can't
just add batch loading there, because the `#upload=` method there would use the
result immediately, making the batch only have one item.

Instead, we can pre-emptively add an item to the batch whenever an avatarable
object is initialized, and then reuse that batch item in
`#retrieve_from_store!`. However, this also has problems:

1. There is a lot of logic in `Avatarable#retrieve_upload_from_batch`.
2. Some of that logic constructs a 'fake' model for the batch key. This should
   be fine, because of ActiveRecord's override of `#==`, but it relies on that
   staying the same.

2 files changed, 26 insertions, 2 deletions

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