diff --git a/app/models/runtime/revision_sidecar_process_type_model.rb b/app/models/runtime/revision_sidecar_process_type_model.rb index e19883c9e8f..f81f07281dd 100644 --- a/app/models/runtime/revision_sidecar_process_type_model.rb +++ b/app/models/runtime/revision_sidecar_process_type_model.rb @@ -6,11 +6,19 @@ class RevisionSidecarProcessTypeModel < Sequel::Model(:revision_sidecar_process_ key: :revision_sidecar_guid, without_guid_generation: true + def around_save + yield + rescue Sequel::UniqueConstraintViolation => e + raise e unless e.message.include?('revision_sidecar_process_types_revision_sidecar_guid_type_index') + + errors.add(%i[revision_sidecar_guid type], "Sidecar is already associated with process type #{type}") + raise Sequel::ValidationFailed.new(self) + end + def validate super validates_presence [:type] validates_max_length 255, :type, message: Sequel.lit('Process type is too long (maximum is 255 characters)') - validates_unique %i[revision_sidecar_guid type], message: Sequel.lit("Sidecar is already associated with process type #{type}") end end end diff --git a/app/models/runtime/sidecar_process_type_model.rb b/app/models/runtime/sidecar_process_type_model.rb index 0eb53936602..e84f7cdb42e 100644 --- a/app/models/runtime/sidecar_process_type_model.rb +++ b/app/models/runtime/sidecar_process_type_model.rb @@ -6,6 +6,15 @@ class SidecarProcessTypeModel < Sequel::Model(:sidecar_process_types) key: :sidecar_guid, without_guid_generation: true + def around_save + yield + rescue Sequel::UniqueConstraintViolation => e + raise e unless e.message.include?('sidecar_process_types_sidecar_guid_type_index') + + errors.add(%i[sidecar_guid type], "Sidecar is already associated with process type #{type}") + raise Sequel::ValidationFailed.new(self) + end + def validate super validates_presence [:type] diff --git a/db/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types.rb b/db/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types.rb new file mode 100644 index 00000000000..782a79c8c34 --- /dev/null +++ b/db/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types.rb @@ -0,0 +1,68 @@ +Sequel.migration do + up do + transaction do + # Clean up sidecar_process_types + duplicates = self[:sidecar_process_types]. + select(:sidecar_guid, :type). + group(:sidecar_guid, :type). + having { count(1) > 1 } + + duplicates.each do |dup| + pks_to_remove = self[:sidecar_process_types]. + where(sidecar_guid: dup[:sidecar_guid], type: dup[:type]). + select(:id). + order(:id). + offset(1). + map(:id) + + self[:sidecar_process_types].where(id: pks_to_remove).delete + end + + alter_table(:sidecar_process_types) do + unless @db.indexes(:sidecar_process_types).key?(:sidecar_process_types_sidecar_guid_type_index) + add_unique_constraint %i[sidecar_guid type], + name: :sidecar_process_types_sidecar_guid_type_index + end + end + end + + transaction do + # Clean up revision_sidecar_process_types + duplicates = self[:revision_sidecar_process_types]. + select(:revision_sidecar_guid, :type). + group(:revision_sidecar_guid, :type). + having { count(1) > 1 } + + duplicates.each do |dup| + pks_to_remove = self[:revision_sidecar_process_types]. + where(revision_sidecar_guid: dup[:revision_sidecar_guid], type: dup[:type]). + select(:id). + order(:id). + offset(1). + map(:id) + + self[:revision_sidecar_process_types].where(id: pks_to_remove).delete + end + + alter_table(:revision_sidecar_process_types) do + unless @db.indexes(:revision_sidecar_process_types).key?(:revision_sidecar_process_types_revision_sidecar_guid_type_index) + add_unique_constraint %i[revision_sidecar_guid type], + name: :revision_sidecar_process_types_revision_sidecar_guid_type_index + end + end + end + end + + down do + alter_table(:sidecar_process_types) do + drop_constraint(:sidecar_process_types_sidecar_guid_type_index, type: :unique) if @db.indexes(:sidecar_process_types).key?(:sidecar_process_types_sidecar_guid_type_index) + end + + alter_table(:revision_sidecar_process_types) do + if @db.indexes(:revision_sidecar_process_types).key?(:revision_sidecar_process_types_revision_sidecar_guid_type_index) + drop_constraint(:revision_sidecar_process_types_revision_sidecar_guid_type_index, + type: :unique) + end + end + end +end diff --git a/spec/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types_spec.rb b/spec/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types_spec.rb new file mode 100644 index 00000000000..51f9ea639dc --- /dev/null +++ b/spec/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types_spec.rb @@ -0,0 +1,61 @@ +require 'spec_helper' +require 'migrations/helpers/migration_shared_context' + +RSpec.describe 'add unique constraint to sidecar process types', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20251030100000_add_unique_constraint_to_sidecar_process_types.rb' } + end + + let!(:app) { VCAP::CloudController::AppModel.make } + let!(:sidecar) { VCAP::CloudController::SidecarModel.make(app:) } + let!(:revision) { VCAP::CloudController::RevisionModel.make(app:) } + let!(:revision_sidecar) { VCAP::CloudController::RevisionSidecarModel.make(revision:) } + + it 'removes duplicates, adds unique constraints, and is reversible' do + # ========================================================================================= + # SETUP: Create duplicate entries for both tables to test the de-duplication logic. + # ========================================================================================= + db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) + db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) + expect(db[:sidecar_process_types].where(sidecar_guid: sidecar.guid, type: 'web').count).to eq(2) + + db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) + db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) + expect(db[:revision_sidecar_process_types].where(revision_sidecar_guid: revision_sidecar.guid, type: 'worker').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[:sidecar_process_types].where(sidecar_guid: sidecar.guid, type: 'web').count).to eq(1) + expect(db.indexes(:sidecar_process_types)).to include(:sidecar_process_types_sidecar_guid_type_index) + expect { db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) }.to raise_error(Sequel::UniqueConstraintViolation) + + expect(db[:revision_sidecar_process_types].where(revision_sidecar_guid: revision_sidecar.guid, type: 'worker').count).to eq(1) + expect(db.indexes(:revision_sidecar_process_types)).to include(:revision_sidecar_process_types_revision_sidecar_guid_type_index) + expect { db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) }.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(:sidecar_process_types)).not_to include(:sidecar_process_types_sidecar_guid_type_index) + expect { db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) }.not_to raise_error + + expect(db.indexes(:revision_sidecar_process_types)).not_to include(:revision_sidecar_process_types_revision_sidecar_guid_type_index) + expect { db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) }.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 0340c9c462e..e2305b8e94a 100644 --- a/spec/unit/models/runtime/revision_sidecar_model_spec.rb +++ b/spec/unit/models/runtime/revision_sidecar_model_spec.rb @@ -24,5 +24,24 @@ module VCAP::CloudController }) end end + + describe 'revision_sidecar_process_types: #around_save' do + it 'raises validation error on unique constraint violation for sidecar_process_types' do + expect do + expect(RevisionSidecarProcessTypeModel.where(revision_sidecar: revision_sidecar, type: 'web').count).to eq(1) + RevisionSidecarProcessTypeModel.create(revision_sidecar: revision_sidecar, type: 'web', guid: SecureRandom.uuid) + end.to raise_error(Sequel::ValidationFailed) { |error| + expect(error.message).to include('Sidecar is already associated with process type web') + } + end + + it 'raises original error on other unique constraint violations' do + expect do + expect(RevisionSidecarProcessTypeModel.where(revision_sidecar: revision_sidecar, type: 'web').count).to eq(1) + RevisionSidecarProcessTypeModel.create(revision_sidecar: revision_sidecar, type: 'worker', + guid: RevisionSidecarProcessTypeModel.where(revision_sidecar: revision_sidecar, type: 'web').first.guid) + end.to raise_error(Sequel::UniqueConstraintViolation) + end + end end end diff --git a/spec/unit/models/runtime/sidecar_model_spec.rb b/spec/unit/models/runtime/sidecar_model_spec.rb index c4873c293b4..c3444664f14 100644 --- a/spec/unit/models/runtime/sidecar_model_spec.rb +++ b/spec/unit/models/runtime/sidecar_model_spec.rb @@ -36,5 +36,28 @@ module VCAP::CloudController }) end end + + describe 'sidecar_process_types: #around_save' do + let(:sidecar) { SidecarModel.make } + let(:app) { AppModel.make } + + it 'raises validation error on unique constraint violation for sidecar_process_types' do + SidecarProcessTypeModel.create(sidecar: sidecar, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) + + expect do + SidecarProcessTypeModel.create(sidecar: sidecar, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) + end.to raise_error(Sequel::ValidationFailed) { |error| + expect(error.message).to include('Sidecar is already associated with process type web') + } + end + + it 'raises original error on other unique constraint violations' do + same_guid = SecureRandom.uuid + SidecarProcessTypeModel.create(sidecar: sidecar, type: 'web', app_guid: app.guid, guid: same_guid) + expect do + SidecarProcessTypeModel.create(sidecar: sidecar, type: 'worker', app_guid: app.guid, guid: same_guid) + end.to raise_error(Sequel::UniqueConstraintViolation) + end + end end end