diff --git a/app/models/runtime/security_group.rb b/app/models/runtime/security_group.rb index 281255b875a..12e0fb72dec 100644 --- a/app/models/runtime/security_group.rb +++ b/app/models/runtime/security_group.rb @@ -12,7 +12,7 @@ class SecurityGroup < Sequel::Model serialize_attributes :json, :rules - many_to_many :spaces + many_to_many :spaces, ignored_unique_constraint_violation_errors: %w[ignored_unique_constraint_violation_errors] many_to_many :staging_spaces, class: 'VCAP::CloudController::Space', join_table: 'staging_security_groups_spaces', diff --git a/db/migrations/20251016120006_security_groups_spaces_unique_index.rb b/db/migrations/20251016120006_security_groups_spaces_unique_index.rb new file mode 100644 index 00000000000..2bb693d8f42 --- /dev/null +++ b/db/migrations/20251016120006_security_groups_spaces_unique_index.rb @@ -0,0 +1,39 @@ +Sequel.migration do + up do + transaction do + # Remove duplicate entries if they exist + duplicates = self[:security_groups_spaces]. + select(:security_group_id, :space_id). + group(:security_group_id, :space_id). + having { count(:security_groups_spaces_pk) > 1 } + + duplicates.each do |dup| + security_groups_spaces_pks_to_remove = self[:security_groups_spaces]. + where(security_group_id: dup[:security_group_id], space_id: dup[:space_id]). + select(:security_groups_spaces_pk). + order(:security_groups_spaces_pk). + offset(1). + map(:security_groups_spaces_pk) + + self[:security_groups_spaces].where(security_groups_spaces_pk: security_groups_spaces_pks_to_remove).delete + end + + alter_table(:security_groups_spaces) do + # Cannot add unique constraint concurrently as it requires a transaction + # rubocop:disable Sequel/ConcurrentIndex + add_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids unless @db.indexes(:security_groups_spaces).key?(:security_groups_spaces_ids) + drop_index %i[security_group_id space_id], name: :sgs_spaces_ids if @db.indexes(:security_groups_spaces).key?(:sgs_spaces_ids) + # rubocop:enable Sequel/ConcurrentIndex + end + end + end + + down do + alter_table(:security_groups_spaces) do + # rubocop:disable Sequel/ConcurrentIndex + add_index %i[security_group_id space_id], name: :sgs_spaces_ids unless @db.indexes(:security_groups_spaces).key?(:sgs_spaces_ids) + drop_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids if @db.indexes(:security_groups_spaces).key?(:security_groups_spaces_ids) + # rubocop:enable Sequel/ConcurrentIndex + end + end +end diff --git a/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb b/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb new file mode 100644 index 00000000000..94affbcf364 --- /dev/null +++ b/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb @@ -0,0 +1,76 @@ +require 'spec_helper' +require 'migrations/helpers/migration_shared_context' + +RSpec.describe 'security groups spaces unique index', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20251016120006_security_groups_spaces_unique_index_spec.rb' } + end + + let(:space_1) { VCAP::CloudController::Space.make } + let(:space_2) { VCAP::CloudController::Space.make } + let(:sec_group_1) { VCAP::CloudController::SecurityGroup.make } + let(:sec_group_2) { VCAP::CloudController::SecurityGroup.make } + + describe 'security_groups_spaces table' do + context 'up migration' do + it 'is in the correct state before migration' do + expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) + end + + it 'removes duplicates and migrates successfully by adding unique index' do + db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_2.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_1.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) + + # Count duplicates before migration + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(2) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(3) + + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + + # Verify duplicates are removed after migration + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(1) + + # Verify indexes are updated + expect(db.indexes(:security_groups_spaces)).not_to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).to include(:security_groups_spaces_ids) + end + + it 'does not fail if indexes/constraints are already in desired state' do + db.alter_table :security_groups_spaces do + add_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids + drop_index %i[security_group_id space_id], name: :sgs_spaces_ids + end + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + end + end + + context 'down migration' do + it 'rolls back successfully' do + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) + end + + it 'does not fail if indexes/constraints are already in desired state' do + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + db.alter_table :security_groups_spaces do + add_index %i[security_group_id space_id], name: :sgs_spaces_ids + drop_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids + end + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + end + end + end +end