From 3f2255688f1b7e2ef57f2cd4c327030427a85ae0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Mon, 23 Mar 2026 16:21:07 +0100 Subject: [PATCH 1/5] feat: unique constraint added on revicion_process_commands table - unique constraint on revision_guid and process type columns are added on the table. - validate_uniqueness check in model side is removed. - respective test cases are added --- .../runtime/revision_process_command_model.rb | 10 ++-- ...constraint_to_revision_process_commands.rb | 35 +++++++++++++ ...raint_to_revision_process_commands_spec.rb | 51 +++++++++++++++++++ .../revision_process_command_model_spec.rb | 27 ++++++++++ 4 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 db/migrations/20260323144429_add_unique_constraint_to_revision_process_commands.rb create mode 100644 spec/migrations/20260323144429_add_unique_constraint_to_revision_process_commands_spec.rb create mode 100644 spec/unit/models/runtime/revision_process_command_model_spec.rb 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/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..81b1e78cbea --- /dev/null +++ b/db/migrations/20260323144429_add_unique_constraint_to_revision_process_commands.rb @@ -0,0 +1,35 @@ +Sequel.migration do + 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 + + alter_table(:revision_process_commands) do + unless @db.indexes(:revision_process_commands).key?(:revision_process_commands_revision_guid_process_type_index) + add_unique_constraint %i[revision_guid process_type], + name: :revision_process_commands_revision_guid_process_type_index + end + end + end + end + + down do + alter_table(:revision_process_commands) do + if @db.indexes(:revision_process_commands).key?(:revision_process_commands_revision_guid_process_type_index) + drop_constraint(:revision_process_commands_revision_guid_process_type_index, type: :unique) + 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 From 32360765a568549902b3f052b0aa5afb6d0839fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 09:39:21 +0100 Subject: [PATCH 2/5] fix: validates_unique in service_key's validate method is removed. The tests for these already refactored in PR 4930 --- app/models/services/service_key.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/services/service_key.rb b/app/models/services/service_key.rb index 0cc8545b3f1..cd33e12034f 100644 --- a/app/models/services/service_key.rb +++ b/app/models/services/service_key.rb @@ -49,7 +49,6 @@ def around_save def validate validates_presence :name validates_presence :service_instance - validates_unique %i[name service_instance_id] return unless service_instance From 815fe8c7778ba171074e4dbb0ce2386212499fe1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 11:03:26 +0100 Subject: [PATCH 3/5] fix: add validates_unique added back because of the http call in service_key_create action. If key is dublicated prevent http call --- app/models/services/service_key.rb | 2 ++ spec/unit/models/services/service_key_spec.rb | 10 +--------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/app/models/services/service_key.rb b/app/models/services/service_key.rb index cd33e12034f..ae3e02f699a 100644 --- a/app/models/services/service_key.rb +++ b/app/models/services/service_key.rb @@ -49,6 +49,8 @@ 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/spec/unit/models/services/service_key_spec.rb b/spec/unit/models/services/service_key_spec.rb index 4ddf0c50d0c..34257d3292a 100644 --- a/spec/unit/models/services/service_key_spec.rb +++ b/spec/unit/models/services/service_key_spec.rb @@ -19,18 +19,10 @@ module VCAP::CloudController it { is_expected.to have_associated :service_instance, associated_instance: ->(service_key) { ServiceInstance.make(space: service_key.space) } } end - describe 'uniqueness' do - it 'enforces uniqueness of name and service_instance_id' do - existing = ServiceKey.make - expect do - ServiceKey.make(name: existing.name, service_instance: existing.service_instance) - end.to raise_error(Sequel::ValidationFailed, /unique/) - end - end - describe 'Validations' do it { is_expected.to validate_presence :service_instance } it { is_expected.to validate_presence :name } + it { is_expected.to validate_uniqueness :name, { service_instance_id: ServiceInstance.make.id } } it { is_expected.to validate_db_presence :service_instance_id } it { is_expected.to validate_db_presence :credentials } From 431150c72a14de12e6a5010abc8018b83e132141 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 15:10:21 +0100 Subject: [PATCH 4/5] fix: testing name and service_instance_id uniqueness in dedicated test --- spec/unit/models/services/service_key_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/unit/models/services/service_key_spec.rb b/spec/unit/models/services/service_key_spec.rb index 34257d3292a..4ddf0c50d0c 100644 --- a/spec/unit/models/services/service_key_spec.rb +++ b/spec/unit/models/services/service_key_spec.rb @@ -19,10 +19,18 @@ module VCAP::CloudController it { is_expected.to have_associated :service_instance, associated_instance: ->(service_key) { ServiceInstance.make(space: service_key.space) } } end + describe 'uniqueness' do + it 'enforces uniqueness of name and service_instance_id' do + existing = ServiceKey.make + expect do + ServiceKey.make(name: existing.name, service_instance: existing.service_instance) + end.to raise_error(Sequel::ValidationFailed, /unique/) + end + end + describe 'Validations' do it { is_expected.to validate_presence :service_instance } it { is_expected.to validate_presence :name } - it { is_expected.to validate_uniqueness :name, { service_instance_id: ServiceInstance.make.id } } it { is_expected.to validate_db_presence :service_instance_id } it { is_expected.to validate_db_presence :credentials } From ffe557e018115c3c3f35a981ac6b94496974580f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 15:45:23 +0100 Subject: [PATCH 5/5] feat: concurrent migration for postgres db --- ...constraint_to_revision_process_commands.rb | 38 ++++++++++++++++--- 1 file changed, 32 insertions(+), 6 deletions(-) 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 index 81b1e78cbea..e5e01c6cc8e 100644 --- a/db/migrations/20260323144429_add_unique_constraint_to_revision_process_commands.rb +++ b/db/migrations/20260323144429_add_unique_constraint_to_revision_process_commands.rb @@ -1,4 +1,6 @@ -Sequel.migration do +Sequel.migration do # rubocop:disable Metrics/BlockLength + no_transaction # required for concurrently option on postgres + up do transaction do duplicates = self[:revision_process_commands]. @@ -15,20 +17,44 @@ 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_unique_constraint %i[revision_guid process_type], - name: :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 - alter_table(:revision_process_commands) do - if @db.indexes(:revision_process_commands).key?(:revision_process_commands_revision_guid_process_type_index) - drop_constraint(:revision_process_commands_revision_guid_process_type_index, type: :unique) + 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