Merge branch '20042-create-lfs-objects-project' into 'master'

Create LfsObjectsProject record for forks as well

See merge request gitlab-org/gitlab!22418

app/controllers/concerns/lfs_request.rb

@@ -112,10 +112,6 @@ module LfsRequest
     has_authentication_ability?(:build_download_code) && can?(user, :build_download_code, project)
   end
 
-  def storage_project
-    @storage_project ||= project.lfs_storage_project
-  end
-
   def objects
     @objects ||= (params[:objects] || []).to_a
   end

app/controllers/repositories/lfs_storage_controller.rb

@@ -80,12 +80,13 @@ module Repositories
       LfsObject.create!(oid: oid, size: size, file: uploaded_file)
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def link_to_project!(object)
-      if object && !object.projects.exists?(storage_project.id)
-        object.lfs_objects_projects.create!(project: storage_project)
-      end
+      return unless object
+
+      LfsObjectsProject.safe_find_or_create_by!(
+        project: project,
+        lfs_object: object
+      )
     end
-    # rubocop: enable CodeReuse/ActiveRecord
   end
 end

app/models/project.rb

@@ -1359,6 +1359,10 @@ class Project < ApplicationRecord
     forked_from_project || fork_network&.root_project
   end
 
+  # TODO: Remove this method once all LfsObjectsProject records are backfilled
+  # for forks.
+  #
+  # See https://gitlab.com/gitlab-org/gitlab/issues/122002 for more info.
   def lfs_storage_project
     @lfs_storage_project ||= begin
       result = self
@@ -1371,14 +1375,27 @@ class Project < ApplicationRecord
     end
   end
 
-  # This will return all `lfs_objects` that are accessible to the project.
-  # So this might be `self.lfs_objects` if the project is not part of a fork
-  # network, or it is the base of the fork network.
+  # This will return all `lfs_objects` that are accessible to the project and
+  # the fork source. This is needed since older forks won't have access to some
+  # LFS objects directly and have to get it from the fork source.
+  #
+  # TODO: Remove this method once all LfsObjectsProject records are backfilled
+  # for forks. At that point, projects can look at their own `lfs_objects`.
   #
-  # TODO: refactor this to get the correct lfs objects when implementing
-  #       https://gitlab.com/gitlab-org/gitlab-foss/issues/39769
+  # See https://gitlab.com/gitlab-org/gitlab/issues/122002 for more info.
   def all_lfs_objects
-    lfs_storage_project.lfs_objects
+    LfsObject
+      .distinct
+      .joins(:lfs_objects_projects)
+      .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`.
+  #
+  # See https://gitlab.com/gitlab-org/gitlab/issues/122002 for more info.
+  def lfs_objects_oids
+    all_lfs_objects.pluck(:oid)
   end
 
   def personal?

app/services/merge_requests/create_service.rb

@@ -27,6 +27,7 @@ module MergeRequests
       create_pipeline_for(issuable, current_user)
       issuable.update_head_pipeline
       Gitlab::UsageDataCounters::MergeRequestCounter.count(:create)
+      link_lfs_objects(issuable)
 
       super
     end
@@ -64,6 +65,10 @@ module MergeRequests
         raise Gitlab::Access::AccessDeniedError
       end
     end
+
+    def link_lfs_objects(issuable)
+      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/services/projects/fork_service.rb

@@ -26,17 +26,7 @@ module Projects
 
       build_fork_network_member(fork_to_project)
 
-      if link_fork_network(fork_to_project)
-        # A forked project stores its LFS objects in the `forked_from_project`.
-        # So the LFS objects become inaccessible, and therefore delete them from
-        # the database so they'll get cleaned up.
-        #
-        # TODO: refactor this to get the correct lfs objects when implementing
-        #       https://gitlab.com/gitlab-org/gitlab-foss/issues/39769
-        fork_to_project.lfs_objects_projects.delete_all
-
-        fork_to_project
-      end
+      fork_to_project if link_fork_network(fork_to_project)
     end
 
     def fork_new_project

app/services/projects/lfs_pointers/lfs_download_service.rb

@@ -39,9 +39,9 @@ module Projects
       def download_lfs_file!
         with_tmp_file do |tmp_file|
           download_and_save_file!(tmp_file)
-          project.all_lfs_objects << LfsObject.new(oid: lfs_oid,
-                                                   size: lfs_size,
-                                                   file: tmp_file)
+          project.lfs_objects << LfsObject.new(oid: lfs_oid,
+                                               size: lfs_size,
+                                               file: tmp_file)
 
           success
         end

app/services/projects/move_lfs_objects_projects_service.rb

@@ -16,7 +16,7 @@ module Projects
     private
 
     def move_lfs_objects_projects
-      non_existent_lfs_objects_projects.update_all(project_id: @project.lfs_storage_project.id)
+      non_existent_lfs_objects_projects.update_all(project_id: @project.id)
     end
 
     def remove_remaining_lfs_objects_project

app/services/projects/unlink_fork_service.rb

@@ -52,6 +52,10 @@ module Projects
       Projects::ForksCountService.new(project).refresh_cache
     end
 
+    # TODO: Remove this method once all LfsObjectsProject records are backfilled
+    # for forks.
+    #
+    # See https://gitlab.com/gitlab-org/gitlab/issues/122002 for more info.
     def save_lfs_objects
       return unless @project.forked?
 

app/workers/repository_fork_worker.rb

@@ -29,7 +29,15 @@ class RepositoryForkWorker
 
     result = gitlab_shell.fork_repository(source_project, target_project)
 
-    raise "Unable to fork project #{target_project.id} for repository #{source_project.disk_path} -> #{target_project.disk_path}" unless result
+    if result
+      link_lfs_objects(source_project, target_project)
+    else
+      raise_fork_failure(
+        source_project,
+        target_project,
+        'Failed to create fork repository'
+      )
+    end
 
     target_project.after_import
   end
@@ -40,4 +48,20 @@ 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 link_lfs_objects(source_project, target_project)
+    Projects::LfsPointers::LfsLinkService
+        .new(target_project)
+        .execute(source_project.lfs_objects_oids)
+  rescue Projects::LfsPointers::LfsLinkService::TooManyOidsError
+    raise_fork_failure(
+      source_project,
+      target_project,
+      'Source project has too many LFS objects'
+    )
+  end
+
+  def raise_fork_failure(source_project, target_project, reason)
+    raise "Unable to fork project #{target_project.id} for repository #{source_project.disk_path} -> #{target_project.disk_path}: #{reason}"
+  end
 end

changelogs/unreleased/20042-create-lfs-objects-project.yml

+---
+title: Create LfsObjectsProject record for forks as well
+merge_request: 22418
+author:
+type: fixed

db/migrate/20200123040535_add_multi_column_index_on_lfs_objects_projects.rb

+# frozen_string_literal: true
+
+class AddMultiColumnIndexOnLfsObjectsProjects < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :lfs_objects_projects, [:project_id, :lfs_object_id]
+  end
+
+  def down
+    remove_concurrent_index :lfs_objects_projects, [:project_id, :lfs_object_id]
+  end
+end

db/migrate/20200123045415_remove_project_id_index_on_lfs_objects_projects.rb

+# frozen_string_literal: true
+
+class RemoveProjectIdIndexOnLfsObjectsProjects < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index :lfs_objects_projects, :project_id
+  end
+
+  def down
+    add_concurrent_index :lfs_objects_projects, :project_id
+  end
+end

db/schema.rb

@@ -2347,7 +2347,7 @@ ActiveRecord::Schema.define(version: 2020_02_06_111847) do
     t.datetime "updated_at"
     t.integer "repository_type", limit: 2
     t.index ["lfs_object_id"], name: "index_lfs_objects_projects_on_lfs_object_id"
-    t.index ["project_id"], name: "index_lfs_objects_projects_on_project_id"
+    t.index ["project_id", "lfs_object_id"], name: "index_lfs_objects_projects_on_project_id_and_lfs_object_id"
   end
 
   create_table "licenses", id: :serial, force: :cascade do |t|

doc/api/projects.md

@@ -2000,11 +2000,6 @@ Allows modification of the forked relationship between existing projects. Availa
 
 ### Create a forked from/to relation between existing projects
 
-CAUTION: **Warning:**
-This will destroy the LFS objects stored in the fork.
-So to retain the LFS objects, make sure you've pulled them **before** creating the fork relation,
-and push them again **after** creating the fork relation.
-
 ```
 POST /projects/:id/fork/:forked_from_id
 ```

spec/controllers/concerns/lfs_request_spec.rb

@@ -10,8 +10,6 @@ describe LfsRequest do
     include LfsRequest
 
     def show
-      storage_project
-
       head :ok
     end
 
@@ -38,22 +36,6 @@ describe LfsRequest do
     stub_lfs_setting(enabled: true)
   end
 
-  describe '#storage_project' do
-    it 'assigns the project as storage project' do
-      get :show, params: { id: project.id }
-
-      expect(assigns(:storage_project)).to eq(project)
-    end
-
-    it 'assigns the source of a forked project' do
-      forked_project = fork_project(project)
-
-      get :show, params: { id: forked_project.id }
-
-      expect(assigns(:storage_project)).to eq(project)
-    end
-  end
-
   context 'user is authenticated without access to lfs' do
     before do
       allow(controller).to receive(:authenticate_user)

spec/models/project_spec.rb

@@ -2700,16 +2700,44 @@ describe Project do
     describe '#all_lfs_objects' do
       let(:lfs_object) { create(:lfs_object) }
 
-      before do
-        project.lfs_objects << lfs_object
+      context 'when LFS object is only associated to the source' do
+        before do
+          project.lfs_objects << lfs_object
+        end
+
+        it 'returns the lfs object for a project' do
+          expect(project.all_lfs_objects).to contain_exactly(lfs_object)
+        end
+
+        it 'returns the lfs object for a fork' do
+          expect(forked_project.all_lfs_objects).to contain_exactly(lfs_object)
+        end
       end
 
-      it 'returns the lfs object for a project' do
-        expect(project.all_lfs_objects).to contain_exactly(lfs_object)
+      context 'when LFS object is only associated to the fork' do
+        before do
+          forked_project.lfs_objects << lfs_object
+        end
+
+        it 'returns nothing' do
+          expect(project.all_lfs_objects).to be_empty
+        end
+
+        it 'returns the lfs object for a fork' do
+          expect(forked_project.all_lfs_objects).to contain_exactly(lfs_object)
+        end
       end
 
-      it 'returns the lfs object for a fork' do
-        expect(forked_project.all_lfs_objects).to contain_exactly(lfs_object)
+      context 'when LFS object is associated to both source and fork' do
+        before do
+          project.lfs_objects << lfs_object
+          forked_project.lfs_objects << lfs_object
+        end
+
+        it 'returns the lfs object for the source and fork' do
+          expect(project.all_lfs_objects).to contain_exactly(lfs_object)
+          expect(forked_project.all_lfs_objects).to contain_exactly(lfs_object)
+        end
       end
     end
   end
@@ -5525,6 +5553,31 @@ describe Project do
     end
   end
 
+  describe '#lfs_objects_oids' do
+    let(:project) { create(:project) }
+    let(:lfs_object) { create(:lfs_object) }
+    let(:another_lfs_object) { create(:lfs_object) }
+
+    subject { project.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
+    end
+
+    context 'when project has no associated LFS objects' do
+      it 'returns empty array' do
+        expect(subject).to be_empty
+      end
+    end
+  end
+
   def rugged_config
     rugged_repo(project.repository).config
   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/requests/lfs_http_spec.rb

@@ -1193,8 +1193,8 @@ describe 'Git LFS API and storage' do
 
             it_behaves_like 'LFS http 200 response'
 
-            it 'LFS object is linked to the source project' do
-              expect(lfs_object.projects.pluck(:id)).to include(upstream_project.id)
+            it 'LFS object is linked to the forked project' do
+              expect(lfs_object.projects.pluck(:id)).to include(project.id)
             end
           end
         end

spec/services/merge_requests/create_service_spec.rb

@@ -483,6 +483,14 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
           expect(merge_request).to be_persisted
         end
 
+        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
+        end
+
         it 'does not create the merge request when the target project is archived' do
           target_project.update!(archived: true)
 

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/services/projects/fork_service_spec.rb

@@ -375,14 +375,6 @@ describe Projects::ForkService do
         expect(fork_from_project.forks_count).to eq(1)
       end
 
-      it 'leaves no LFS objects dangling' do
-        create(:lfs_objects_project, project: fork_to_project)
-
-        expect { subject.execute(fork_to_project) }
-          .to change { fork_to_project.lfs_objects_projects.count }
-          .to(0)
-      end
-
       context 'if the fork is not allowed' do
         let(:fork_from_project) { create(:project, :private) }
 

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -48,10 +48,11 @@ describe Projects::LfsPointers::LfsDownloadService do
   end
 
   shared_examples 'lfs object is created' do
-    it do
+    it 'creates and associate the LFS object to project' do
       expect(subject).to receive(:download_and_save_file!).and_call_original
 
       expect { subject.execute }.to change { LfsObject.count }.by(1)
+      expect(LfsObject.first.projects).to include(project)
     end
 
     it 'returns success result' do

spec/workers/repository_fork_worker_spec.rb

@@ -72,13 +72,33 @@ describe RepositoryForkWorker do
         perform!
       end
 
-      it "handles bad fork" do
-        error_message = "Unable to fork project #{forked_project.id} for repository #{project.disk_path} -> #{forked_project.disk_path}"
+      it 'handles bad fork' do
+        error_message = "Unable to fork project #{forked_project.id} for repository #{project.disk_path} -> #{forked_project.disk_path}: Failed to create fork repository"
 
         expect_fork_repository.and_return(false)
 
         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_oids)
+        end
+
+        perform!
+      end
+
+      it "handles LFS objects link failure" do
+        error_message = "Unable to fork project #{forked_project.id} for repository #{project.disk_path} -> #{forked_project.disk_path}: Source project has too many LFS objects"
+
+        expect_fork_repository.and_return(true)
+        expect_next_instance_of(Projects::LfsPointers::LfsLinkService) do |service|
+          expect(service).to receive(:execute).and_raise(Projects::LfsPointers::LfsLinkService::TooManyOidsError)
+        end
+
+        expect { perform! }.to raise_error(StandardError, error_message)
+      end
     end
 
     context 'only project ID passed' do