Commit 94e08bc1 authored by David Fernandez's avatar David Fernandez

Remove safe_find_or_create_by! calls

For package build_infos

See https://gitlab.com/gitlab-org/gitlab/-/issues/338954

Changelog: performance
parent 95bc8490
...@@ -291,6 +291,13 @@ class Packages::Package < ApplicationRecord ...@@ -291,6 +291,13 @@ class Packages::Package < ApplicationRecord
::Packages::Maven::Metadata::SyncWorker.perform_async(user.id, project.id, name) ::Packages::Maven::Metadata::SyncWorker.perform_async(user.id, project.id, name)
end 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 private
def composer_tag_version? def composer_tag_version?
......
...@@ -29,7 +29,8 @@ module Packages ...@@ -29,7 +29,8 @@ module Packages
package.update_column(:status, params[:status]) if params[:status] && params[:status] != package.status 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 package
end end
......
...@@ -51,7 +51,7 @@ module Packages ...@@ -51,7 +51,7 @@ module Packages
.execute .execute
end 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 }) ServiceResponse.success(payload: { package: package })
end end
......
...@@ -354,4 +354,12 @@ FactoryBot.define do ...@@ -354,4 +354,12 @@ FactoryBot.define do
package package
sequence(:name) { |n| "tag-#{n}"} sequence(:name) { |n| "tag-#{n}"}
end end
factory :packages_build_info, class: 'Packages::BuildInfo' do
package
trait :with_pipeline do
association :pipeline, factory: [:ci_pipeline, :with_job]
end
end
end end
...@@ -1165,4 +1165,47 @@ RSpec.describe Packages::Package, type: :model do ...@@ -1165,4 +1165,47 @@ RSpec.describe Packages::Package, type: :model do
it_behaves_like 'not enqueuing a sync worker job' it_behaves_like 'not enqueuing a sync worker job'
end end
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 end
...@@ -105,6 +105,37 @@ RSpec.describe Packages::Generic::CreatePackageFileService do ...@@ -105,6 +105,37 @@ RSpec.describe Packages::Generic::CreatePackageFileService do
it { expect { execute_service }.to change { project.package_files.count }.by(1) } it { expect { execute_service }.to change { project.package_files.count }.by(1) }
end end
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 end
end end
...@@ -98,6 +98,19 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do ...@@ -98,6 +98,19 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do
it 'creates a build_info' do it 'creates a build_info' do
expect { subject }.to change { Packages::BuildInfo.count }.by(1) expect { subject }.to change { Packages::BuildInfo.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: '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 end
context 'when package duplicates are not allowed' do context 'when package duplicates are not allowed' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment