Merge branch '207216-lfs-batch-upload-fix' into 'master'

Mark existing LFS object for upload for forks

Closes #207216

See merge request gitlab-org/gitlab!26344

app/controllers/concerns/lfs_request.rb

@@ -116,6 +116,10 @@ module LfsRequest
     @objects ||= (params[:objects] || []).to_a
   end
 
+  def objects_oids
+    objects.map { |o| o['oid'].to_s }
+  end
+
   def has_authentication_ability?(capability)
     (authentication_abilities || []).include?(capability)
   end

app/controllers/repositories/lfs_api_controller.rb

@@ -45,15 +45,9 @@ module Repositories
       params[:operation] == 'upload'
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def existing_oids
-      @existing_oids ||= begin
-        project.all_lfs_objects.where(oid: objects.map { |o| o['oid'].to_s }).pluck(:oid)
-      end
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
     def download_objects!
+      existing_oids = project.all_lfs_objects_oids(oids: objects_oids)
+
       objects.each do |object|
         if existing_oids.include?(object[:oid])
           object[:actions] = download_actions(object)
@@ -68,13 +62,17 @@ module Repositories
           }
         end
       end
+
       objects
     end
 
     def upload_objects!
+      existing_oids = project.lfs_objects_oids(oids: objects_oids)
+
       objects.each do |object|
         object[:actions] = upload_actions(object) unless existing_oids.include?(object[:oid])
       end
+
       objects
     end
 

app/models/project.rb

@@ -1398,12 +1398,17 @@ class Project < ApplicationRecord
       .where(lfs_objects_projects: { project_id: [self, lfs_storage_project] })
   end
 
-  # TODO: Call `#lfs_objects` instead once all LfsObjectsProject records are
-  # backfilled. At that point, projects can look at their own `lfs_objects`.
+  # TODO: Remove this method once all LfsObjectsProject records are backfilled
+  # for forks. At that point, projects can look at their own `lfs_objects` so
+  # `lfs_objects_oids` can be used instead.
   #
   # See https://gitlab.com/gitlab-org/gitlab/issues/122002 for more info.
-  def lfs_objects_oids
-    all_lfs_objects.pluck(:oid)
+  def all_lfs_objects_oids(oids: [])
+    oids(all_lfs_objects, oids: oids)
+  end
+
+  def lfs_objects_oids(oids: [])
+    oids(lfs_objects, oids: oids)
   end
 
   def personal?
@@ -2515,6 +2520,12 @@ class Project < ApplicationRecord
     reset
     retry
   end
+
+  def oids(objects, oids: [])
+    collection = oids.any? ? objects.where(oid: oids) : objects
+
+    collection.pluck(:oid)
+  end
 end
 
 Project.prepend_if_ee('EE::Project')

app/workers/repository_fork_worker.rb

@@ -52,7 +52,7 @@ class RepositoryForkWorker # rubocop:disable Scalability/IdempotentWorker
   def link_lfs_objects(source_project, target_project)
     Projects::LfsPointers::LfsLinkService
         .new(target_project)
-        .execute(source_project.lfs_objects_oids)
+        .execute(source_project.all_lfs_objects_oids)
   rescue Projects::LfsPointers::LfsLinkService::TooManyOidsError
     raise_fork_failure(
       source_project,

changelogs/unreleased/207216-lfs-batch-upload-fix.yml

+---
+title: Mark existing LFS object for upload for forks
+merge_request: 26344
+author:
+type: fixed

spec/models/project_spec.rb

@@ -5692,6 +5692,53 @@ describe Project do
     end
   end
 
+  describe '#all_lfs_objects_oids' do
+    let(:project) { create(:project) }
+    let(:lfs_object) { create(:lfs_object) }
+    let(:another_lfs_object) { create(:lfs_object) }
+
+    subject { project.all_lfs_objects_oids }
+
+    context 'when project has associated LFS objects' do
+      before do
+        create(:lfs_objects_project, lfs_object: lfs_object, project: project)
+        create(:lfs_objects_project, lfs_object: another_lfs_object, project: project)
+      end
+
+      it 'returns OIDs of LFS objects' do
+        expect(subject).to match_array([lfs_object.oid, another_lfs_object.oid])
+      end
+
+      context 'and there are specified oids' do
+        subject { project.all_lfs_objects_oids(oids: [lfs_object.oid]) }
+
+        it 'returns OIDs of LFS objects that match specified oids' do
+          expect(subject).to eq([lfs_object.oid])
+        end
+      end
+    end
+
+    context 'when fork has associated LFS objects to itself and source' do
+      let(:source) { create(:project) }
+      let(:project) { fork_project(source) }
+
+      before do
+        create(:lfs_objects_project, lfs_object: lfs_object, project: source)
+        create(:lfs_objects_project, lfs_object: another_lfs_object, project: project)
+      end
+
+      it 'returns OIDs of LFS objects' do
+        expect(subject).to match_array([lfs_object.oid, another_lfs_object.oid])
+      end
+    end
+
+    context 'when project has no associated LFS objects' do
+      it 'returns empty array' do
+        expect(subject).to be_empty
+      end
+    end
+  end
+
   describe '#lfs_objects_oids' do
     let(:project) { create(:project) }
     let(:lfs_object) { create(:lfs_object) }
@@ -5708,6 +5755,14 @@ describe Project do
       it 'returns OIDs of LFS objects' do
         expect(subject).to match_array([lfs_object.oid, another_lfs_object.oid])
       end
+
+      context 'and there are specified oids' do
+        subject { project.lfs_objects_oids(oids: [lfs_object.oid]) }
+
+        it 'returns OIDs of LFS objects that match specified oids' do
+          expect(subject).to eq([lfs_object.oid])
+        end
+      end
     end
 
     context 'when project has no associated LFS objects' do

spec/requests/lfs_http_spec.rb

@@ -690,10 +690,7 @@ describe 'Git LFS API and storage' do
           end
 
           context 'when pushing an LFS object that already exists' do
-            let(:update_lfs_permissions) do
-              other_project.lfs_objects << lfs_object
-            end
-
+            shared_examples_for 'batch upload with existing LFS object' do
               it_behaves_like 'LFS http 200 response'
 
               it 'responds with links the object to the project' do
@@ -708,6 +705,21 @@ describe 'Git LFS API and storage' do
               it_behaves_like 'process authorization header', renew_authorization: true
             end
 
+            let(:update_lfs_permissions) do
+              other_project.lfs_objects << lfs_object
+            end
+
+            context 'in another project' do
+              it_behaves_like 'batch upload with existing LFS object'
+            end
+
+            context 'in source of fork project' do
+              let(:project) { fork_project(other_project) }
+
+              it_behaves_like 'batch upload with existing LFS object'
+            end
+          end
+
           context 'when pushing a LFS object that does not exist' do
             it_behaves_like 'pushes new LFS objects', renew_authorization: true
           end

spec/workers/repository_fork_worker_spec.rb

@@ -83,7 +83,7 @@ describe RepositoryForkWorker do
       it 'calls Projects::LfsPointers::LfsLinkService#execute with OIDs of source project LFS objects' do
         expect_fork_repository.and_return(true)
         expect_next_instance_of(Projects::LfsPointers::LfsLinkService) do |service|
-          expect(service).to receive(:execute).with(project.lfs_objects_oids)
+          expect(service).to receive(:execute).with(project.all_lfs_objects_oids)
         end
 
         perform!