Replacing validate uniqueness with db constraint on SecurityGroup model (#4955)

* feat: unique constraint added to security group table

- sg_name_index replaced with unique security_group_name_index on name column. For that db migration is added
- around_save hook added in model class to catch such error

* feat: concurrent migration for postgres db

* fix: add index first, then drop

* fix: index name is fixed

* fix: comments are fixed

Author: serdarozerr

app/models/runtime/security_group.rb

@@ -22,9 +22,17 @@ class SecurityGroup < Sequel::Model
 
     add_association_dependencies spaces: :nullify, staging_spaces: :nullify
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('security_groups_name_index')
+
+      errors.add(:name, :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_presence :name
-      validates_unique :name
       validates_format SECURITY_GROUP_NAME_REGEX, :name
       validate_rules_length
       validate_rules

db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb

@@ -0,0 +1,64 @@
+Sequel.migration do # rubocop:disable Metrics/BlockLength
+  no_transaction # required for concurrently option on postgres
+
+  up do
+    transaction do
+      duplicates = self[:security_groups].select(:name).
+                   group(:name).
+                   having { count(1) > 1 }
+
+      duplicates.each do |dup|
+        ids_to_remove = self[:security_groups].
+                        where(name: dup[:name]).
+                        select(:id).
+                        order(:id).
+                        offset(1).
+                        map(:id)
+        self[:security_groups].where(id: ids_to_remove).delete
+      end
+    end
+
+    if database_type == :postgres
+      VCAP::Migration.with_concurrent_timeout(self) do
+        add_index :security_groups, :name,
+                  name: :security_groups_name_index,
+                  unique: true,
+                  concurrently: true,
+                  if_not_exists: true
+        drop_index :security_groups, nil,
+                   name: :sg_name_index,
+                   concurrently: true,
+                   if_exists: true
+      end
+    else
+      alter_table(:security_groups) do
+        # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations
+        add_index :name, name: :security_groups_name_index, unique: true unless @db.indexes(:security_groups).key?(:security_groups_name_index)
+        drop_index :name, name: :sg_name_index if @db.indexes(:security_groups).key?(:sg_name_index)
+        # rubocop:enable Sequel/ConcurrentIndex
+      end
+    end
+  end
+
+  down do
+    if database_type == :postgres
+      VCAP::Migration.with_concurrent_timeout(self) do
+        add_index :security_groups, :name,
+                  name: :sg_name_index,
+                  concurrently: true,
+                  if_not_exists: true
+        drop_index :security_groups, nil,
+                   name: :security_groups_name_index,
+                   concurrently: true,
+                   if_exists: true
+      end
+    else
+      alter_table(:security_groups) do
+        # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations
+        add_index :name, name: :sg_name_index unless @db.indexes(:security_groups).key?(:sg_name_index)
+        drop_index :name, name: :security_groups_name_index if @db.indexes(:security_groups).key?(:security_groups_name_index)
+        # rubocop:enable Sequel/ConcurrentIndex
+      end
+    end
+  end
+end

spec/migrations/20260323130619_add_unique_constraint_to_security_groups_spec.rb

@@ -0,0 +1,33 @@
+require 'spec_helper'
+require 'migrations/helpers/migration_shared_context'
+RSpec.describe 'add unique constraint to security_groups', isolation: :truncation, type: :migration do
+  include_context 'migration' do
+    let(:migration_filename) { '20260323130619_add_unique_constraint_to_security_groups.rb' }
+  end
+
+  it 'remove duplicates, add constraint and revert migration' do
+    # create duplicate entries
+    db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1')
+    db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1')
+    expect(db[:security_groups].where(name: 'sec1').count).to eq(2)
+
+    # run the migration
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
+
+    # verify duplicates are removed and constraint is enforced
+    expect(db[:security_groups].where(name: 'sec1').count).to eq(1)
+    expect(db.indexes(:security_groups)).to include(:security_groups_name_index)
+    expect { db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') }.to raise_error(Sequel::UniqueConstraintViolation)
+
+    # running the migration again should not cause any errors
+    expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
+
+    # roll back the migration
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true)
+
+    # verify constraint is removed and duplicates can be re-inserted
+    expect(db.indexes(:security_groups)).not_to include(:security_groups_name_index)
+    expect(db.indexes(:security_groups)).to include(:sg_name_index)
+    expect { db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') }.not_to raise_error
+  end
+end

spec/unit/models/runtime/security_group_spec.rb

@@ -441,7 +441,6 @@ def build_all_rule(attrs={})
 
     describe 'Validations' do
       it { is_expected.to validate_presence :name }
-      it { is_expected.to validate_uniqueness :name }
 
       context 'name' do
         subject(:sec_group) { SecurityGroup.make }
@@ -971,6 +970,29 @@ def build_all_rule(attrs={})
       end
     end
 
+    describe 'security_group_model #around_save' do
+      it 'raises validation error on unique constraint violation for security group' do
+        sg = SecurityGroup.make(name: 'sec')
+        expect do
+          SecurityGroup.make(name: sg.name)
+        end.to raise_error(Sequel::ValidationFailed) { |error| expect(error.message).to match(/unique/) }
+      end
+
+      it 'raises the original error on other unique constraint violations' do
+        sg = SecurityGroup.make(name: 'sec1')
+
+        # Unlike other models, security_groups.guid does not have a unique index,
+        # so we use the primary key (id) to trigger a UniqueConstraintViolation.
+        # Sequel restricts setting id by default, so unrestrict_primary_key is required.
+        SecurityGroup.unrestrict_primary_key
+        expect do
+          SecurityGroup.create(id: sg.id, name: 'sec2')
+        end.to raise_error(Sequel::UniqueConstraintViolation)
+      ensure
+        SecurityGroup.restrict_primary_key
+      end
+    end
+
     describe '.user_visibility_filter' do
       let(:security_group) { SecurityGroup.make }
       let(:space) { Space.make }