diff --git a/app/services/packages/nuget/update_package_from_metadata_service.rb b/app/services/packages/nuget/update_package_from_metadata_service.rb
index 6ffe4f097f4897eda9c4e5c99570b922fb8811d1..d1e47ad00a11282c7704176fe23ceb682b82f8c1 100644
--- a/app/services/packages/nuget/update_package_from_metadata_service.rb
+++ b/app/services/packages/nuget/update_package_from_metadata_service.rb
@@ -21,11 +21,7 @@ def execute
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 @@ def execute
private
- def new_execute
+ def process_package_update
package_to_destroy = nil
target_package = @package_file.package
@@ -50,36 +46,11 @@ def new_execute
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?
diff --git a/config/feature_flags/development/packages_nuget_new_package_file_updater.yml b/config/feature_flags/development/packages_nuget_new_package_file_updater.yml
deleted file mode 100644
index b1f8a79da9a1f560b97b079042ac1f0217cd7526..0000000000000000000000000000000000000000
--- a/config/feature_flags/development/packages_nuget_new_package_file_updater.yml
+++ /dev/null
@@ -1,8 +0,0 @@
----
-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
diff --git a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb
index 66ff6a8d03fc980e5e5732078754bd21ca894453..d682ee12ed586a34f0d06d173043502641d0c31c 100644
--- a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb
+++ b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb
@@ -5,7 +5,7 @@
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 @@
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