Merge branch '338954_remove_subtransactions_for_build_infos' into 'master'

Package build infos: remove safe_find_or_create_by! calls

See merge request gitlab-org/gitlab!68649

app/models/packages/package.rb

@@ -293,6 +293,13 @@ class Packages::Package < ApplicationRecord
     ::Packages::Maven::Metadata::SyncWorker.perform_async(user.id, project.id, name)
   end
 
+  def create_build_infos!(build)
+    return unless build&.pipeline
+
+    # TODO: use an upsert call when https://gitlab.com/gitlab-org/gitlab/-/issues/339093 is implemented
+    build_infos.find_or_create_by!(pipeline: build.pipeline)
+  end
+
   private
 
   def composer_tag_version?

app/services/packages/generic/create_package_file_service.rb

@@ -29,7 +29,8 @@ module Packages
 
         package.update_column(:status, params[:status]) if params[:status] && params[:status] != package.status
 
-        package.build_infos.safe_find_or_create_by!(pipeline: params[:build].pipeline) if params[:build].present?
+        package.create_build_infos!(params[:build])
+
         package
       end
 

app/services/packages/maven/find_or_create_package_service.rb

@@ -51,7 +51,7 @@ module Packages
                                                    .execute
         end
 
-        package.build_infos.safe_find_or_create_by!(pipeline: params[:build].pipeline) if params[:build].present?
+        package.create_build_infos!(params[:build])
 
         ServiceResponse.success(payload: { package: package })
       end

spec/factories/packages.rb

@@ -354,4 +354,12 @@ FactoryBot.define do
     package
     sequence(:name) { |n| "tag-#{n}"}
   end
+
+  factory :packages_build_info, class: 'Packages::BuildInfo' do
+    package
+
+    trait :with_pipeline do
+      association :pipeline, factory: [:ci_pipeline, :with_job]
+    end
+  end
 end

spec/models/packages/package_spec.rb

@@ -1165,4 +1165,47 @@ RSpec.describe Packages::Package, type: :model do
       it_behaves_like 'not enqueuing a sync worker job'
     end
   end
+
+  describe '#create_build_infos!' do
+    let_it_be(:package) { create(:package) }
+    let_it_be(:pipeline) { create(:ci_pipeline) }
+
+    let(:build) { double(pipeline: pipeline) }
+
+    subject { package.create_build_infos!(build) }
+
+    context 'with a valid build' do
+      it 'creates a build info' do
+        expect { subject }.to change { ::Packages::BuildInfo.count }.by(1)
+
+        last_build = ::Packages::BuildInfo.last
+        expect(last_build.package).to eq(package)
+        expect(last_build.pipeline).to eq(pipeline)
+      end
+
+      context 'with an already existing build info' do
+        let_it_be(:build_info) { create(:packages_build_info, package: package, pipeline: pipeline) }
+
+        it 'does not create a build info' do
+          expect { subject }.not_to change { ::Packages::BuildInfo.count }
+        end
+      end
+    end
+
+    context 'with a nil build' do
+      let(:build) { nil }
+
+      it 'does not create a build info' do
+        expect { subject }.not_to change { ::Packages::BuildInfo.count }
+      end
+    end
+
+    context 'with a build without a pipeline' do
+      let(:build) { double(pipeline: nil) }
+
+      it 'does not create a build info' do
+        expect { subject }.not_to change { ::Packages::BuildInfo.count }
+      end
+    end
+  end
 end

spec/services/packages/generic/create_package_file_service_spec.rb

@@ -105,6 +105,37 @@ RSpec.describe Packages::Generic::CreatePackageFileService do
           it { expect { execute_service }.to change { project.package_files.count }.by(1) }
         end
       end
+
+      context 'with multiple files for the same package and the same pipeline' do
+        let(:file_2_params) { params.merge(file_name:  'myfile.tar.gz.2', file: file2) }
+        let(:file_3_params) { params.merge(file_name:  'myfile.tar.gz.3', file: file3) }
+
+        let(:temp_file2) { Tempfile.new("test2") }
+        let(:temp_file3) { Tempfile.new("test3") }
+
+        let(:file2) { UploadedFile.new(temp_file2.path, sha256: sha256) }
+        let(:file3) { UploadedFile.new(temp_file3.path, sha256: sha256) }
+
+        before do
+          FileUtils.touch(temp_file2)
+          FileUtils.touch(temp_file3)
+          expect(::Packages::Generic::FindOrCreatePackageService).to receive(:new).with(project, user, package_params).and_return(package_service).twice
+          expect(package_service).to receive(:execute).and_return(package).twice
+        end
+
+        after do
+          FileUtils.rm_f(temp_file2)
+          FileUtils.rm_f(temp_file3)
+        end
+
+        it 'creates the build info only once' do
+          expect do
+            described_class.new(project, user, params).execute
+            described_class.new(project, user, file_2_params).execute
+            described_class.new(project, user, file_3_params).execute
+          end.to change { package.build_infos.count }.by(1)
+        end
+      end
     end
   end
 end

spec/services/packages/maven/find_or_create_package_service_spec.rb

@@ -98,6 +98,19 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do
       it 'creates a build_info' do
         expect { subject }.to change { Packages::BuildInfo.count }.by(1)
       end
+
+      context 'with multiple files for the same package and the same pipeline' do
+        let(:file_2_params) { params.merge(file_name: 'test2.jar') }
+        let(:file_3_params) { params.merge(file_name: 'test3.jar') }
+
+        it 'creates a single build info' do
+          expect do
+            described_class.new(project, user, params).execute
+            described_class.new(project, user, file_2_params).execute
+            described_class.new(project, user, file_3_params).execute
+          end.to change { ::Packages::BuildInfo.count }.by(1)
+        end
+      end
     end
 
     context 'when package duplicates are not allowed' do