From a558e386749c579a70cca6463926092926627388 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 21 Jul 2023 00:08:18 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- .../concerns/packages/error_handling_spec.rb | 81 +++++++++ .../debian/process_package_file_worker_spec.rb | 196 ++++++++++++++++----- .../packages/helm/extraction_worker_spec.rb | 54 ++++-- .../packages/nuget/extraction_worker_spec.rb | 126 ++++++++----- .../packages/rubygems/extraction_worker_spec.rb | 80 ++++++--- 5 files changed, 412 insertions(+), 125 deletions(-) create mode 100644 spec/workers/concerns/packages/error_handling_spec.rb (limited to 'spec/workers') diff --git a/spec/workers/concerns/packages/error_handling_spec.rb b/spec/workers/concerns/packages/error_handling_spec.rb new file mode 100644 index 00000000000..3db32778a88 --- /dev/null +++ b/spec/workers/concerns/packages/error_handling_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Packages::ErrorHandling, feature_category: :build_artifacts do + let_it_be(:worker_class) do + Class.new do + def self.name + 'Gitlab::Foo::Bar::DummyWorker' + end + + include ApplicationWorker + include ::Packages::ErrorHandling + end + end + + let(:worker) { worker_class.new } + + describe '#process_package_file_error' do + let_it_be_with_reload(:package) { create(:generic_package, :processing, :with_zip_file) } + + let(:package_file) { package.package_files.first } + let(:package_name) { 'TempProject.TempPackage' } + let(:exception) { StandardError.new('42') } + let(:extra_log_payload) { { answer: 42 } } + let(:expected_log_payload) do + { + project_id: package_file.project_id, + package_file_id: package_file.id, + answer: 42 + } + end + + subject do + worker.process_package_file_error( + package_file: package_file, + exception: exception, + extra_log_payload: extra_log_payload + ) + end + + it 'logs the error with the correct parameters' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with(exception, expected_log_payload) + + subject + end + + shared_examples 'updates the package status and status message' do |error_message:| + it :aggregate_failures do + expect { subject } + .to change { package.status }.to('error') + .and change { package.status_message }.to(error_message) + end + end + + described_class::CONTROLLED_ERRORS.each do |exception_class| + context "with controlled exception #{exception_class}" do + let(:exception) { exception_class.new } + + it_behaves_like 'updates the package status and status message', error_message: exception_class.new.message + end + end + + context 'with all other errors' do + let(:exception) { StandardError.new('String that will not appear in status_message') } + + it_behaves_like 'updates the package status and status message', + error_message: 'Unexpected error: StandardError' + end + + context 'with a very long error message' do + let(:exception) { ArgumentError.new('a' * 1000) } + + it 'truncates the error message' do + subject + + expect(package.status_message.length).to eq(::Packages::Package::STATUS_MESSAGE_MAX_LENGTH) + end + end + end +end diff --git a/spec/workers/packages/debian/process_package_file_worker_spec.rb b/spec/workers/packages/debian/process_package_file_worker_spec.rb index 1ef3119ecd3..0c60633ef45 100644 --- a/spec/workers/packages/debian/process_package_file_worker_spec.rb +++ b/spec/workers/packages/debian/process_package_file_worker_spec.rb @@ -29,6 +29,17 @@ RSpec.describe Packages::Debian::ProcessPackageFileWorker, type: :worker, featur subject { worker.perform(package_file_id, distribution_name, component_name) } + shared_context 'with changes file' do + let(:package) { temp_with_changes } + let(:package_file) { package.package_files.first } + let(:distribution_name) { nil } + let(:component_name) { nil } + + before do + distribution.update! suite: 'unstable' + end + end + context 'with non existing package file' do let(:package_file_id) { non_existing_record_id } @@ -59,13 +70,12 @@ RSpec.describe Packages::Debian::ProcessPackageFileWorker, type: :worker, featur end end - context 'when the service raises an error' do - let(:package_file) { package.package_files.with_file_name('sample_1.2.3~alpha2.tar.xz').first } - + shared_examples 'handling error' do |error_message:, error_class:| it 'marks the package as errored', :aggregate_failures do expect(Gitlab::ErrorTracking).to receive(:log_exception).with( - instance_of(ArgumentError), + instance_of(error_class), package_file_id: package_file_id, + project_id: package.project_id, distribution_name: distribution_name, component_name: component_name ) @@ -75,21 +85,128 @@ RSpec.describe Packages::Debian::ProcessPackageFileWorker, type: :worker, featur .and not_change { package.package_files.count } .and change { package_file.reload.status }.to('error') .and change { package.reload.status }.from('processing').to('error') + .and change { package.status_message }.to(error_message) end end - context 'with a Debian changes file' do - let(:package) { temp_with_changes } - let(:package_file) { package.package_files.first } - let(:distribution_name) { nil } - let(:component_name) { nil } + context 'with controlled errors' do + context 'with a package file' do + context 'when component name is blank' do + let(:component_name) { '' } + + it_behaves_like 'handling error', + error_message: 'missing component name', + error_class: ArgumentError + end + + context 'when distribution name is blank' do + let(:distribution_name) { '' } + + it_behaves_like 'handling error', + error_message: 'missing distribution name', + error_class: ArgumentError + end + + context 'when package file is not deb, ddeb or udeb' do + let(:package_file) { package.package_files.with_file_name('sample_1.2.3~alpha2.tar.xz').first } + + it_behaves_like 'handling error', + error_message: 'invalid package file type: source', + error_class: ArgumentError + end + end + + context 'with changes file' do + include_context 'with changes file' + + let(:file_metadata_source) { 'src' } + let(:file_metadata_version) { '0.1' } + let(:file_metadata_distribution) { 'Breezy Badger' } + let(:file_metadata) do + { + fields: { + 'Source' => file_metadata_source, + 'Version' => file_metadata_version, + 'Distribution' => file_metadata_distribution + } + } + end + + context 'when component name is blank' do + let(:component_name) { '' } + + it_behaves_like 'handling error', + error_message: 'unwanted component name', + error_class: ArgumentError + end + + context 'with distribution name is blank' do + let(:distribution_name) { '' } + + it_behaves_like 'handling error', + error_message: 'unwanted distribution name', + error_class: ArgumentError + end + context 'with missing file metadata fields' do + before do + allow_next_instance_of(::Packages::Debian::ExtractChangesMetadataService) do |instance| + allow(instance).to receive(:execute).and_return(file_metadata) + end + end + + context 'when source field is missing' do + before do + file_metadata[:fields].delete('Source') + end + + it_behaves_like 'handling error', + error_message: 'missing Source field', + error_class: ArgumentError + end + + context 'when Version field is missing' do + before do + file_metadata[:fields].delete('Version') + end + + it_behaves_like 'handling error', + error_message: 'missing Version field', + error_class: ArgumentError + end + + context 'when Distribution field is missing' do + before do + file_metadata[:fields].delete('Distribution') + end + + it_behaves_like 'handling error', + error_message: 'missing Distribution field', + error_class: ArgumentError + end + end + end + end + + context 'with uncontrolled errors' do before do - distribution.update! suite: 'unstable' + allow_next_instance_of(::Packages::Debian::ProcessPackageFileService) do |instance| + allow(instance).to receive(:execute).and_raise(StandardError.new('Boom')) + end end - it_behaves_like 'an idempotent worker' do - let(:job_args) { [package_file.id, distribution_name, component_name] } + it_behaves_like 'handling error', + error_message: 'Unexpected error: StandardError', + error_class: StandardError + end + + context 'with correct job arguments' do + let(:job_args) { [package_file.id, distribution_name, component_name] } + + it_behaves_like 'an idempotent worker' + + context 'with a Debian changes file' do + include_context 'with changes file' it 'sets the Debian file type to changes', :aggregate_failures do expect(::Packages::Debian::GenerateDistributionWorker) @@ -110,39 +227,38 @@ RSpec.describe Packages::Debian::ProcessPackageFileWorker, type: :worker, featur .and not_change { debian_file_metadatum.component } end end - end - - using RSpec::Parameterized::TableSyntax - where(:case_name, :expected_file_type, :file_name, :component_name) do - 'with a deb' | 'deb' | 'libsample0_1.2.3~alpha2_amd64.deb' | 'main' - 'with an udeb' | 'udeb' | 'sample-udeb_1.2.3~alpha2_amd64.udeb' | 'contrib' - 'with a ddeb' | 'ddeb' | 'sample-ddeb_1.2.3~alpha2_amd64.ddeb' | 'main' - end + context 'with Debian files' do + using RSpec::Parameterized::TableSyntax - with_them do - let(:package_file) { package.package_files.with_file_name(file_name).first } + where(:case_name, :expected_file_type, :file_name, :component_name) do + 'with a deb' | 'deb' | 'libsample0_1.2.3~alpha2_amd64.deb' | 'main' + 'with an udeb' | 'udeb' | 'sample-udeb_1.2.3~alpha2_amd64.udeb' | 'contrib' + 'with a ddeb' | 'ddeb' | 'sample-ddeb_1.2.3~alpha2_amd64.ddeb' | 'main' + end - it_behaves_like 'an idempotent worker' do - let(:job_args) { [package_file.id, distribution_name, component_name] } + with_them do + let(:package_file) { package.package_files.with_file_name(file_name).first } + let(:job_args) { [package_file.id, distribution_name, component_name] } - it 'sets the correct Debian file type', :aggregate_failures do - expect(::Packages::Debian::GenerateDistributionWorker) - .to receive(:perform_async).with(:project, distribution.id) - expect(Gitlab::ErrorTracking).not_to receive(:log_exception) + it 'sets the correct Debian file type', :aggregate_failures do + expect(::Packages::Debian::GenerateDistributionWorker) + .to receive(:perform_async).with(:project, distribution.id) + expect(Gitlab::ErrorTracking).not_to receive(:log_exception) - # Using subject inside this block will process the job multiple times - expect { subject } - .to not_change(Packages::Package, :count) - .and not_change(Packages::PackageFile, :count) - .and change { Packages::Debian::Publication.count }.by(1) - .and not_change(package.package_files, :count) - .and change { package.reload.name }.to('sample') - .and change { package.version }.to('1.2.3~alpha2') - .and change { package.status }.from('processing').to('default') - .and change { package.debian_publication }.from(nil) - .and change { debian_file_metadatum.reload.file_type }.from('unknown').to(expected_file_type) - .and change { debian_file_metadatum.component }.from(nil).to(component_name) + # Using subject inside this block will process the job multiple times + expect { subject } + .to not_change(Packages::Package, :count) + .and not_change(Packages::PackageFile, :count) + .and change { Packages::Debian::Publication.count }.by(1) + .and not_change(package.package_files, :count) + .and change { package.reload.name }.to('sample') + .and change { package.version }.to('1.2.3~alpha2') + .and change { package.status }.from('processing').to('default') + .and change { package.debian_publication }.from(nil) + .and change { debian_file_metadatum.reload.file_type }.from('unknown').to(expected_file_type) + .and change { debian_file_metadatum.component }.from(nil).to(component_name) + end end end end diff --git a/spec/workers/packages/helm/extraction_worker_spec.rb b/spec/workers/packages/helm/extraction_worker_spec.rb index a764c2ad939..d6c1bdfcd6a 100644 --- a/spec/workers/packages/helm/extraction_worker_spec.rb +++ b/spec/workers/packages/helm/extraction_worker_spec.rb @@ -23,16 +23,22 @@ RSpec.describe Packages::Helm::ExtractionWorker, type: :worker, feature_category subject { described_class.new.perform(channel, package_file_id) } - shared_examples 'handling error' do |error_class = Packages::Helm::ExtractFileMetadataService::ExtractionError| + shared_examples 'handling error' do |error_message:, + error_class: Packages::Helm::ExtractFileMetadataService::ExtractionError| it 'mark the package as errored', :aggregate_failures do expect(Gitlab::ErrorTracking).to receive(:log_exception).with( instance_of(error_class), - project_id: package_file.package.project_id + { + package_file_id: package_file.id, + project_id: package_file.project_id + } ) expect { subject } .to not_change { Packages::Package.count } .and not_change { Packages::PackageFile.count } .and change { package.reload.status }.from('processing').to('error') + + expect(package.status_message).to match(error_message) end end @@ -69,34 +75,46 @@ RSpec.describe Packages::Helm::ExtractionWorker, type: :worker, feature_category end end - context 'with an empty package file' do - before do - expect_next_instance_of(Gem::Package::TarReader) do |tar_reader| - expect(tar_reader).to receive(:each).and_return([]) + context 'with controlled errors' do + context 'with an empty package file' do + before do + expect_next_instance_of(Gem::Package::TarReader) do |tar_reader| + expect(tar_reader).to receive(:each).and_return([]) + end end - end - it_behaves_like 'handling error' - end + it_behaves_like 'handling error', error_message: /Chart.yaml not found/ + end - context 'with an invalid YAML' do - before do - expect_next_instance_of(Gem::Package::TarReader::Entry) do |entry| - expect(entry).to receive(:read).and_return('{') + context 'with an invalid YAML' do + before do + expect_next_instance_of(Gem::Package::TarReader::Entry) do |entry| + expect(entry).to receive(:read).and_return('{') + end end + + it_behaves_like 'handling error', error_message: /Error while parsing Chart.yaml/ end - it_behaves_like 'handling error' + context 'with an invalid Chart.yaml' do + before do + expect_next_instance_of(Gem::Package::TarReader::Entry) do |entry| + expect(entry).to receive(:read).and_return('{}') + end + end + + it_behaves_like 'handling error', error_class: ActiveRecord::RecordInvalid, error_message: /Validation failed/ + end end - context 'with an invalid Chart.yaml' do + context 'with uncontrolled errors' do before do - expect_next_instance_of(Gem::Package::TarReader::Entry) do |entry| - expect(entry).to receive(:read).and_return('{}') + allow_next_instance_of(::Packages::Helm::ProcessFileService) do |instance| + allow(instance).to receive(:execute).and_raise(StandardError.new('Boom')) end end - it_behaves_like 'handling error', ActiveRecord::RecordInvalid + it_behaves_like 'handling error', error_class: StandardError, error_message: 'Unexpected error: StandardError' end end end diff --git a/spec/workers/packages/nuget/extraction_worker_spec.rb b/spec/workers/packages/nuget/extraction_worker_spec.rb index 11eaa1b5dde..d261002a339 100644 --- a/spec/workers/packages/nuget/extraction_worker_spec.rb +++ b/spec/workers/packages/nuget/extraction_worker_spec.rb @@ -13,16 +13,21 @@ RSpec.describe Packages::Nuget::ExtractionWorker, type: :worker, feature_categor subject { described_class.new.perform(package_file_id) } - shared_examples 'handling the metadata error' do |exception_class: ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError| + shared_examples 'handling error' do |error_message:, + error_class: ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError| it 'updates package status to error', :aggregate_failures do expect(Gitlab::ErrorTracking).to receive(:log_exception).with( - instance_of(exception_class), - project_id: package.project_id + instance_of(error_class), + { + package_file_id: package_file.id, + project_id: package.project_id + } ) subject expect(package.reload).to be_error + expect(package.status_message).to match(error_message) end end @@ -56,64 +61,101 @@ RSpec.describe Packages::Nuget::ExtractionWorker, type: :worker, feature_categor end end - context 'with package file not containing a nuspec file' do - before do - allow_any_instance_of(Zip::File).to receive(:glob).and_return([]) - end - - it_behaves_like 'handling the metadata error', exception_class: ::Packages::Nuget::ExtractMetadataFileService::ExtractionError - end + context 'with controlled errors' do + context 'with package file not containing a nuspec file' do + before do + allow_any_instance_of(Zip::File).to receive(:glob).and_return([]) + end - context 'with package with an invalid package name' do - invalid_names = [ - '', - 'My/package', - '../../../my_package', - '%2e%2e%2fmy_package' - ] + it_behaves_like 'handling error', + error_class: ::Packages::Nuget::ExtractMetadataFileService::ExtractionError, + error_message: 'nuspec file not found' + end - invalid_names.each do |invalid_name| - context "with #{invalid_name}" do + context 'with invalid metadata' do + shared_context 'with a blank attribute' do before do allow_next_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService) do |service| - allow(service).to receive(:package_name).and_return(invalid_name) + allow(service).to receive(attribute).and_return('') end end + end + + context 'with a blank package name' do + include_context 'with a blank attribute' do + let(:attribute) { :package_name } - it_behaves_like 'handling the metadata error' + it_behaves_like 'handling error', error_message: /not found in metadata/ + end end - end - end - context 'with package with an invalid package version' do - invalid_versions = [ - '', - '555', - '1./2.3', - '../../../../../1.2.3', - '%2e%2e%2f1.2.3' - ] - - invalid_versions.each do |invalid_version| - context "with #{invalid_version}" do - before do - allow_next_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService) do |service| - allow(service).to receive(:package_version).and_return(invalid_version) + context 'with package with an invalid package name' do + invalid_names = [ + 'My/package', + '../../../my_package', + '%2e%2e%2fmy_package' + ] + + invalid_names.each do |invalid_name| + context "with #{invalid_name}" do + before do + allow_next_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService) do |service| + allow(service).to receive(:package_name).and_return(invalid_name) + end + end + + it_behaves_like 'handling error', error_message: 'Validation failed: Name is invalid' + end + end + end + + context 'with package with a blank package version' do + include_context 'with a blank attribute' do + let(:attribute) { :package_version } + + it_behaves_like 'handling error', error_message: /not found in metadata/ + end + end + + context 'with package with an invalid package version' do + invalid_versions = [ + '555', + '1./2.3', + '../../../../../1.2.3', + '%2e%2e%2f1.2.3' + ] + + invalid_versions.each do |invalid_version| + context "with #{invalid_version}" do + before do + allow_next_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService) do |service| + allow(service).to receive(:package_version).and_return(invalid_version) + end + end + + it_behaves_like 'handling error', error_message: 'Validation failed: Version is invalid' end end + end + end - it_behaves_like 'handling the metadata error' + context 'handling a Zip::Error exception' do + before do + allow_any_instance_of(::Packages::UpdatePackageFileService).to receive(:execute).and_raise(::Zip::Error) end + + it_behaves_like 'handling error', + error_class: ::Packages::Nuget::UpdatePackageFromMetadataService::ZipError, + error_message: 'Could not open the .nupkg file' end end - context 'handles a processing an unaccounted for error' do + context 'with uncontrolled errors' do before do - expect(::Packages::Nuget::UpdatePackageFromMetadataService).to receive(:new) - .and_raise(Zip::Error) + allow_any_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService).to receive(:execute).and_raise(StandardError.new('Boom')) end - it_behaves_like 'handling the metadata error', exception_class: Zip::Error + it_behaves_like 'handling error', error_class: StandardError, error_message: 'Unexpected error: StandardError' end end end diff --git a/spec/workers/packages/rubygems/extraction_worker_spec.rb b/spec/workers/packages/rubygems/extraction_worker_spec.rb index 8ad4c2e6447..4ae8f729117 100644 --- a/spec/workers/packages/rubygems/extraction_worker_spec.rb +++ b/spec/workers/packages/rubygems/extraction_worker_spec.rb @@ -14,41 +14,71 @@ RSpec.describe Packages::Rubygems::ExtractionWorker, type: :worker, feature_cate subject { described_class.new.perform(*job_args) } - it 'processes the gem', :aggregate_failures do - expect { subject } - .to change { Packages::Package.count }.by(0) - .and change { Packages::PackageFile.count }.by(1) + context 'without errors' do + let_it_be(:package_for_processing) { create(:rubygems_package, :processing) } + let(:package_file) { package_for_processing.package_files.first } - expect(Packages::Package.last.id).to be(package.id) - expect(package.name).not_to be(package_name) - end + it 'processes the gem', :aggregate_failures do + expect { subject } + .to change { Packages::Package.count }.by(0) + .and change { Packages::PackageFile.count }.by(1) - it 'handles a processing failure', :aggregate_failures do - expect(::Packages::Rubygems::ProcessGemService).to receive(:new) - .and_raise(::Packages::Rubygems::ProcessGemService::ExtractionError) + expect(Packages::Package.last.id).to be(package_for_processing.id) + expect(package_for_processing.name).not_to be(package_name) + end + end - expect(Gitlab::ErrorTracking).to receive(:log_exception).with( - instance_of(::Packages::Rubygems::ProcessGemService::ExtractionError), - project_id: package.project_id - ) + shared_examples 'handling error' do |error_message:, error_class:| + it 'mark the package as errored', :aggregate_failures do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + instance_of(error_class), + { + package_file_id: package_file.id, + project_id: package.project_id + } + ) - subject + expect { subject } + .to not_change { Packages::Package.count } + .and not_change { Packages::PackageFile.count } + .and change { package.reload.status }.from('processing').to('error') - expect(package.reload).to be_error + expect(package.status_message).to match(error_message) + end end - it 'handles processing an unaccounted for error', :aggregate_failures do - expect(::Packages::Rubygems::ProcessGemService).to receive(:new) - .and_raise(Zip::Error) + context 'with controlled errors' do + context 'handling metadata with invalid size' do + include_context 'with invalid Rubygems metadata' + + it_behaves_like 'handling error', + error_class: ::Packages::Rubygems::ProcessGemService::InvalidMetadataError, + error_message: 'Invalid metadata' + end - expect(Gitlab::ErrorTracking).to receive(:log_exception).with( - instance_of(Zip::Error), - project_id: package.project_id - ) + context 'handling a file error' do + before do + package_file.file = nil + end - subject + it_behaves_like 'handling error', + error_class: ::Packages::Rubygems::ProcessGemService::ExtractionError, + error_message: 'Unable to read gem file' + end + end - expect(package.reload).to be_error + context 'with uncontrolled errors' do + [Zip::Error, StandardError].each do |exception| + context "handling #{exception}", :aggregate_failures do + before do + allow(::Packages::Rubygems::ProcessGemService).to receive(:new).and_raise(exception) + end + + it_behaves_like 'handling error', + error_class: exception, + error_message: "Unexpected error: #{exception}" + end + end end context 'returns when there is no package file' do -- cgit v1.2.3