Skip to content

Commit 64dec49

Browse files
committed
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
1 parent d16757b commit 64dec49

4 files changed

Lines changed: 113 additions & 2 deletions

File tree

app/models/runtime/security_group.rb

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,17 @@ class SecurityGroup < Sequel::Model
2222

2323
add_association_dependencies spaces: :nullify, staging_spaces: :nullify
2424

25+
def around_save
26+
yield
27+
rescue Sequel::UniqueConstraintViolation => e
28+
raise e unless e.message.include?('security_group_name_index')
29+
30+
errors.add(:name, :unique)
31+
raise validation_failed_error
32+
end
33+
2534
def validate
2635
validates_presence :name
27-
validates_unique :name
2836
validates_format SECURITY_GROUP_NAME_REGEX, :name
2937
validate_rules_length
3038
validate_rules
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
Sequel.migration do
2+
up do
3+
transaction do
4+
duplicates = self[:security_groups].select(:name).
5+
group(:name).
6+
having { count(1) > 1 }
7+
8+
duplicates.each do |dup|
9+
ids_to_remove = self[:security_groups].
10+
where(name: dup[:name]).
11+
select(:id).
12+
order(:id).
13+
offset(1).
14+
map(:id)
15+
self[:security_groups].where(id: ids_to_remove).delete
16+
end
17+
18+
alter_table(:security_groups) do
19+
# Cannot add unique constraint concurrently as it requires a transaction
20+
# rubocop:disable Sequel/ConcurrentIndex
21+
drop_index :name, name: :sg_name_index if @db.indexes(:security_groups).key?(:sg_name_index)
22+
add_index :name, name: :security_group_name_index, unique: true unless @db.indexes(:security_groups).key?(:security_group_name_index)
23+
# rubocop:enable Sequel/ConcurrentIndex
24+
end
25+
end
26+
end
27+
28+
down do
29+
alter_table(:security_groups) do
30+
# rubocop:disable Sequel/ConcurrentIndex
31+
drop_index :name, name: :security_group_name_index if @db.indexes(:security_groups).key?(:security_group_name_index)
32+
add_index :name, name: :sg_name_index unless @db.indexes(:security_groups).key?(:sg_name_index)
33+
# rubocop:enable Sequel/ConcurrentIndex
34+
end
35+
end
36+
end
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
require 'spec_helper'
2+
require 'migrations/helpers/migration_shared_context'
3+
RSpec.describe 'add unique constraint to security_groups', isolation: :truncation, type: :migration do
4+
include_context 'migration' do
5+
let(:migration_filename) { '20260323130619_add_unique_constraint_to_security_groups.rb' }
6+
end
7+
8+
it 'remove dublicates, add constraint and revert migration' do
9+
# =========================================================================================
10+
# SETUP: Create duplicate entries to test the de-duplication logic.
11+
# =========================================================================================
12+
db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1')
13+
db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1')
14+
expect(db[:security_groups].where(name: 'sec1').count).to eq(2)
15+
16+
# =========================================================================================
17+
# UP MIGRATION: Run the migration to apply the unique constraints.
18+
# ========================================================================================
19+
Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
20+
21+
# =========================================================================================
22+
# ASSERT UP MIGRATION: Verify that duplicates are removed and constraints are enforced.
23+
# =========================================================================================
24+
expect(db[:security_groups].where(name: 'sec1').count).to eq(1)
25+
expect(db.indexes(:security_groups)).to include(:security_group_name_index)
26+
expect { db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') }.to raise_error(Sequel::UniqueConstraintViolation)
27+
28+
# =========================================================================================
29+
# TEST IDEMPOTENCY: Running the migration again should not cause any errors.
30+
# =========================================================================================
31+
expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
32+
33+
# =========================================================================================
34+
# DOWN MIGRATION: Roll back the migration to remove the constraints.
35+
# =========================================================================================
36+
Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true)
37+
38+
# =========================================================================================
39+
# ASSERT DOWN MIGRATION: Verify that constraints are removed and duplicates can be re-inserted.
40+
# =========================================================================================
41+
expect(db.indexes(:security_groups)).not_to include(:security_group_name_index)
42+
expect(db.indexes(:security_groups)).to include(:sg_name_index)
43+
expect { db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') }.not_to raise_error
44+
end
45+
end

spec/unit/models/runtime/security_group_spec.rb

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,6 @@ def build_all_rule(attrs={})
441441

442442
describe 'Validations' do
443443
it { is_expected.to validate_presence :name }
444-
it { is_expected.to validate_uniqueness :name }
445444

446445
context 'name' do
447446
subject(:sec_group) { SecurityGroup.make }
@@ -971,6 +970,29 @@ def build_all_rule(attrs={})
971970
end
972971
end
973972

973+
describe 'security_group_model #around_save' do
974+
it 'raises validation error on unique constraint violation for securit group' do
975+
sg = SecurityGroup.make(name: 'sec')
976+
expect do
977+
SecurityGroup.make(name: sg.name)
978+
end.to raise_error(Sequel::ValidationFailed) { |error| expect(error.message).to match(/unique/) }
979+
end
980+
981+
it 'raises the original error on other unique constraint violations' do
982+
sg = SecurityGroup.make(name: 'sec1')
983+
984+
# Unlike other models, security_groups.guid does not have a unique index,
985+
# so we use the primary key (id) to trigger a UniqueConstraintViolation.
986+
# Sequel restricts setting id by default, so unrestrict_primary_key is required.
987+
SecurityGroup.unrestrict_primary_key
988+
expect do
989+
SecurityGroup.create(id: sg.id, name: 'sec2')
990+
end.to raise_error(Sequel::UniqueConstraintViolation)
991+
ensure
992+
SecurityGroup.restrict_primary_key
993+
end
994+
end
995+
974996
describe '.user_visibility_filter' do
975997
let(:security_group) { SecurityGroup.make }
976998
let(:space) { Space.make }

0 commit comments

Comments
 (0)