From 64dec4967fa311b4afa040745b6b606f4f8e918c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Mon, 23 Mar 2026 15:15:34 +0100 Subject: [PATCH 1/3] 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 --- app/models/runtime/security_group.rb | 10 ++++- ...dd_unique_constraint_to_security_groups.rb | 36 +++++++++++++++ ...ique_constraint_to_security_groups_spec.rb | 45 +++++++++++++++++++ .../models/runtime/security_group_spec.rb | 24 +++++++++- 4 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb create mode 100644 spec/migrations/20260323130619_add_unique_constraint_to_security_groups_spec.rb 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..265ee5b5cd2 --- /dev/null +++ b/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb @@ -0,0 +1,36 @@ +Sequel.migration do + 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 + + alter_table(:security_groups) do + # Cannot add unique constraint concurrently as it requires a transaction + # rubocop:disable Sequel/ConcurrentIndex + drop_index :name, name: :sg_name_index if @db.indexes(:security_groups).key?(:sg_name_index) + add_index :name, name: :security_group_name_index, unique: true unless @db.indexes(:security_groups).key?(:security_group_name_index) + # rubocop:enable Sequel/ConcurrentIndex + end + end + end + + down do + alter_table(:security_groups) do + # rubocop:disable Sequel/ConcurrentIndex + drop_index :name, name: :security_group_name_index if @db.indexes(:security_groups).key?(:security_group_name_index) + add_index :name, name: :sg_name_index unless @db.indexes(:security_groups).key?(:sg_name_index) + # rubocop:enable Sequel/ConcurrentIndex + 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 } From eed86a8714e32003f28e56bb3e731cad7bab4744 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 15:55:10 +0100 Subject: [PATCH 2/3] feat: concurrent migration for postgres db --- ...dd_unique_constraint_to_security_groups.rb | 44 +++++++++++++++---- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb b/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb index 265ee5b5cd2..c38dced334c 100644 --- a/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb +++ b/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb @@ -1,4 +1,6 @@ -Sequel.migration do +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). @@ -14,10 +16,23 @@ map(:id) self[:security_groups].where(id: ids_to_remove).delete end + end + if database_type == :postgres + VCAP::Migration.with_concurrent_timeout(self) do + drop_index :security_groups, nil, + name: :sg_name_index, + concurrently: true, + if_exists: true + add_index :security_groups, :name, + name: :security_group_name_index, + unique: true, + concurrently: true, + if_not_exists: true + end + else alter_table(:security_groups) do - # Cannot add unique constraint concurrently as it requires a transaction - # rubocop:disable Sequel/ConcurrentIndex + # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations drop_index :name, name: :sg_name_index if @db.indexes(:security_groups).key?(:sg_name_index) add_index :name, name: :security_group_name_index, unique: true unless @db.indexes(:security_groups).key?(:security_group_name_index) # rubocop:enable Sequel/ConcurrentIndex @@ -26,11 +41,24 @@ end down do - alter_table(:security_groups) do - # rubocop:disable Sequel/ConcurrentIndex - drop_index :name, name: :security_group_name_index if @db.indexes(:security_groups).key?(:security_group_name_index) - add_index :name, name: :sg_name_index unless @db.indexes(:security_groups).key?(:sg_name_index) - # rubocop:enable Sequel/ConcurrentIndex + if database_type == :postgres + VCAP::Migration.with_concurrent_timeout(self) do + drop_index :security_groups, nil, + name: :security_group_name_index, + concurrently: true, + if_exists: true + add_index :security_groups, :name, + name: :sg_name_index, + concurrently: true, + if_not_exists: true + end + else + alter_table(:security_groups) do + # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations + drop_index :name, name: :security_group_name_index if @db.indexes(:security_groups).key?(:security_group_name_index) + add_index :name, name: :sg_name_index unless @db.indexes(:security_groups).key?(:sg_name_index) + # rubocop:enable Sequel/ConcurrentIndex + end end end end From a70e59a3a6d47148ce0cc1410830bb8a55521a63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 16:33:00 +0100 Subject: [PATCH 3/3] fix: add index first, then drop --- ...dd_unique_constraint_to_security_groups.rb | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb b/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb index c38dced334c..a6651e7a189 100644 --- a/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb +++ b/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb @@ -20,21 +20,21 @@ if database_type == :postgres VCAP::Migration.with_concurrent_timeout(self) do - drop_index :security_groups, nil, - name: :sg_name_index, - concurrently: true, - if_exists: true 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 - drop_index :name, name: :sg_name_index if @db.indexes(:security_groups).key?(:sg_name_index) 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 @@ -43,20 +43,20 @@ down do if database_type == :postgres VCAP::Migration.with_concurrent_timeout(self) do - drop_index :security_groups, nil, - name: :security_group_name_index, - concurrently: true, - if_exists: true 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 - drop_index :name, name: :security_group_name_index if @db.indexes(:security_groups).key?(:security_group_name_index) 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