Only link LFS objects that are in merge request

Utilize existing Projects::LfsPointers::LfsLinkService after
getting the OIDs of LFS objects that are present in the MR.

Also call the service in MergeRequests::RefreshService so if new
LFS object/s get pushed to MR's source branch, they will be linked
to target project as well (if they don't exist yet).

app/services/lfs_objects_projects/bulk_create_service.rb

-# frozen_string_literal: true
-
-module LfsObjectsProjects
-  class BulkCreateService < BaseService
-    def initialize(project, params = {})
-      @project = project
-      @params = params
-    end
-
-    def execute
-      target_project = params[:target_project]
-
-      return unless target_project.present?
-
-      Gitlab::Database.bulk_insert(
-        LfsObjectsProject.table_name,
-        lfs_objects_projects_map(target_project),
-        on_conflict: :do_nothing
-      )
-    end
-
-    private
-
-    # rubocop: disable CodeReuse/ActiveRecord
-    def lfs_objects_projects_map(target_project)
-      project.lfs_objects_projects.where.not(lfs_object: target_project.lfs_objects).map do |objects_project|
-        {
-          lfs_object_id: objects_project.lfs_object_id,
-          project_id: target_project.id,
-          repository_type: objects_project.read_attribute_before_type_cast(:repository_type)
-        }
-      end
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-  end
-end

app/services/merge_requests/create_service.rb

@@ -67,11 +67,7 @@ module MergeRequests
     end
 
     def link_lfs_objects(issuable)
-      return if issuable.source_project == issuable.target_project
-
-      LfsObjectsProjects::BulkCreateService
-        .new(issuable.source_project, target_project: issuable.target_project)
-        .execute
+      LinkLfsObjectsService.new(issuable.target_project).execute(issuable)
     end
   end
 end

app/services/merge_requests/link_lfs_objects_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class LinkLfsObjectsService < ::BaseService
+    def execute(merge_request, oldrev: merge_request.diff_base_sha, newrev: merge_request.diff_head_sha)
+      return if merge_request.source_project == project
+      return if no_changes?(oldrev, newrev)
+
+      new_lfs_oids = lfs_oids(merge_request.source_project.repository, oldrev, newrev)
+
+      return if new_lfs_oids.empty?
+
+      Projects::LfsPointers::LfsLinkService
+        .new(project)
+        .execute(new_lfs_oids)
+    end
+
+    private
+
+    def no_changes?(oldrev, newrev)
+      oldrev == newrev
+    end
+
+    def lfs_oids(source_repository, oldrev, newrev)
+      Gitlab::Git::LfsChanges
+        .new(source_repository, newrev)
+        .new_pointers(not_in: [oldrev])
+        .map(&:lfs_oid)
+    end
+  end
+end

app/services/merge_requests/refresh_service.rb

@@ -21,6 +21,7 @@ module MergeRequests
       # empty diff during a manual merge
       close_upon_missing_source_branch_ref
       post_merge_manually_merged
+      link_forks_lfs_objects
       reload_merge_requests
       outdate_suggestions
       refresh_pipelines_on_merge_requests
@@ -91,17 +92,25 @@ module MergeRequests
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    # Link LFS objects that exists in forks but does not exists in merge requests
+    # target project
+    def link_forks_lfs_objects
+      return unless @push.branch_updated?
+
+      merge_requests_for_forks.find_each do |mr|
+        LinkLfsObjectsService
+          .new(mr.target_project)
+          .execute(mr, oldrev: @push.oldrev, newrev: @push.newrev)
+      end
+    end
+
     # Refresh merge request diff if we push to source or target branch of merge request
     # Note: we should update merge requests from forks too
-    # rubocop: disable CodeReuse/ActiveRecord
     def reload_merge_requests
       merge_requests = @project.merge_requests.opened
         .by_source_or_target_branch(@push.branch_name).to_a
 
-      # Fork merge requests
-      merge_requests += MergeRequest.opened
-        .where(source_branch: @push.branch_name, source_project: @project)
-        .where.not(target_project: @project).to_a
+      merge_requests += merge_requests_for_forks.to_a
 
       filter_merge_requests(merge_requests).each do |merge_request|
         if branch_and_project_match?(merge_request) || @push.force_push?
@@ -117,7 +126,6 @@ module MergeRequests
       # @source_merge_requests diffs (for MergeRequest#commit_shas for instance).
       merge_requests_for_source_branch(reload: true)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     def push_commit_ids
       @push_commit_ids ||= @commits.map(&:id)
@@ -282,6 +290,15 @@ module MergeRequests
       @source_merge_requests = nil if reload
       @source_merge_requests ||= merge_requests_for(@push.branch_name)
     end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def merge_requests_for_forks
+      @merge_requests_for_forks ||=
+        MergeRequest.opened
+          .where(source_branch: @push.branch_name, source_project: @project)
+          .where.not(target_project: @project)
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
   end
 end
 

app/workers/repository_fork_worker.rb

@@ -30,9 +30,9 @@ class RepositoryForkWorker
     result = gitlab_shell.fork_repository(source_project, target_project)
 
     if result
-      LfsObjectsProjects::BulkCreateService
-        .new(source_project, target_project: target_project)
-        .execute
+      Projects::LfsPointers::LfsLinkService
+        .new(target_project)
+        .execute(lfs_pointers(source_project))
     else
       raise "Unable to fork project #{target_project.id} for repository #{source_project.disk_path} -> #{target_project.disk_path}"
     end
@@ -46,4 +46,8 @@ class RepositoryForkWorker
     Rails.logger.info("Project #{project.full_path} was in inconsistent state (#{project.import_status}) while forking.") # rubocop:disable Gitlab/RailsLogger
     false
   end
+
+  def lfs_pointers(project)
+    project.lfs_objects.map(&:oid)
+  end
 end

spec/requests/api/merge_requests_spec.rb

@@ -1492,7 +1492,7 @@ describe API::MergeRequests do
       end
     end
 
-    context 'forked projects' do
+    context 'forked projects', :sidekiq_might_not_need_inline do
       let!(:user2) { create(:user) }
       let(:project) { create(:project, :public, :repository) }
       let!(:forked_project) { fork_project(project, user2, repository: true) }

spec/services/lfs_objects_projects/bulk_create_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe LfsObjectsProjects::BulkCreateService do
-  let(:project) { create(:project) }
-  let(:target_project) { create(:project) }
-  let(:params) { { target_project: target_project } }
-
-  subject { described_class.new(project, params).execute }
-
-  shared_examples_for 'LFS objects assigned to target project' do
-    it 'creates LfsObjectsProject records for the target project' do
-      expect { subject }.to change { target_project.lfs_objects.count }
-      expect(target_project.lfs_objects).to eq(project.lfs_objects)
-    end
-  end
-
-  context 'target_project is not associated with any of the LFS objects' do
-    before do
-      create(:lfs_objects_project, project: project)
-      create(:lfs_objects_project, project: project)
-    end
-
-    it_behaves_like 'LFS objects assigned to target project'
-  end
-
-  context 'target_project already associated with some of the LFS objects' do
-    before do
-      objects_project = create(:lfs_objects_project, project: project)
-      create(:lfs_objects_project, project: target_project, lfs_object: objects_project.lfs_object)
-      create(:lfs_objects_project, project: project)
-    end
-
-    it_behaves_like 'LFS objects assigned to target project'
-  end
-
-  context 'target_project is not passed' do
-    let(:params) { {} }
-
-    it 'does not create LfsObjectsProject records for the target project' do
-      expect { subject }.not_to change { target_project.lfs_objects.count }
-      expect(target_project.lfs_objects).to be_empty
-    end
-  end
-end

spec/services/merge_requests/create_service_spec.rb

@@ -483,9 +483,9 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
           expect(merge_request).to be_persisted
         end
 
-        it 'calls LfsObjectsProjects::BulkCreateService#execute', :sidekiq_might_not_need_inline do
-          expect_next_instance_of(LfsObjectsProjects::BulkCreateService) do |service|
-            expect(service).to receive(:execute)
+        it 'calls MergeRequests::LinkLfsObjectsService#execute', :sidekiq_might_not_need_inline do
+          expect_next_instance_of(MergeRequests::LinkLfsObjectsService) do |service|
+            expect(service).to receive(:execute).with(instance_of(MergeRequest))
           end
 
           described_class.new(project, user, opts).execute

spec/services/merge_requests/link_lfs_objects_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::LinkLfsObjectsService, :sidekiq_inline do
+  include ProjectForksHelper
+  include RepoHelpers
+
+  let(:target_project) { create(:project, :public, :repository) }
+
+  let(:merge_request) do
+    create(
+      :merge_request,
+      target_project: target_project,
+      target_branch: 'lfs',
+      source_project: source_project,
+      source_branch: 'link-lfs-objects'
+    )
+  end
+
+  subject { described_class.new(target_project) }
+
+  shared_examples_for 'linking LFS objects' do
+    context 'when source project is the same as target project' do
+      let(:source_project) { target_project }
+
+      it 'does not call Projects::LfsPointers::LfsLinkService#execute' do
+        expect(Projects::LfsPointers::LfsLinkService).not_to receive(:new)
+
+        execute
+      end
+    end
+
+    context 'when source project is different from target project' do
+      let(:user) { create(:user) }
+      let(:source_project) { fork_project(target_project, user, namespace: user.namespace, repository: true) }
+
+      before do
+        create_branch(source_project, 'link-lfs-objects', 'lfs')
+      end
+
+      context 'and there are changes' do
+        before do
+          allow(source_project).to receive(:lfs_enabled?).and_return(true)
+        end
+
+        context 'and there are LFS objects added' do
+          before do
+            create_file_in_repo(source_project, 'link-lfs-objects', 'link-lfs-objects', 'one.lfs', 'One')
+            create_file_in_repo(source_project, 'link-lfs-objects', 'link-lfs-objects', 'two.lfs', 'Two')
+          end
+
+          it 'calls Projects::LfsPointers::LfsLinkService#execute with OIDs of LFS objects in merge request' do
+            expect_next_instance_of(Projects::LfsPointers::LfsLinkService) do |service|
+              expect(service).to receive(:execute).with(%w[
+                8b12507783d5becacbf2ebe5b01a60024d8728a8f86dcc818bce699e8b3320bc
+                94a72c074cfe574742c9e99e863322f73feff82981d065ff65a0308f44f19f62
+              ])
+            end
+
+            execute
+          end
+        end
+
+        context 'but there are no LFS objects added' do
+          before do
+            create_file_in_repo(source_project, 'link-lfs-objects', 'link-lfs-objects', 'one.txt', 'One')
+          end
+
+          it 'does not call Projects::LfsPointers::LfsLinkService#execute' do
+            expect(Projects::LfsPointers::LfsLinkService).not_to receive(:new)
+
+            execute
+          end
+        end
+      end
+
+      context 'and there are no changes' do
+        it 'does not call Projects::LfsPointers::LfsLinkService#execute' do
+          expect(Projects::LfsPointers::LfsLinkService).not_to receive(:new)
+
+          execute
+        end
+      end
+    end
+  end
+
+  context 'when no oldrev and newrev passed' do
+    let(:execute) { subject.execute(merge_request) }
+
+    it_behaves_like 'linking LFS objects'
+  end
+
+  context 'when oldrev and newrev are passed' do
+    let(:execute) { subject.execute(merge_request, oldrev: merge_request.diff_base_sha, newrev: merge_request.diff_head_sha) }
+
+    it_behaves_like 'linking LFS objects'
+  end
+
+  def create_branch(project, new_name, branch_name)
+    ::Branches::CreateService.new(project, user).execute(new_name, branch_name)
+  end
+end

spec/services/merge_requests/refresh_service_spec.rb

@@ -384,6 +384,14 @@ describe MergeRequests::RefreshService do
       end
 
       context 'open fork merge request' do
+        it 'calls MergeRequests::LinkLfsObjectsService#execute' do
+          expect_next_instance_of(MergeRequests::LinkLfsObjectsService) do |svc|
+            expect(svc).to receive(:execute).with(@fork_merge_request, oldrev: @oldrev, newrev: @newrev)
+          end
+
+          refresh
+        end
+
         it 'executes hooks with update action' do
           refresh
 

spec/workers/repository_fork_worker_spec.rb

@@ -47,15 +47,6 @@ describe RepositoryForkWorker do
         perform!
       end
 
-      it 'calls LfsObjectsProjects::BulkCreateService#execute' do
-        expect_fork_repository.and_return(true)
-        expect_next_instance_of(LfsObjectsProjects::BulkCreateService) do |service|
-          expect(service).to receive(:execute)
-        end
-
-        perform!
-      end
-
       it 'protects the default branch' do
         expect_fork_repository.and_return(true)
 
@@ -88,6 +79,15 @@ describe RepositoryForkWorker do
 
         expect { perform! }.to raise_error(StandardError, error_message)
       end
+
+      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.map(&:oid))
+        end
+
+        perform!
+      end
     end
 
     context 'only project ID passed' do