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

@@ -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

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,

db/migrate/20241217093056_add_index_packages_nuget_symbols_on_lower_signature_and_filepath.rb

+# 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

db/post_migrate/20241205142115_remove_index_packages_nuget_symbols_on_signature_and_filepath_with_pkg.rb

+# 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

db/schema_migrations/20241205142115

+02dca3a01bfdca1ca3ba2fb75689d39f580923739c6edc19b047b97c250e1f14
\ No newline at end of file

db/schema_migrations/20241217093056

+e840562920a4271e12412d7600f7702eaa99e41f97a0a9b23469388966085a9c
\ No newline at end of file

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])));

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) }
 

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