Replacing validate uniqueness with db constraint on RevisionProcessCommand model (#4956)

* 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

* fix: validates_unique in service_key's validate method is removed. The tests for these already refactored in PR 4930

* fix: add validates_unique added back because of the http call in service_key_create action. If key is dublicated prevent http call

* fix: testing name and service_instance_id uniqueness in dedicated test

* feat: concurrent migration for postgres db

* fix: comments are fixed

Author: serdarozerr

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

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

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

spec/migrations/20260323144429_add_unique_constraint_to_revision_process_commands_spec.rb

@@ -0,0 +1,39 @@
+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
+    # create duplicate entries
+    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)
+
+    # run the migration
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
+
+    # verify duplicates are removed and constraint is 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)
+
+    # 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
+
+    # roll back the migration
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true)
+
+    # verify constraint is 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

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