From 41cac0cbb23fcb89a2c2202a0eafcaca8252fd89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Mon, 23 Mar 2026 13:17:46 +0100 Subject: [PATCH 1/2] feat: unique constraint added on db level for sidecar table - db migration added on columns app_guid and name. Unit testting added too - sequel validate uniqueness removed from model and around save added instead --- app/models/runtime/sidecar_model.rb | 10 +++- ...92954_add_unique_constraint_to_sidecars.rb | 33 +++++++++++++ ..._add_unique_constraint_to_sidecars_spec.rb | 46 +++++++++++++++++++ .../unit/models/runtime/sidecar_model_spec.rb | 20 ++++++-- 4 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb create mode 100644 spec/migrations/20260323092954_add_unique_constraint_to_sidecars_spec.rb diff --git a/app/models/runtime/sidecar_model.rb b/app/models/runtime/sidecar_model.rb index 990c165cc77..96c15bd8853 100644 --- a/app/models/runtime/sidecar_model.rb +++ b/app/models/runtime/sidecar_model.rb @@ -16,12 +16,20 @@ class SidecarModel < Sequel::Model(:sidecars) key: :sidecar_guid, primary_key: :guid + def around_save + yield + rescue Sequel::UniqueConstraintViolation => e + raise e unless e.message.include?('sidecars_app_guid_name_index') + + errors.add(%i[app_guid name], Sequel.lit("Sidecar with name '#{name}' already exists for given app")) + raise validation_failed_error + end + def validate super validates_presence %i[name command] validates_max_length 255, :name, message: Sequel.lit('Name is too long (maximum is 255 characters)') validates_max_length 4096, :command, message: Sequel.lit('Command is too long (maximum is 4096 characters)') - validates_unique %i[app_guid name], message: Sequel.lit("Sidecar with name '#{name}' already exists for given app") end end end diff --git a/db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb b/db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb new file mode 100644 index 00000000000..0503879ed7f --- /dev/null +++ b/db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb @@ -0,0 +1,33 @@ +Sequel.migration do + up do + transaction do + duplicates = self[:sidecars]. + select(:app_guid, :name). + group(:app_guid, :name). + having { count(1) > 1 } + duplicates.each do |dup| + ids_to_remove = self[:sidecars]. + where(app_guid: dup[:app_guid], name: dup[:name]). + select(:id). + order(:id). + offset(1). + map(:id) + + self[:sidecars].where(id: ids_to_remove).delete + end + + alter_table(:sidecars) do + unless @db.indexes(:sidecars).key?(:sidecars_app_guid_name_index) + add_unique_constraint %i[app_guid name], + name: :sidecars_app_guid_name_index + end + end + end + end + + down do + alter_table(:sidecars) do + drop_constraint(:sidecars_app_guid_name_index, type: :unique) if @db.indexes(:sidecars).key?(:sidecars_app_guid_name_index) + end + end +end diff --git a/spec/migrations/20260323092954_add_unique_constraint_to_sidecars_spec.rb b/spec/migrations/20260323092954_add_unique_constraint_to_sidecars_spec.rb new file mode 100644 index 00000000000..b778c687e57 --- /dev/null +++ b/spec/migrations/20260323092954_add_unique_constraint_to_sidecars_spec.rb @@ -0,0 +1,46 @@ +require 'spec_helper' +require 'migrations/helpers/migration_shared_context' +RSpec.describe 'add unique constraint to sidecars', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20260323092954_add_unique_constraint_to_sidecars.rb' } + end + + let!(:app) { VCAP::CloudController::AppModel.make } + + it 'remove dublicates, add constraint and revert migration' do + # ========================================================================================= + # SETUP: Create duplicate entries to test the de-duplication logic. + # ========================================================================================= + db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid) + db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid) + expect(db[:sidecars].where(name: 'app', app_guid: app.guid).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[:sidecars].where(name: 'app', app_guid: app.guid).count).to eq(1) + expect(db.indexes(:sidecars)).to include(:sidecars_app_guid_name_index) + expect { db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid) }.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(:sidecars)).not_to include(:sidecars_app_guid_name_index) + expect { db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid) }.not_to raise_error + end +end diff --git a/spec/unit/models/runtime/sidecar_model_spec.rb b/spec/unit/models/runtime/sidecar_model_spec.rb index c3444664f14..6d759a8cd86 100644 --- a/spec/unit/models/runtime/sidecar_model_spec.rb +++ b/spec/unit/models/runtime/sidecar_model_spec.rb @@ -14,12 +14,24 @@ module VCAP::CloudController end describe 'validations' do + it { is_expected.to validate_presence :name } + it { is_expected.to validate_presence :command } + end + + describe 'sidecar model #around_save' do let(:app_model) { AppModel.make } - let!(:sidecar) { SidecarModel.make(name: 'my_sidecar', app: app_model) } + let!(:sidecar) { SidecarModel.make(app_guid: app_model.guid, name: 'sidecar1', command: 'exec') } - it 'validates unique sidecar name per app' do - expect { SidecarModel.create(app: app_model, name: 'my_sidecar', command: 'some-command') }. - to raise_error(Sequel::ValidationFailed, /Sidecar with name 'my_sidecar' already exists for given app/) + it 'raises validation error on unique constraint violation for sidecar' do + expect do + SidecarModel.create(guid: SecureRandom.uuid, app_guid: app_model.guid, name: sidecar.name, command: 'exec') + end.to raise_error(Sequel::ValidationFailed) { |error| expect(error.message).to include("Sidecar with name 'sidecar1' already exists for given app") } + end + + it 'raises the original error on other unique constraint violations' do + expect do + SidecarModel.create(guid: sidecar.guid, app_guid: app_model.guid, name: 'sidecar2', command: 'exec') + end.to raise_error(Sequel::UniqueConstraintViolation) end end From f5a3fc3e09918a8715521907be8caf9847b7b452 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 16:03:46 +0100 Subject: [PATCH 2/2] feat: concurrent migration for postgres db --- ...92954_add_unique_constraint_to_sidecars.rb | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb b/db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb index 0503879ed7f..096d0454871 100644 --- a/db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb +++ b/db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb @@ -1,10 +1,13 @@ Sequel.migration do + no_transaction # required for concurrently option on postgres + up do transaction do duplicates = self[:sidecars]. select(:app_guid, :name). group(:app_guid, :name). having { count(1) > 1 } + duplicates.each do |dup| ids_to_remove = self[:sidecars]. where(app_guid: dup[:app_guid], name: dup[:name]). @@ -15,19 +18,42 @@ self[:sidecars].where(id: ids_to_remove).delete end + end + if database_type == :postgres + VCAP::Migration.with_concurrent_timeout(self) do + add_index :sidecars, %i[app_guid name], + name: :sidecars_app_guid_name_index, + unique: true, + concurrently: true, + if_not_exists: true + end + else alter_table(:sidecars) do + # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations unless @db.indexes(:sidecars).key?(:sidecars_app_guid_name_index) - add_unique_constraint %i[app_guid name], - name: :sidecars_app_guid_name_index + add_index %i[app_guid name], unique: true, + name: :sidecars_app_guid_name_index end + # rubocop:enable Sequel/ConcurrentIndex end end end down do - alter_table(:sidecars) do - drop_constraint(:sidecars_app_guid_name_index, type: :unique) if @db.indexes(:sidecars).key?(:sidecars_app_guid_name_index) + if database_type == :postgres + VCAP::Migration.with_concurrent_timeout(self) do + drop_index :sidecars, nil, + name: :sidecars_app_guid_name_index, + concurrently: true, + if_exists: true + end + else + alter_table(:sidecars) do + # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations + drop_index %i[app_guid name], name: :sidecars_app_guid_name_index if @db.indexes(:sidecars).key?(:sidecars_app_guid_name_index) + # rubocop:enable Sequel/ConcurrentIndex + end end end end