diff --git a/app/models/packages/nuget/symbol.rb b/app/models/packages/nuget/symbol.rb
index bbfaff3e8989d5b1aacce1b0a6ba124d6675a4f2..36213a85073cf9c08046a59802a37d204243687c 100644
--- a/app/models/packages/nuget/symbol.rb
+++ b/app/models/packages/nuget/symbol.rb
@@ -16,10 +16,10 @@ class Symbol < ApplicationRecord
update_project_statistics project_statistics_name: :packages_size
- validates :file, :file_path, :signature, :object_storage_key, :size, presence: true
- validates :signature, uniqueness: { scope: %i[file_path package_id] }, if: -> { package }
+ validates :package, :file, :file_path, :signature, :object_storage_key, :size, presence: true
validates :object_storage_key, uniqueness: true
- validates :package, presence: true
+
+ validate :unique_signature_and_file_path_when_installable_package, on: :create
sha256_attribute :file_sha256
@@ -32,6 +32,7 @@ class Symbol < ApplicationRecord
scope :with_file_name, ->(file_name) { where(arel_table[:file].lower.eq(file_name.downcase)) }
scope :with_signature, ->(signature) { where(arel_table[:signature].lower.eq(signature.downcase)) }
scope :with_file_sha256, ->(checksums) { where(file_sha256: Array.wrap(checksums).map(&:downcase)) }
+ scope :with_file_path, ->(file_path) { where(arel_table[:file_path].lower.eq(file_path.downcase)) }
def self.find_by_signature_and_file_and_checksum(signature, file_name, checksums)
with_signature(signature)
@@ -51,6 +52,17 @@ def set_object_storage_key
root_hash: project_id
).to_s
end
+
+ def unique_signature_and_file_path_when_installable_package
+ return if errors.any?
+
+ return unless ::Packages::Package
+ .id_in(self.class.with_signature(signature).with_file_path(file_path).select(:package_id))
+ .installable
+ .exists?
+
+ errors.add(:signature, 'has already been taken')
+ end
end
end
end
diff --git a/app/services/packages/nuget/symbols/create_symbol_files_service.rb b/app/services/packages/nuget/symbols/create_symbol_files_service.rb
index be931413ae6c99f11087de9f0d6d7670a1f96ff0..9de32dab18f39cbab394e0eff8230e1ed588fd86 100644
--- a/app/services/packages/nuget/symbols/create_symbol_files_service.rb
+++ b/app/services/packages/nuget/symbols/create_symbol_files_service.rb
@@ -46,8 +46,7 @@ def create_symbol(path, file)
signature, checksum = extract_signature_and_checksum(file)
return if signature.blank? || checksum.blank?
- ::Packages::Nuget::Symbol.create!(
- package: package,
+ package.nuget_symbols.create(
file: { tempfile: file, filename: path.downcase, content_type: CONTENT_TYPE },
file_path: path,
signature: signature,
diff --git a/db/migrate/20241217093056_add_index_packages_nuget_symbols_on_lower_signature_and_filepath.rb b/db/migrate/20241217093056_add_index_packages_nuget_symbols_on_lower_signature_and_filepath.rb
new file mode 100644
index 0000000000000000000000000000000000000000..76102189a2061c6643cd571db79ca124c3b49e5e
--- /dev/null
+++ b/db/migrate/20241217093056_add_index_packages_nuget_symbols_on_lower_signature_and_filepath.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class AddIndexPackagesNugetSymbolsOnLowerSignatureAndFilepath < Gitlab::Database::Migration[2.2]
+ disable_ddl_transaction!
+ milestone '17.8'
+
+ TABLE_NAME = :packages_nuget_symbols
+ INDEX_NAME = :idx_pkgs_nuget_symbols_on_lowercase_signature_and_file_path
+
+ def up
+ add_concurrent_index TABLE_NAME, 'LOWER(signature), LOWER(file_path)', name: INDEX_NAME
+ end
+
+ def down
+ remove_concurrent_index_by_name TABLE_NAME, INDEX_NAME
+ end
+end
diff --git a/db/post_migrate/20241205142115_remove_index_packages_nuget_symbols_on_signature_and_filepath_with_pkg.rb b/db/post_migrate/20241205142115_remove_index_packages_nuget_symbols_on_signature_and_filepath_with_pkg.rb
new file mode 100644
index 0000000000000000000000000000000000000000..0b645d8be2b5a861e12e5c8324145437313b48aa
--- /dev/null
+++ b/db/post_migrate/20241205142115_remove_index_packages_nuget_symbols_on_signature_and_filepath_with_pkg.rb
@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+class RemoveIndexPackagesNugetSymbolsOnSignatureAndFilepathWithPkg < Gitlab::Database::Migration[2.2]
+ disable_ddl_transaction!
+ milestone '17.8'
+
+ TABLE_NAME = :packages_nuget_symbols
+ INDEX_NAME = :idx_pkgs_nuget_symbols_on_signature_and_file_path_with_pkg_id
+
+ def up
+ remove_concurrent_index_by_name TABLE_NAME, INDEX_NAME
+ end
+
+ def down
+ add_concurrent_index(
+ TABLE_NAME,
+ %i[signature file_path],
+ unique: true,
+ name: INDEX_NAME,
+ where: 'package_id IS NOT NULL'
+ )
+ end
+end
diff --git a/db/schema_migrations/20241205142115 b/db/schema_migrations/20241205142115
new file mode 100644
index 0000000000000000000000000000000000000000..7e0b83b76a036d212dd0c9cd329c9cf1a7a5003b
--- /dev/null
+++ b/db/schema_migrations/20241205142115
@@ -0,0 +1 @@
+02dca3a01bfdca1ca3ba2fb75689d39f580923739c6edc19b047b97c250e1f14
\ No newline at end of file
diff --git a/db/schema_migrations/20241217093056 b/db/schema_migrations/20241217093056
new file mode 100644
index 0000000000000000000000000000000000000000..c89b7bf9389096d3a7327f46ff874c8329dabdaf
--- /dev/null
+++ b/db/schema_migrations/20241217093056
@@ -0,0 +1 @@
+e840562920a4271e12412d7600f7702eaa99e41f97a0a9b23469388966085a9c
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 2b7520aaaf419107545e938668ecf78e50249c27..91bbe9995ea3eb755760587f93b52f596af7e278 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -29074,7 +29074,7 @@ CREATE INDEX idx_pkgs_npm_metadata_caches_on_id_and_project_id_and_status ON pac
CREATE INDEX idx_pkgs_nuget_symbols_on_lowercase_signature_and_file_name ON packages_nuget_symbols USING btree (lower(signature), lower(file));
-CREATE UNIQUE INDEX idx_pkgs_nuget_symbols_on_signature_and_file_path_with_pkg_id ON packages_nuget_symbols USING btree (signature, file_path) WHERE (package_id IS NOT NULL);
+CREATE INDEX idx_pkgs_nuget_symbols_on_lowercase_signature_and_file_path ON packages_nuget_symbols USING btree (lower(signature), lower(file_path));
CREATE INDEX idx_pkgs_on_project_id_name_version_on_installable_terraform ON packages_packages USING btree (project_id, name, version, id) WHERE ((package_type = 12) AND (status = ANY (ARRAY[0, 1])));
diff --git a/spec/models/packages/nuget/symbol_spec.rb b/spec/models/packages/nuget/symbol_spec.rb
index b4a5eb019964e0d6df91975805394f7b78001c97..018dc93e671dfe157d10dd7f43a1a89830a2a738 100644
--- a/spec/models/packages/nuget/symbol_spec.rb
+++ b/spec/models/packages/nuget/symbol_spec.rb
@@ -26,18 +26,55 @@
it { is_expected.to validate_presence_of(:object_storage_key) }
it { is_expected.to validate_presence_of(:size) }
it { is_expected.to validate_uniqueness_of(:object_storage_key).case_insensitive }
- it { is_expected.to validate_uniqueness_of(:signature).scoped_to(:file_path, :package_id) }
- context 'when package is nil' do
+ context 'for signature & file_path uniqueness' do
let(:new_symbol) { build(:nuget_symbol, signature: symbol.signature) }
- before do
- new_symbol.package = nil
- new_symbol.validate
+ context 'when package is nil' do
+ before do
+ symbol.package = nil
+ new_symbol.validate
+ end
+
+ it 'does not validate uniqueness of signature' do
+ expect(new_symbol.errors.messages_for(:signature)).not_to include 'has already been taken'
+ end
end
- it 'does not validate uniqueness of signature' do
- expect(new_symbol.errors.messages_for(:signature)).not_to include 'has already been taken'
+ context 'when package is installable' do
+ before do
+ new_symbol.object_storage_key = '123/foobar/456'
+ new_symbol.validate
+ end
+
+ it 'validates uniqueness of signature' do
+ expect(new_symbol.errors.messages_for(:signature)).to include 'has already been taken'
+ end
+ end
+
+ context 'when package is not installable' do
+ before do
+ new_symbol.package = symbol.package if package_exists
+ symbol.package.update_column(:status, :pending_destruction)
+ new_symbol.object_storage_key = '123/foobar/456'
+ new_symbol.validate
+ end
+
+ context 'and package already exists' do
+ let(:package_exists) { true }
+
+ it 'does not validate uniqueness of signature' do
+ expect(new_symbol.errors.messages_for(:signature)).not_to include 'has already been taken'
+ end
+ end
+
+ context 'and package does not exist' do
+ let(:package_exists) { false }
+
+ it 'does not validate uniqueness of signature' do
+ expect(new_symbol.errors.messages_for(:signature)).not_to include 'has already been taken'
+ end
+ end
end
end
end
@@ -120,6 +157,21 @@
end
end
+ describe '.with_file_path' do
+ let_it_be(:file_path) { 'symbol_package/file.pdb' }
+ let_it_be(:symbol) { create(:nuget_symbol, file_path: file_path) }
+
+ subject { described_class.with_file_path(file_path) }
+
+ it { is_expected.to contain_exactly(symbol) }
+
+ context 'when file_path is in uppercase' do
+ subject { described_class.with_file_path(file_path.upcase) }
+
+ it { is_expected.to contain_exactly(symbol) }
+ end
+ end
+
describe '.find_by_signature_and_file_and_checksum' do
subject { described_class.find_by_signature_and_file_and_checksum(signature, file_name, checksum) }
diff --git a/spec/services/packages/nuget/symbols/create_symbol_files_service_spec.rb b/spec/services/packages/nuget/symbols/create_symbol_files_service_spec.rb
index 9707386ec21fcc95b573b243c8b60b1af74bd10d..01a86d7eb3d995b4d3dfd7d6cff9134532eee387 100644
--- a/spec/services/packages/nuget/symbols/create_symbol_files_service_spec.rb
+++ b/spec/services/packages/nuget/symbols/create_symbol_files_service_spec.rb
@@ -93,8 +93,6 @@
it 'does not create a symbol record' do
expect { subject }.not_to change { package.nuget_symbols.count }
end
-
- it_behaves_like 'logging an error', ActiveRecord::RecordInvalid
end
context 'when a symbol file has the wrong entry size' do