diff --git a/app/models/runtime/revision_process_command_model.rb b/app/models/runtime/revision_process_command_model.rb index 30bf774b01b..b06659f27ac 100644 --- a/app/models/runtime/revision_process_command_model.rb +++ b/app/models/runtime/revision_process_command_model.rb @@ -6,9 +6,13 @@ class RevisionProcessCommandModel < Sequel::Model(:revision_process_commands) key: :revision_guid, without_guid_generation: true - def validate - super - validates_unique(%i[revision_guid process_type]) + def around_save + yield + rescue Sequel::UniqueConstraintViolation => e + raise e unless e.message.include?('revision_process_commands_revision_guid_process_type_index') + + errors.add(%i[revision_guid process_type], Sequel.lit("Process type '#{process_type}' already exists for given revision")) + raise validation_failed_error end end end diff --git a/app/models/services/service_key.rb b/app/models/services/service_key.rb index 0cc8545b3f1..ae3e02f699a 100644 --- a/app/models/services/service_key.rb +++ b/app/models/services/service_key.rb @@ -49,6 +49,7 @@ def around_save def validate validates_presence :name validates_presence :service_instance + # Keep validates_unique as a pre-guard to prevent broker HTTP call when key already exists validates_unique %i[name service_instance_id] return unless service_instance diff --git a/db/migrations/20260323144429_add_unique_constraint_to_revision_process_commands.rb b/db/migrations/20260323144429_add_unique_constraint_to_revision_process_commands.rb new file mode 100644 index 00000000000..e5e01c6cc8e --- /dev/null +++ b/db/migrations/20260323144429_add_unique_constraint_to_revision_process_commands.rb @@ -0,0 +1,61 @@ +Sequel.migration do # rubocop:disable Metrics/BlockLength + no_transaction # required for concurrently option on postgres + + up do + transaction do + duplicates = self[:revision_process_commands]. + select(:revision_guid, :process_type). + group(:revision_guid, :process_type). + having { count(1) > 1 } + + duplicates.each do |dup| + ids_to_remove = self[:revision_process_commands]. + where(revision_guid: dup[:revision_guid], process_type: dup[:process_type]). + select(:id). + order(:id). + offset(1). + map(:id) + self[:revision_process_commands].where(id: ids_to_remove).delete + end + end + + if database_type == :postgres + VCAP::Migration.with_concurrent_timeout(self) do + add_index :revision_process_commands, %i[revision_guid process_type], + name: :revision_process_commands_revision_guid_process_type_index, + unique: true, + concurrently: true, + if_not_exists: true + end + else + alter_table(:revision_process_commands) do + # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations + unless @db.indexes(:revision_process_commands).key?(:revision_process_commands_revision_guid_process_type_index) + add_index %i[revision_guid process_type], unique: true, + name: :revision_process_commands_revision_guid_process_type_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_process_commands, nil, + name: :revision_process_commands_revision_guid_process_type_index, + concurrently: true, + if_exists: true + end + else + alter_table(:revision_process_commands) do + # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations + if @db.indexes(:revision_process_commands).key?(:revision_process_commands_revision_guid_process_type_index) + drop_index %i[revision_guid process_type], + name: :revision_process_commands_revision_guid_process_type_index + end + # rubocop:enable Sequel/ConcurrentIndex + end + end + end +end diff --git a/spec/migrations/20260323144429_add_unique_constraint_to_revision_process_commands_spec.rb b/spec/migrations/20260323144429_add_unique_constraint_to_revision_process_commands_spec.rb new file mode 100644 index 00000000000..8906c8aceca --- /dev/null +++ b/spec/migrations/20260323144429_add_unique_constraint_to_revision_process_commands_spec.rb @@ -0,0 +1,51 @@ +require 'spec_helper' +require 'migrations/helpers/migration_shared_context' + +RSpec.describe 'add unique constraint to revision_process_commands', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20260323144429_add_unique_constraint_to_revision_process_commands.rb' } + end + + let!(:revision) { VCAP::CloudController::RevisionModel.make } + + it 'removes duplicates, adds constraint and reverts migration' do + # ========================================================================================= + # SETUP: Create duplicate entries to test the de-duplication logic. + # ========================================================================================= + db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision.guid, process_type: 'worker') + db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision.guid, process_type: 'worker') + expect(db[:revision_process_commands].where(revision_guid: revision.guid, process_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[:revision_process_commands].where(revision_guid: revision.guid, process_type: 'worker').count).to eq(1) + expect(db.indexes(:revision_process_commands)).to include(:revision_process_commands_revision_guid_process_type_index) + expect do + db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision.guid, process_type: 'worker') + end.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_process_commands)).not_to include(:revision_process_commands_revision_guid_process_type_index) + expect do + db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision.guid, process_type: 'worker') + end.not_to raise_error + end +end diff --git a/spec/unit/models/runtime/revision_process_command_model_spec.rb b/spec/unit/models/runtime/revision_process_command_model_spec.rb new file mode 100644 index 00000000000..6484ab302f9 --- /dev/null +++ b/spec/unit/models/runtime/revision_process_command_model_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +module VCAP::CloudController + RSpec.describe RevisionProcessCommandModel do + describe 'revision_process_command_model #around_save' do + let(:revision) { RevisionModel.make } + let!(:revision_process_command) { RevisionProcessCommandModel.make(revision_guid: revision.guid, process_type: 'worker') } + + it 'raises validation error on unique constraint violation' do + expect do + RevisionProcessCommandModel.make( + revision_guid: revision_process_command.revision_guid, + process_type: revision_process_command.process_type + ) + end.to raise_error(Sequel::ValidationFailed) { |error| + expect(error.message).to include('already exists for given revision') + } + end + + it 'raises the original error on other unique constraint violations' do + expect do + RevisionProcessCommandModel.make(guid: revision_process_command.guid, revision_guid: revision_process_command.revision_guid, process_type: 'worker') + end.to raise_error(Sequel::UniqueConstraintViolation) + end + end + end +end