diff --git a/app/models/runtime/revision_sidecar_model.rb b/app/models/runtime/revision_sidecar_model.rb index 7ac46a80381..433c27dff94 100644 --- a/app/models/runtime/revision_sidecar_model.rb +++ b/app/models/runtime/revision_sidecar_model.rb @@ -17,12 +17,20 @@ class RevisionSidecarModel < Sequel::Model(:revision_sidecars) add_association_dependencies revision_sidecar_process_types: :destroy + def around_save + yield + rescue Sequel::UniqueConstraintViolation => e + raise e unless e.message.include?('revision_sidecars_revision_guid_name_index') + + errors.add(%i[revision_guid name], Sequel.lit("Sidecar with name '#{name}' already exists for revision '#{revision_guid}'")) + 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[revision_guid name], message: Sequel.lit("Sidecar with name '#{name}' already exists for given revision") end end end diff --git a/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb b/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb new file mode 100644 index 00000000000..da687bf671b --- /dev/null +++ b/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb @@ -0,0 +1,59 @@ +Sequel.migration do + no_transaction # required for concurrently option on postgres + + up do + transaction do + # remove duplicate entries if they exist + duplicates = self[:revision_sidecars]. + select(:revision_guid, :name). + group(:revision_guid, :name). + having { count(1) > 1 } + + duplicates.each do |dup| + ids_to_remove = self[:revision_sidecars]. + where(revision_guid: dup[:revision_guid], name: dup[:name]). + select(:id). + order(:id). + offset(1). + map(:id) + self[:revision_sidecars].where(id: ids_to_remove).delete + end + end + + if database_type == :postgres + VCAP::Migration.with_concurrent_timeout(self) do + add_index :revision_sidecars, %i[revision_guid name], + name: :revision_sidecars_revision_guid_name_index, + unique: true, + concurrently: true, + if_not_exists: true + end + else + alter_table(:revision_sidecars) do + # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations + unless @db.indexes(:revision_sidecars).key?(:revision_sidecars_revision_guid_name_index) + add_index %i[revision_guid name], unique: true, + name: :revision_sidecars_revision_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 :revision_sidecars, nil, + name: :revision_sidecars_revision_guid_name_index, + concurrently: true, + if_exists: true + end + else + alter_table(:revision_sidecars) do + # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations + drop_index %i[revision_guid name], name: :revision_sidecars_revision_guid_name_index if @db.indexes(:revision_sidecars).key?(:revision_sidecars_revision_guid_name_index) + # rubocop:enable Sequel/ConcurrentIndex + end + end + end +end diff --git a/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb b/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb new file mode 100644 index 00000000000..8e880505f31 --- /dev/null +++ b/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' +require 'migrations/helpers/migration_shared_context' +RSpec.describe 'add unique constraint to revision sidecar types', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20260320141005_add_unique_constraint_to_revision_sidecars.rb' } + end + + let!(:app) { VCAP::CloudController::AppModel.make } + let!(:revision) { VCAP::CloudController::RevisionModel.make(:app) } + + it 'remove dublicates, add constraint and revert migration' do + # ========================================================================================= + # SETUP: Create duplicate entries to test the de-duplication logic. + # ========================================================================================= + db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision.guid) + db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision.guid) + expect(db[:revision_sidecars].where(name: 'app', revision_guid: revision.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[:revision_sidecars].where(name: 'app', revision_guid: revision.guid).count).to eq(1) + expect(db.indexes(:revision_sidecars)).to include(:revision_sidecars_revision_guid_name_index) + expect { db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision.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(:revision_sidecars)).not_to include(:revision_sidecars_revision_guid_name_index) + expect { db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision.guid) }.not_to raise_error + end +end diff --git a/spec/unit/models/runtime/revision_sidecar_model_spec.rb b/spec/unit/models/runtime/revision_sidecar_model_spec.rb index e2305b8e94a..878e87c6647 100644 --- a/spec/unit/models/runtime/revision_sidecar_model_spec.rb +++ b/spec/unit/models/runtime/revision_sidecar_model_spec.rb @@ -43,5 +43,26 @@ module VCAP::CloudController end.to raise_error(Sequel::UniqueConstraintViolation) end end + + describe 'Validations' do + it { is_expected.to validate_presence :name } + it { is_expected.to validate_presence :command } + end + + describe 'revision_sidecar_model #around_save' do + it 'raises validation error on unique constraint violation for revision sidecar' do + revision_sidecar = RevisionSidecarModel.make(name: 'revision2', command: 'exec') + expect do + RevisionSidecarModel.make(name: revision_sidecar.name, revision_guid: revision_sidecar.revision_guid, command: 'exec') + end.to raise_error(Sequel::ValidationFailed) { |error| expect(error.message).to include('already exists for revision') } + end + + it 'raises the original error on other unique constraint violations' do + revision_sidecar = RevisionSidecarModel.make(name: 'revision3', command: 'exec') + expect do + RevisionSidecarModel.make(guid: revision_sidecar.guid, name: 'revision4', command: 'exec') + end.to raise_error(Sequel::UniqueConstraintViolation) + end + end end end