Merge branch '321361-move-file-in-object-storage' into 'master'

Refactor how nuget package files are updated

See merge request gitlab-org/gitlab!66728

app/models/packages/package_file.rb

@@ -10,6 +10,9 @@ class Packages::PackageFile < ApplicationRecord
 
   belongs_to :package
 
+  # used to move the linked file within object storage
+  attribute :new_file_path, default: nil
+
   has_one :conan_file_metadatum, inverse_of: :package_file, class_name: 'Packages::Conan::FileMetadatum'
   has_many :package_file_build_infos, inverse_of: :package_file, class_name: 'Packages::PackageFileBuildInfo'
   has_many :pipelines, through: :package_file_build_infos
@@ -80,6 +83,12 @@ class Packages::PackageFile < ApplicationRecord
 
   before_save :update_size_from_file
 
+  # if a new_file_path is provided, we need
+  # * disable the remove_previously_stored_file callback so that carrierwave doesn't take care of the file
+  # * enable a new after_commit callback that will move the file in object storage
+  skip_callback :commit, :after, :remove_previously_stored_file, if: :execute_move_in_object_storage?
+  after_commit :move_in_object_storage, if: :execute_move_in_object_storage?
+
   def download_path
     Gitlab::Routing.url_helpers.download_project_package_file_path(project, self)
   end
@@ -89,6 +98,17 @@ class Packages::PackageFile < ApplicationRecord
   def update_size_from_file
     self.size ||= file.size
   end
+
+  def execute_move_in_object_storage?
+    !file.file_storage? && new_file_path?
+  end
+
+  def move_in_object_storage
+    carrierwave_file = file.file
+
+    carrierwave_file.copy_to(new_file_path)
+    carrierwave_file.delete
+  end
 end
 
 Packages::PackageFile.prepend_mod_with('Packages::PackageFile')

app/services/packages/nuget/update_package_from_metadata_service.rb

@@ -21,22 +21,11 @@ module Packages
 
         try_obtain_lease do
           @package_file.transaction do
-            if existing_package
-              package = link_to_existing_package
-            elsif symbol_package?
-              raise InvalidMetadataError, 'symbol package is invalid, matching package does not exist'
+            if use_new_package_file_updater?
+              new_execute
             else
-              package = update_linked_package
+              legacy_execute
             end
-
-            update_package(package)
-
-            # Updating file_name updates the path where the file is stored.
-            # We must pass the file again so that CarrierWave can handle the update
-            @package_file.update!(
-              file_name: package_filename,
-              file: @package_file.file
-            )
           end
         end
       rescue ActiveRecord::RecordInvalid => e
@@ -45,6 +34,52 @@ module Packages
 
       private
 
+      def new_execute
+        package_to_destroy = nil
+        target_package = @package_file.package
+
+        if existing_package
+          package_to_destroy = @package_file.package
+          target_package = existing_package
+        else
+          if symbol_package?
+            raise InvalidMetadataError, 'symbol package is invalid, matching package does not exist'
+          end
+
+          update_linked_package
+        end
+
+        update_package(target_package)
+
+        ::Packages::UpdatePackageFileService.new(@package_file, package_id: target_package.id, file_name: package_filename)
+                                            .execute
+
+        package_to_destroy&.destroy!
+      end
+
+      def legacy_execute
+        if existing_package
+          package = link_to_existing_package
+        elsif symbol_package?
+          raise InvalidMetadataError, 'symbol package is invalid, matching package does not exist'
+        else
+          package = update_linked_package
+        end
+
+        update_package(package)
+
+        # Updating file_name updates the path where the file is stored.
+        # We must pass the file again so that CarrierWave can handle the update
+        @package_file.update!(
+          file_name: package_filename,
+          file: @package_file.file
+        )
+      end
+
+      def use_new_package_file_updater?
+        ::Feature.enabled?(:packages_nuget_new_package_file_updater, @package_file.project, default_enabled: :yaml)
+      end
+
       def update_package(package)
         return if symbol_package?
 

app/services/packages/update_package_file_service.rb

+# frozen_string_literal: true
+
+module Packages
+  class UpdatePackageFileService
+    delegate :file, to: :@package_file
+
+    def initialize(package_file, params)
+      @package_file = package_file
+      @params = params
+    end
+
+    def execute
+      check_params
+
+      return if same_as_params?
+
+      # we need to access the file *before* updating the attributes linked to its path/key.
+      file_storage_mode = file.file_storage?
+
+      @package_file.package_id = package_id if package_id
+      @package_file.file_name = file_name if file_name
+
+      if file_storage_mode
+        # package file is in mode LOCAL: we can pass the `file` to the update
+        @package_file.file = file
+      else
+        # package file is in mode REMOTE: don't pass the `file` to the update
+        # instead, pass the new file path. This will move the file
+        # in object storage.
+        @package_file.new_file_path = File.join(file.store_dir, @package_file.file_name)
+      end
+
+      @package_file.save!
+    end
+
+    private
+
+    def check_params
+      raise ArgumentError, 'package_file not persisted' unless @package_file.persisted?
+      raise ArgumentError, 'package_id and file_name are blank' if package_id.blank? && file_name.blank?
+    end
+
+    def same_as_params?
+      return false if package_id && package_id != @package_file.package_id
+      return false if file_name && file_name != @package_file.file_name
+
+      true
+    end
+
+    def package_id
+      @params[:package_id]
+    end
+
+    def file_name
+      @params[:file_name]
+    end
+  end
+end

config/feature_flags/development/packages_nuget_new_package_file_updater.yml

+---
+name: packages_nuget_new_package_file_updater
+introduced_by_url:
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/336511
+milestone: '14.1'
+type: development
+group: group::package
+default_enabled: false

spec/models/packages/package_file_spec.rb

@@ -159,4 +159,71 @@ RSpec.describe Packages::PackageFile, type: :model do
       expect { subject }.to change { package_file.size }.from(nil).to(3513)
     end
   end
+
+  context 'update callbacks' do
+    subject { package_file.save! }
+
+    shared_examples 'executing the default callback' do
+      it 'executes the default callback' do
+        expect(package_file).to receive(:remove_previously_stored_file)
+        expect(package_file).not_to receive(:move_in_object_storage)
+
+        subject
+      end
+    end
+
+    context 'with object storage disabled' do
+      let(:package_file) { create(:package_file, file_name: 'file_name.txt') }
+
+      before do
+        stub_package_file_object_storage(enabled: false)
+      end
+
+      it_behaves_like 'executing the default callback'
+
+      context 'with new_file_path set' do
+        before do
+          package_file.new_file_path = 'test'
+        end
+
+        it_behaves_like 'executing the default callback'
+      end
+    end
+
+    context 'with object storage enabled' do
+      let(:package_file) do
+        create(
+          :package_file,
+          file_name: 'file_name.txt',
+          file: CarrierWaveStringFile.new_file(
+            file_content: 'content',
+            filename: 'file_name.txt',
+            content_type: 'text/plain'
+          ),
+          file_store: ::Packages::PackageFileUploader::Store::REMOTE
+        )
+      end
+
+      before do
+        stub_package_file_object_storage(enabled: true)
+      end
+
+      it_behaves_like 'executing the default callback'
+
+      context 'with new_file_path set' do
+        before do
+          package_file.new_file_path = 'test'
+        end
+
+        it 'executes the move_in_object_storage callback' do
+          expect(package_file).not_to receive(:remove_previously_stored_file)
+          expect(package_file).to receive(:move_in_object_storage).and_call_original
+          expect(package_file.file.file).to receive(:copy_to).and_call_original
+          expect(package_file.file.file).to receive(:delete).and_call_original
+
+          subject
+        end
+      end
+    end
+  end
 end

spec/services/packages/nuget/update_package_from_metadata_service_spec.rb

@@ -49,7 +49,7 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_
           allow(service).to receive(:metadata).and_return(metadata)
         end
 
-        it 'does not update the package' do
+        it 'does not update the package', :aggregate_failures do
           expect(service).to receive(:try_obtain_lease).and_call_original
 
           expect { subject }
@@ -63,213 +63,234 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_
       end
     end
 
-    context 'with no existing package' do
-      let(:package_id) { package.id }
-
-      it 'updates package and package file' do
-        expect { subject }
-          .to change { ::Packages::Package.count }.by(1)
-          .and change { Packages::Dependency.count }.by(1)
-          .and change { Packages::DependencyLink.count }.by(1)
-          .and change { ::Packages::Nuget::Metadatum.count }.by(0)
-
-        expect(package.reload.name).to eq(package_name)
-        expect(package.version).to eq(package_version)
-        expect(package).to be_default
-        expect(package_file.reload.file_name).to eq(package_file_name)
-        # hard reset needed to properly reload package_file.file
-        expect(Packages::PackageFile.find(package_file.id).file.size).not_to eq 0
-      end
-
-      it_behaves_like 'taking the lease'
+    shared_examples 'handling all conditions' do
+      context 'with no existing package' do
+        let(:package_id) { package.id }
 
-      it_behaves_like 'not updating the package if the lease is taken'
-    end
+        it 'updates package and package file', :aggregate_failures do
+          expect { subject }
+            .to not_change { ::Packages::Package.count }
+            .and change { Packages::Dependency.count }.by(1)
+            .and change { Packages::DependencyLink.count }.by(1)
+            .and change { ::Packages::Nuget::Metadatum.count }.by(0)
 
-    context 'with existing package' do
-      let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) }
-      let(:package_id) { existing_package.id }
+          expect(package.reload.name).to eq(package_name)
+          expect(package.version).to eq(package_version)
+          expect(package).to be_default
+          expect(package_file.reload.file_name).to eq(package_file_name)
+          # hard reset needed to properly reload package_file.file
+          expect(Packages::PackageFile.find(package_file.id).file.size).not_to eq 0
+        end
 
-      it 'link existing package and updates package file' do
-        expect(service).to receive(:try_obtain_lease).and_call_original
+        it_behaves_like 'taking the lease'
 
-        expect { subject }
-          .to change { ::Packages::Package.count }.by(-1)
-          .and change { Packages::Dependency.count }.by(0)
-          .and change { Packages::DependencyLink.count }.by(0)
-          .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0)
-          .and change { ::Packages::Nuget::Metadatum.count }.by(0)
-        expect(package_file.reload.file_name).to eq(package_file_name)
-        expect(package_file.package).to eq(existing_package)
+        it_behaves_like 'not updating the package if the lease is taken'
       end
 
-      it_behaves_like 'taking the lease'
-
-      it_behaves_like 'not updating the package if the lease is taken'
-    end
+      context 'with existing package' do
+        let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) }
+        let(:package_id) { existing_package.id }
 
-    context 'with a nuspec file with metadata' do
-      let(:nuspec_filepath) { 'packages/nuget/with_metadata.nuspec' }
-      let(:expected_tags) { %w(foo bar test tag1 tag2 tag3 tag4 tag5) }
+        it 'link existing package and updates package file', :aggregate_failures do
+          expect(service).to receive(:try_obtain_lease).and_call_original
 
-      before do
-        allow_next_instance_of(Packages::Nuget::MetadataExtractionService) do |service|
-          allow(service)
-            .to receive(:nuspec_file_content).and_return(fixture_file(nuspec_filepath))
+          expect { subject }
+            .to change { ::Packages::Package.count }.by(-1)
+            .and change { Packages::Dependency.count }.by(0)
+            .and change { Packages::DependencyLink.count }.by(0)
+            .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0)
+            .and change { ::Packages::Nuget::Metadatum.count }.by(0)
+          expect(package_file.reload.file_name).to eq(package_file_name)
+          expect(package_file.package).to eq(existing_package)
         end
-      end
 
-      it 'creates tags' do
-        expect(service).to receive(:try_obtain_lease).and_call_original
-        expect { subject }.to change { ::Packages::Tag.count }.by(8)
-        expect(package.reload.tags.map(&:name)).to contain_exactly(*expected_tags)
+        it_behaves_like 'taking the lease'
+
+        it_behaves_like 'not updating the package if the lease is taken'
       end
 
-      context 'with existing package and tags' do
-        let!(:existing_package) { create(:nuget_package, project: package.project, name: 'DummyProject.WithMetadata', version: '1.2.3') }
-        let!(:tag1) { create(:packages_tag, package: existing_package, name: 'tag1') }
-        let!(:tag2) { create(:packages_tag, package: existing_package, name: 'tag2') }
-        let!(:tag3) { create(:packages_tag, package: existing_package, name: 'tag_not_in_metadata') }
+      context 'with a nuspec file with metadata' do
+        let(:nuspec_filepath) { 'packages/nuget/with_metadata.nuspec' }
+        let(:expected_tags) { %w(foo bar test tag1 tag2 tag3 tag4 tag5) }
+
+        before do
+          allow_next_instance_of(Packages::Nuget::MetadataExtractionService) do |service|
+            allow(service)
+              .to receive(:nuspec_file_content).and_return(fixture_file(nuspec_filepath))
+          end
+        end
 
-        it 'creates tags and deletes those not in metadata' do
+        it 'creates tags' do
           expect(service).to receive(:try_obtain_lease).and_call_original
-          expect { subject }.to change { ::Packages::Tag.count }.by(5)
-          expect(existing_package.tags.map(&:name)).to contain_exactly(*expected_tags)
+          expect { subject }.to change { ::Packages::Tag.count }.by(8)
+          expect(package.reload.tags.map(&:name)).to contain_exactly(*expected_tags)
         end
-      end
 
-      it 'creates nuget metadatum' do
-        expect { subject }
-          .to change { ::Packages::Package.count }.by(1)
-          .and change { ::Packages::Nuget::Metadatum.count }.by(1)
+        context 'with existing package and tags' do
+          let!(:existing_package) { create(:nuget_package, project: package.project, name: 'DummyProject.WithMetadata', version: '1.2.3') }
+          let!(:tag1) { create(:packages_tag, package: existing_package, name: 'tag1') }
+          let!(:tag2) { create(:packages_tag, package: existing_package, name: 'tag2') }
+          let!(:tag3) { create(:packages_tag, package: existing_package, name: 'tag_not_in_metadata') }
+
+          it 'creates tags and deletes those not in metadata' do
+            expect(service).to receive(:try_obtain_lease).and_call_original
+            expect { subject }.to change { ::Packages::Tag.count }.by(5)
+            expect(existing_package.tags.map(&:name)).to contain_exactly(*expected_tags)
+          end
+        end
 
-        metadatum = package_file.reload.package.nuget_metadatum
-        expect(metadatum.license_url).to eq('https://opensource.org/licenses/MIT')
-        expect(metadatum.project_url).to eq('https://gitlab.com/gitlab-org/gitlab')
-        expect(metadatum.icon_url).to eq('https://opensource.org/files/osi_keyhole_300X300_90ppi_0.png')
-      end
+        it 'creates nuget metadatum', :aggregate_failures do
+          expect { subject }
+            .to not_change { ::Packages::Package.count }
+            .and change { ::Packages::Nuget::Metadatum.count }.by(1)
 
-      context 'with too long url' do
-        let_it_be(:too_long_url) { "http://localhost/#{'bananas' * 50}" }
+          metadatum = package_file.reload.package.nuget_metadatum
+          expect(metadatum.license_url).to eq('https://opensource.org/licenses/MIT')
+          expect(metadatum.project_url).to eq('https://gitlab.com/gitlab-org/gitlab')
+          expect(metadatum.icon_url).to eq('https://opensource.org/files/osi_keyhole_300X300_90ppi_0.png')
+        end
 
-        let(:metadata) { { package_name: package_name, package_version: package_version, license_url: too_long_url } }
+        context 'with too long url' do
+          let_it_be(:too_long_url) { "http://localhost/#{'bananas' * 50}" }
 
-        before do
-          allow(service).to receive(:metadata).and_return(metadata)
-        end
+          let(:metadata) { { package_name: package_name, package_version: package_version, license_url: too_long_url } }
+
+          before do
+            allow(service).to receive(:metadata).and_return(metadata)
+          end
 
-        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+          it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+        end
       end
-    end
 
-    context 'with nuspec file with dependencies' do
-      let(:nuspec_filepath) { 'packages/nuget/with_dependencies.nuspec' }
-      let(:package_name) { 'Test.Package' }
-      let(:package_version) { '3.5.2' }
-      let(:package_file_name) { 'test.package.3.5.2.nupkg' }
+      context 'with nuspec file with dependencies' do
+        let(:nuspec_filepath) { 'packages/nuget/with_dependencies.nuspec' }
+        let(:package_name) { 'Test.Package' }
+        let(:package_version) { '3.5.2' }
+        let(:package_file_name) { 'test.package.3.5.2.nupkg' }
 
-      before do
-        allow_next_instance_of(Packages::Nuget::MetadataExtractionService) do |service|
-          allow(service)
-            .to receive(:nuspec_file_content).and_return(fixture_file(nuspec_filepath))
+        before do
+          allow_next_instance_of(Packages::Nuget::MetadataExtractionService) do |service|
+            allow(service)
+              .to receive(:nuspec_file_content).and_return(fixture_file(nuspec_filepath))
+          end
         end
-      end
 
-      it 'updates package and package file' do
-        expect { subject }
-          .to change { ::Packages::Package.count }.by(1)
-          .and change { Packages::Dependency.count }.by(4)
-          .and change { Packages::DependencyLink.count }.by(4)
-          .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(2)
-
-        expect(package.reload.name).to eq(package_name)
-        expect(package.version).to eq(package_version)
-        expect(package).to be_default
-        expect(package_file.reload.file_name).to eq(package_file_name)
-        # hard reset needed to properly reload package_file.file
-        expect(Packages::PackageFile.find(package_file.id).file.size).not_to eq 0
+        it 'updates package and package file', :aggregate_failures do
+          expect { subject }
+            .to not_change { ::Packages::Package.count }
+            .and change { Packages::Dependency.count }.by(4)
+            .and change { Packages::DependencyLink.count }.by(4)
+            .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(2)
+
+          expect(package.reload.name).to eq(package_name)
+          expect(package.version).to eq(package_version)
+          expect(package).to be_default
+          expect(package_file.reload.file_name).to eq(package_file_name)
+          # hard reset needed to properly reload package_file.file
+          expect(Packages::PackageFile.find(package_file.id).file.size).not_to eq 0
+        end
       end
-    end
 
-    context 'with package file not containing a nuspec file' do
-      before do
-        allow_next_instance_of(Zip::File) do |file|
-          allow(file).to receive(:glob).and_return([])
+      context 'with package file not containing a nuspec file' do
+        before do
+          allow_next_instance_of(Zip::File) do |file|
+            allow(file).to receive(:glob).and_return([])
+          end
         end
+
+        it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError
       end
 
-      it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError
-    end
+      context 'with a symbol package' do
+        let(:package_file) { package.package_files.last }
+        let(:package_file_name) { 'dummyproject.dummypackage.1.0.0.snupkg' }
 
-    context 'with a symbol package' do
-      let(:package_file) { package.package_files.last }
-      let(:package_file_name) { 'dummyproject.dummypackage.1.0.0.snupkg' }
+        context 'with no existing package' do
+          let(:package_id) { package.id }
 
-      context 'with no existing package' do
-        let(:package_id) { package.id }
+          it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+        end
 
-        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
-      end
+        context 'with existing package' do
+          let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) }
+          let(:package_id) { existing_package.id }
 
-      context 'with existing package' do
-        let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) }
-        let(:package_id) { existing_package.id }
+          it 'link existing package and updates package file', :aggregate_failures do
+            expect(service).to receive(:try_obtain_lease).and_call_original
+            expect(::Packages::Nuget::SyncMetadatumService).not_to receive(:new)
+            expect(::Packages::UpdateTagsService).not_to receive(:new)
 
-        it 'link existing package and updates package file', :aggregate_failures do
-          expect(service).to receive(:try_obtain_lease).and_call_original
-          expect(::Packages::Nuget::SyncMetadatumService).not_to receive(:new)
-          expect(::Packages::UpdateTagsService).not_to receive(:new)
-
-          expect { subject }
-            .to change { ::Packages::Package.count }.by(-1)
-            .and change { Packages::Dependency.count }.by(0)
-            .and change { Packages::DependencyLink.count }.by(0)
-            .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0)
-            .and change { ::Packages::Nuget::Metadatum.count }.by(0)
-          expect(package_file.reload.file_name).to eq(package_file_name)
-          expect(package_file.package).to eq(existing_package)
-        end
+            expect { subject }
+              .to change { ::Packages::Package.count }.by(-1)
+              .and change { Packages::Dependency.count }.by(0)
+              .and change { Packages::DependencyLink.count }.by(0)
+              .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0)
+              .and change { ::Packages::Nuget::Metadatum.count }.by(0)
+            expect(package_file.reload.file_name).to eq(package_file_name)
+            expect(package_file.package).to eq(existing_package)
+          end
 
-        it_behaves_like 'taking the lease'
+          it_behaves_like 'taking the lease'
 
-        it_behaves_like 'not updating the package if the lease is taken'
+          it_behaves_like 'not updating the package if the lease is taken'
+        end
       end
-    end
 
-    context 'with an invalid package name' do
-      invalid_names = [
-        '',
-        'My/package',
-        '../../../my_package',
-        '%2e%2e%2fmy_package'
-      ]
+      context 'with an invalid package name' do
+        invalid_names = [
+          '',
+          'My/package',
+          '../../../my_package',
+          '%2e%2e%2fmy_package'
+        ]
 
-      invalid_names.each do |invalid_name|
-        before do
-          allow(service).to receive(:package_name).and_return(invalid_name)
+        invalid_names.each do |invalid_name|
+          before do
+            allow(service).to receive(:package_name).and_return(invalid_name)
+          end
+
+          it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
         end
+      end
 
-        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+      context 'with an invalid package version' do
+        invalid_versions = [
+          '',
+          '555',
+          '1.2',
+          '1./2.3',
+          '../../../../../1.2.3',
+          '%2e%2e%2f1.2.3'
+        ]
+
+        invalid_versions.each do |invalid_version|
+          before do
+            allow(service).to receive(:package_version).and_return(invalid_version)
+          end
+
+          it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+        end
       end
     end
 
-    context 'with an invalid package version' do
-      invalid_versions = [
-        '',
-        '555',
-        '1.2',
-        '1./2.3',
-        '../../../../../1.2.3',
-        '%2e%2e%2f1.2.3'
-      ]
-
-      invalid_versions.each do |invalid_version|
-        before do
-          allow(service).to receive(:package_version).and_return(invalid_version)
-        end
+    context 'with packages_nuget_new_package_file_updater enabled' do
+      before do
+        expect(service).not_to receive(:legacy_execute)
+      end
+
+      it_behaves_like 'handling all conditions'
+    end
 
-        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+    context 'with packages_nuget_new_package_file_updater disabled' do
+      before do
+        stub_feature_flags(packages_nuget_new_package_file_updater: false)
+        expect(::Packages::UpdatePackageFileService)
+          .not_to receive(:new).with(package_file, instance_of(Hash)).and_call_original
+        expect(service).not_to receive(:new_execute)
       end
+
+      it_behaves_like 'handling all conditions'
     end
   end
 end

spec/services/packages/update_package_file_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Packages::UpdatePackageFileService do
+  let_it_be(:another_package) { create(:package) }
+  let_it_be(:old_file_name) { 'old_file_name.txt' }
+  let_it_be(:new_file_name) { 'new_file_name.txt' }
+
+  let(:package) { package_file.package }
+  let(:params) { { package_id: another_package.id, file_name: new_file_name } }
+  let(:service) { described_class.new(package_file, params) }
+
+  describe '#execute' do
+    subject { service.execute }
+
+    shared_examples 'updating package file with valid parameters' do
+      context 'with both parameters set' do
+        it 'updates the package file accordingly' do
+          expect { subject }
+            .to change { package.package_files.count }.from(1).to(0)
+            .and change { another_package.package_files.count }.from(0).to(1)
+            .and change { package_file.package_id }.from(package.id).to(another_package.id)
+            .and change { package_file.file_name }.from(old_file_name).to(new_file_name)
+        end
+      end
+
+      context 'with only file_name set' do
+        let(:params) { { file_name: new_file_name } }
+
+        it 'updates the package file accordingly' do
+          expect { subject }
+            .to not_change { package.package_files.count }
+            .and not_change { another_package.package_files.count }
+            .and not_change { package_file.package_id }
+            .and change { package_file.file_name }.from(old_file_name).to(new_file_name)
+        end
+      end
+
+      context 'with only package_id set' do
+        let(:params) { { package_id: another_package.id } }
+
+        it 'updates the package file accordingly' do
+          expect { subject }
+            .to change { package.package_files.count }.from(1).to(0)
+            .and change { another_package.package_files.count }.from(0).to(1)
+            .and change { package_file.package_id }.from(package.id).to(another_package.id)
+            .and not_change { package_file.file_name }
+        end
+      end
+    end
+
+    shared_examples 'not updating package with invalid parameters' do
+      context 'with blank parameters' do
+        let(:params) { {} }
+
+        it 'raise an argument error' do
+          expect { subject }.to raise_error(ArgumentError, 'package_id and file_name are blank')
+        end
+      end
+
+      context 'with non persisted package file' do
+        let(:package_file) { build(:package_file) }
+
+        it 'raise an argument error' do
+          expect { subject }.to raise_error(ArgumentError, 'package_file not persisted')
+        end
+      end
+    end
+
+    context 'with object storage disabled' do
+      let(:package_file) { create(:package_file, file_name: old_file_name) }
+
+      before do
+        stub_package_file_object_storage(enabled: false)
+      end
+
+      it_behaves_like 'updating package file with valid parameters' do
+        before do
+          expect(package_file).to receive(:remove_previously_stored_file).and_call_original
+          expect(package_file).not_to receive(:move_in_object_storage)
+        end
+      end
+
+      it_behaves_like 'not updating package with invalid parameters'
+    end
+
+    context 'with object storage enabled' do
+      let(:package_file) do
+        create(
+          :package_file,
+          file_name: old_file_name,
+          file: CarrierWaveStringFile.new_file(
+            file_content: 'content',
+            filename: old_file_name,
+            content_type: 'text/plain'
+          ),
+          file_store: ::Packages::PackageFileUploader::Store::REMOTE
+        )
+      end
+
+      before do
+        stub_package_file_object_storage(enabled: true)
+      end
+
+      it_behaves_like 'updating package file with valid parameters' do
+        before do
+          expect(package_file).not_to receive(:remove_previously_stored_file)
+          expect(package_file).to receive(:move_in_object_storage).and_call_original
+        end
+      end
+
+      it_behaves_like 'not updating package with invalid parameters' do
+        before do
+          expect(package_file.file.file).not_to receive(:copy_to)
+        end
+      end
+    end
+  end
+end