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..096d0454871 --- /dev/null +++ b/db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb @@ -0,0 +1,59 @@ +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]). + select(:id). + order(:id). + offset(1). + map(:id) + + 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_index %i[app_guid name], unique: true, + name: :sidecars_app_guid_name_index + end + # rubocop:enable Sequel/ConcurrentIndex + end + end + end + + down do + 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 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