diff --git a/app/models/runtime/security_group.rb b/app/models/runtime/security_group.rb index e69f0e18f5d..0b70567d17c 100644 --- a/app/models/runtime/security_group.rb +++ b/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_group_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 diff --git a/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb b/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb new file mode 100644 index 00000000000..a6651e7a189 --- /dev/null +++ b/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_group_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_group_name_index, unique: true unless @db.indexes(:security_groups).key?(:security_group_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_group_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_group_name_index if @db.indexes(:security_groups).key?(:security_group_name_index) + # rubocop:enable Sequel/ConcurrentIndex + end + end + end +end diff --git a/spec/migrations/20260323130619_add_unique_constraint_to_security_groups_spec.rb b/spec/migrations/20260323130619_add_unique_constraint_to_security_groups_spec.rb new file mode 100644 index 00000000000..b63ca83ffe3 --- /dev/null +++ b/spec/migrations/20260323130619_add_unique_constraint_to_security_groups_spec.rb @@ -0,0 +1,45 @@ +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 dublicates, add constraint and revert migration' do + # ========================================================================================= + # SETUP: Create duplicate entries to test the de-duplication logic. + # ========================================================================================= + 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) + + # ========================================================================================= + # UP MIGRATION: Run the migration to apply the unique constraints. + # ======================================================================================== + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + + # ========================================================================================= + # ASSERT UP MIGRATION: Verify that duplicates are removed and constraints are enforced. + # ========================================================================================= + expect(db[:security_groups].where(name: 'sec1').count).to eq(1) + expect(db.indexes(:security_groups)).to include(:security_group_name_index) + expect { db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') }.to raise_error(Sequel::UniqueConstraintViolation) + + # ========================================================================================= + # TEST IDEMPOTENCY: 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 + + # ========================================================================================= + # DOWN MIGRATION: Roll back the migration to remove the constraints. + # ========================================================================================= + Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) + + # ========================================================================================= + # ASSERT DOWN MIGRATION: Verify that constraints are removed and duplicates can be re-inserted. + # ========================================================================================= + expect(db.indexes(:security_groups)).not_to include(:security_group_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 diff --git a/spec/unit/models/runtime/security_group_spec.rb b/spec/unit/models/runtime/security_group_spec.rb index 73cc0e09ded..393cce99f91 100644 --- a/spec/unit/models/runtime/security_group_spec.rb +++ b/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 securit 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 }