Merge branch '324206-error-packages' into 'master'

Set error status on failed package extraction

See merge request gitlab-org/gitlab!60172

app/presenters/packages/detail/package_presenter.rb

@@ -17,6 +17,7 @@ module Packages
           name: name,
           package_files: @package.package_files.map { |pf| build_package_file_view(pf) },
           package_type: @package.package_type,
+          status: @package.status,
           project_id: @package.project_id,
           tags: @package.tags.as_json,
           updated_at: @package.updated_at,

app/services/packages/rubygems/process_gem_service.rb

@@ -16,6 +16,7 @@ module Packages
       end
 
       def execute
+        raise ExtractionError, 'Gem was not processed - package_file is not set' unless package_file
         return success if process_gem
 
         error('Gem was not processed')
@@ -26,8 +27,6 @@ module Packages
       attr_reader :package_file
 
       def process_gem
-        return false unless package_file
-
         try_obtain_lease do
           package.transaction do
             rename_package_and_set_version

app/workers/all_queues.yml

@@ -1354,7 +1354,7 @@
   :urgency: :low
   :resource_boundary: :unknown
   :weight: 1
-  :idempotent: true
+  :idempotent:
   :tags:
   - :exclude_from_kubernetes
 - :name: pipeline_background:archive_trace

app/workers/packages/nuget/extraction_worker.rb

@@ -20,7 +20,7 @@ module Packages
       rescue ::Packages::Nuget::MetadataExtractionService::ExtractionError,
              ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError => e
         Gitlab::ErrorTracking.log_exception(e, project_id: package_file.project_id)
-        package_file.package.destroy!
+        package_file.package.update_column(:status, :error)
       end
     end
   end

app/workers/packages/rubygems/extraction_worker.rb

@@ -12,8 +12,6 @@ module Packages
       tags :exclude_from_kubernetes
       deduplicate :until_executing
 
-      idempotent!
-
       def perform(package_file_id)
         package_file = ::Packages::PackageFile.find_by_id(package_file_id)
 
@@ -23,7 +21,7 @@ module Packages
 
       rescue ::Packages::Rubygems::ProcessGemService::ExtractionError => e
         Gitlab::ErrorTracking.log_exception(e, project_id: package_file.project_id)
-        package_file.package.destroy!
+        package_file.package.update_column(:status, :error)
       end
     end
   end

changelogs/unreleased/324206-error-packages.yml

+---
+title: Update RubyGems and NuGet packages to error status upon metadata extraction
+  failure
+merge_request: 60172
+author:
+type: changed

lib/api/entities/package.rb

@@ -22,6 +22,7 @@ module API
 
       expose :version
       expose :package_type
+      expose :status
 
       expose :_links do
         expose :web_path do |package|

spec/fixtures/api/schemas/public_api/v4/packages/package.json

@@ -4,6 +4,7 @@
     "name",
     "version",
     "package_type",
+    "status",
     "_links",
     "versions"
   ],
@@ -20,6 +21,9 @@
     "package_type": {
       "type": "string"
     },
+    "status": {
+      "type": "string"
+    },
     "_links": {
       "type": "object",
       "required": [

spec/presenters/packages/detail/package_presenter_spec.rb

@@ -50,6 +50,7 @@ RSpec.describe ::Packages::Detail::PackagePresenter do
       name: package.name,
       package_files: expected_package_files,
       package_type: package.package_type,
+      status: package.status,
       project_id: package.project_id,
       tags: package.tags.as_json,
       updated_at: package.updated_at,

spec/services/packages/rubygems/process_gem_service_spec.rb

@@ -16,12 +16,11 @@ RSpec.describe Packages::Rubygems::ProcessGemService do
   describe '#execute' do
     subject { service.execute }
 
-    context 'no gem file', :aggregate_failures do
+    context 'no gem file' do
       let(:package_file) { nil }
 
       it 'returns an error' do
-        expect(subject.error?).to be(true)
-        expect(subject.message).to eq('Gem was not processed')
+        expect { subject }.to raise_error(::Packages::Rubygems::ProcessGemService::ExtractionError, 'Gem was not processed - package_file is not set')
       end
     end
 

spec/workers/packages/nuget/extraction_worker_spec.rb

@@ -14,14 +14,15 @@ RSpec.describe Packages::Nuget::ExtractionWorker, type: :worker do
     subject { described_class.new.perform(package_file_id) }
 
     shared_examples 'handling the metadata error' do |exception_class: ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError|
-      it 'removes the package and the package file' do
+      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
         )
-        expect { subject }
-          .to change { Packages::Package.count }.by(-1)
-          .and change { Packages::PackageFile.count }.by(-1)
+
+        subject
+
+        expect(package.reload).to be_error
       end
     end
 

spec/workers/packages/rubygems/extraction_worker_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Packages::Rubygems::ExtractionWorker, type: :worker do
   describe '#perform' do
-    let_it_be(:package) { create(:rubygems_package) }
+    let_it_be(:package) { create(:rubygems_package, :processing) }
 
     let(:package_file) { package.package_files.first }
     let(:package_file_id) { package_file.id }
@@ -14,15 +14,13 @@ RSpec.describe Packages::Rubygems::ExtractionWorker, type: :worker do
 
     subject { described_class.new.perform(*job_args) }
 
-    include_examples 'an idempotent worker' do
-      it 'processes the gem', :aggregate_failures do
-        expect { subject }
-          .to change { Packages::Package.count }.by(0)
-          .and change { Packages::PackageFile.count }.by(2)
+    it 'processes the gem', :aggregate_failures do
+      expect { subject }
+        .to change { Packages::Package.count }.by(0)
+        .and change { Packages::PackageFile.count }.by(1)
 
-        expect(Packages::Package.last.id).to be(package.id)
-        expect(package.name).not_to be(package_name)
-      end
+      expect(Packages::Package.last.id).to be(package.id)
+      expect(package.name).not_to be(package_name)
     end
 
     it 'handles a processing failure', :aggregate_failures do
@@ -34,9 +32,9 @@ RSpec.describe Packages::Rubygems::ExtractionWorker, type: :worker do
         project_id: package.project_id
       )
 
-      expect { subject }
-        .to change { Packages::Package.count }.by(-1)
-        .and change { Packages::PackageFile.count }.by(-2)
+      subject
+
+      expect(package.reload).to be_error
     end
 
     context 'returns when there is no package file' do