From 692b65dcc9aedbedb07c5cb13d39314b3186da94 Mon Sep 17 00:00:00 2001
From: Moaz Khalifa <mkhalifa@gitlab.com>
Date: Wed, 18 Dec 2024 10:46:40 +0000
Subject: [PATCH] Replace DB unique index with model validator
The DB unique index in packages_nuget_symbols table cannot be aware of the package's status. However, we need to only validate the uniqueness if the package is installable. So we decided to remove the DB index and do the uniqueness validation on the application level to have access to the package's status. We can tolerate a few duplicates in the table if something escaped the model validation due to some race conditional, which isn't likely to happen.
Changelog: fixed
---
app/models/packages/nuget/symbol.rb | 18 ++++-
.../symbols/create_symbol_files_service.rb | 3 +-
...symbols_on_lower_signature_and_filepath.rb | 17 +++++
...bols_on_signature_and_filepath_with_pkg.rb | 23 +++++++
db/schema_migrations/20241205142115 | 1 +
db/schema_migrations/20241217093056 | 1 +
db/structure.sql | 2 +-
spec/models/packages/nuget/symbol_spec.rb | 66 +++++++++++++++++--
.../create_symbol_files_service_spec.rb | 2 -
9 files changed, 118 insertions(+), 15 deletions(-)
create mode 100644 db/migrate/20241217093056_add_index_packages_nuget_symbols_on_lower_signature_and_filepath.rb
create mode 100644 db/post_migrate/20241205142115_remove_index_packages_nuget_symbols_on_signature_and_filepath_with_pkg.rb
create mode 100644 db/schema_migrations/20241205142115
create mode 100644 db/schema_migrations/20241217093056
diff --git a/app/models/packages/nuget/symbol.rb b/app/models/packages/nuget/symbol.rb
index bbfaff3e8989d..36213a85073cf 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 be931413ae6c9..9de32dab18f39 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 0000000000000..76102189a2061
--- /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 0000000000000..0b645d8be2b5a
--- /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 0000000000000..7e0b83b76a036
--- /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 0000000000000..c89b7bf938909
--- /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 2b7520aaaf419..91bbe9995ea3e 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 b4a5eb019964e..018dc93e671df 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 9707386ec21fc..01a86d7eb3d99 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
--
GitLab