Merge branch '336511-feature-flag-cleanup' into 'master'

Remove the feature flag packages_nuget_new_package_file_updater

See merge request gitlab-org/gitlab!69755

app/services/packages/nuget/update_package_from_metadata_service.rb

@@ -21,11 +21,7 @@ module Packages
 
         try_obtain_lease do
           @package_file.transaction do
-            if use_new_package_file_updater?
-              new_execute
-            else
-              legacy_execute
-            end
+            process_package_update
           end
         end
       rescue ActiveRecord::RecordInvalid => e
@@ -34,7 +30,7 @@ module Packages
 
       private
 
-      def new_execute
+      def process_package_update
         package_to_destroy = nil
         target_package = @package_file.package
 
@@ -50,36 +46,11 @@ module Packages
         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?
 

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.2'
-type: development
-group: group::package
-default_enabled: true

spec/services/packages/nuget/update_package_from_metadata_service_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_redis_shared_state do
   include ExclusiveLeaseHelpers
 
-  let(:package) { create(:nuget_package, :processing, :with_symbol_package) }
+  let!(:package) { create(:nuget_package, :processing, :with_symbol_package) }
   let(:package_file) { package.package_files.first }
   let(:service) { described_class.new(package_file) }
   let(:package_name) { 'DummyProject.DummyPackage' }
@@ -63,234 +63,213 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_
       end
     end
 
-    shared_examples 'handling all conditions' do
-      context 'with no existing package' do
-        let(:package_id) { package.id }
+    context 'with no existing package' do
+      let(:package_id) { package.id }
+
+      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)
+
+        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 '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)
+      it_behaves_like 'taking the lease'
 
-          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 'not updating the package if the lease is taken'
+    end
 
-        it_behaves_like 'taking the lease'
+    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_behaves_like 'not updating the package if the lease is taken'
-      end
+      it 'link existing package and updates package file', :aggregate_failures do
+        expect(service).to receive(:try_obtain_lease).and_call_original
 
-      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 { 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 'link existing package and updates package file', :aggregate_failures 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)
-        end
+      it_behaves_like 'not updating the package if the lease is taken'
+    end
 
-        it_behaves_like 'taking the lease'
+    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_behaves_like 'not updating the package if the lease is taken'
+      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
 
-      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 '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)
+      end
 
-        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
+      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' do
+        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(8)
-          expect(package.reload.tags.map(&:name)).to contain_exactly(*expected_tags)
+          expect { subject }.to change { ::Packages::Tag.count }.by(5)
+          expect(existing_package.tags.map(&:name)).to contain_exactly(*expected_tags)
         end
+      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') }
-
-          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
-
-        it 'creates nuget metadatum', :aggregate_failures do
-          expect { subject }
-            .to not_change { ::Packages::Package.count }
-            .and change { ::Packages::Nuget::Metadatum.count }.by(1)
-
-          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 } }
 
-          it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+        before do
+          allow(service).to receive(:metadata).and_return(metadata)
         end
+
+        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
       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))
-          end
+      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 '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
+      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
 
-      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
+    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
-
-        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' }
+      it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError
+    end
 
-        context 'with no existing package' do
-          let(:package_id) { package.id }
+    context 'with a symbol package' do
+      let(:package_file) { package.package_files.last }
+      let(:package_file_name) { 'dummyproject.dummypackage.1.0.0.snupkg' }
 
-          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 no existing package' do
+        let(:package_id) { 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_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+      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
+      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_behaves_like 'taking the lease'
+        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_behaves_like 'not updating the package if the lease is taken'
+          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
-
-      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)
-          end
+        it_behaves_like 'taking the lease'
 
-          it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
-        end
+        it_behaves_like 'not updating the package if the lease is taken'
       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
-
-          it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+    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)
         end
-      end
-    end
 
-    context 'with packages_nuget_new_package_file_updater enabled' do
-      before do
-        expect(service).not_to receive(:legacy_execute)
+        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
       end
-
-      it_behaves_like 'handling all conditions'
     end
 
-    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
+    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 'handling all conditions'
+        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
+      end
     end
   end
 end