From 2fa162e66498d8b6c2a5fd7d1d2d34ae690a306f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Mon, 23 Mar 2026 09:11:14 +0100 Subject: [PATCH 1/3] feat: unique constraint for revision_guid and name added at db level - DB migration added for new uniqueness constraint. Tests for the new migration is added too. - around_save method added into RevisionSidecarModel to catch new constraint and augment it with message. Sequel:validate_uniqueness removed from the model --- app/models/runtime/revision_sidecar_model.rb | 10 +++- ...d_unique_constraint_to_revision_sidecar.rb | 34 ++++++++++++++ ...que_constraint_to_revision_sidecar_spec.rb | 47 +++++++++++++++++++ .../runtime/revision_sidecar_model_spec.rb | 23 +++++++++ 4 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 db/migrations/20260320141005_add_unique_constraint_to_revision_sidecar.rb create mode 100644 spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecar_spec.rb 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_sidecar.rb b/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecar.rb new file mode 100644 index 00000000000..70eb6c4ce5b --- /dev/null +++ b/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecar.rb @@ -0,0 +1,34 @@ +Sequel.migration do + up do + # remove duplicate entries if they exist + transaction do + 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 + + alter_table(:revision_sidecars) do + unless @db.indexes(:revision_sidecars).key?(:revision_sidecars_revision_guid_name_index) + add_unique_constraint(%i[revision_guid name], + name: :revision_sidecars_revision_guid_name_index) + end + end + end + end + + down do + alter_table(:revision_sidecars) do + drop_constraint(:revision_sidecars_revision_guid_name_index, type: :unique) if @db.indexes(:revision_sidecars).key?(:revision_sidecars_revision_guid_name_index) + end + end +end diff --git a/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecar_spec.rb b/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecar_spec.rb new file mode 100644 index 00000000000..f46fc88c4f4 --- /dev/null +++ b/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecar_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_sidecar.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..c5896f82d2f 100644 --- a/spec/unit/models/runtime/revision_sidecar_model_spec.rb +++ b/spec/unit/models/runtime/revision_sidecar_model_spec.rb @@ -43,5 +43,28 @@ module VCAP::CloudController end.to raise_error(Sequel::UniqueConstraintViolation) end end + + describe 'Validations' do + let!(:revision_sidecar) { RevisionSidecarModel.make(name: 'revision1', command: 'exec') } + + 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: 'exe') + end.to raise_error(Sequel::UniqueConstraintViolation) + end + end end end From bce39ea98ee9a5ec6b1c3412a071ccfa0a168092 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Mon, 23 Mar 2026 13:34:05 +0100 Subject: [PATCH 2/3] fix: migration files are renamed --- ...60320141005_add_unique_constraint_to_revision_sidecars.rb} | 0 ...141005_add_unique_constraint_to_revision_sidecars_spec.rb} | 2 +- spec/unit/models/runtime/revision_sidecar_model_spec.rb | 4 +--- 3 files changed, 2 insertions(+), 4 deletions(-) rename db/migrations/{20260320141005_add_unique_constraint_to_revision_sidecar.rb => 20260320141005_add_unique_constraint_to_revision_sidecars.rb} (100%) rename spec/migrations/{20260320141005_add_unique_constraint_to_revision_sidecar_spec.rb => 20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb} (99%) diff --git a/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecar.rb b/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb similarity index 100% rename from db/migrations/20260320141005_add_unique_constraint_to_revision_sidecar.rb rename to db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb diff --git a/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecar_spec.rb b/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb similarity index 99% rename from spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecar_spec.rb rename to spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb index f46fc88c4f4..8e880505f31 100644 --- a/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecar_spec.rb +++ b/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb @@ -2,7 +2,7 @@ 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_sidecar.rb' } + let(:migration_filename) { '20260320141005_add_unique_constraint_to_revision_sidecars.rb' } end let!(:app) { VCAP::CloudController::AppModel.make } diff --git a/spec/unit/models/runtime/revision_sidecar_model_spec.rb b/spec/unit/models/runtime/revision_sidecar_model_spec.rb index c5896f82d2f..878e87c6647 100644 --- a/spec/unit/models/runtime/revision_sidecar_model_spec.rb +++ b/spec/unit/models/runtime/revision_sidecar_model_spec.rb @@ -45,8 +45,6 @@ module VCAP::CloudController end describe 'Validations' do - let!(:revision_sidecar) { RevisionSidecarModel.make(name: 'revision1', command: 'exec') } - it { is_expected.to validate_presence :name } it { is_expected.to validate_presence :command } end @@ -62,7 +60,7 @@ module VCAP::CloudController 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: 'exe') + RevisionSidecarModel.make(guid: revision_sidecar.guid, name: 'revision4', command: 'exec') end.to raise_error(Sequel::UniqueConstraintViolation) end end From 012595333b366c84dafbe363cfa55edbc0466d02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 16:10:53 +0100 Subject: [PATCH 3/3] feat: concurrent migration for postgres db --- ..._unique_constraint_to_revision_sidecars.rb | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb b/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb index 70eb6c4ce5b..da687bf671b 100644 --- a/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb +++ b/db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb @@ -1,7 +1,9 @@ Sequel.migration do + no_transaction # required for concurrently option on postgres + up do - # remove duplicate entries if they exist transaction do + # remove duplicate entries if they exist duplicates = self[:revision_sidecars]. select(:revision_guid, :name). group(:revision_guid, :name). @@ -16,19 +18,42 @@ 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_unique_constraint(%i[revision_guid name], - name: :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 - alter_table(:revision_sidecars) do - drop_constraint(:revision_sidecars_revision_guid_name_index, type: :unique) if @db.indexes(:revision_sidecars).key?(:revision_sidecars_revision_guid_name_index) + 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