Replacing validate uniqueness with db constraint on RevisionSidecar model (#4950)

* 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

* fix: migration files are renamed

* feat: concurrent migration for postgres db

* fix: typo fixed

* fix: comments are fixed

Author: serdarozerr

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

db/migrations/20260320141005_add_unique_constraint_to_revision_sidecars.rb

@@ -0,0 +1,59 @@
+Sequel.migration do
+  no_transaction # required for concurrently option on postgres
+
+  up do
+    transaction do
+      # remove duplicate entries if they exist
+      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
+    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_index %i[revision_guid name], unique: true,
+                                            name: :revision_sidecars_revision_guid_name_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_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

spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb

@@ -0,0 +1,35 @@
+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_sidecars.rb' }
+  end
+
+  let!(:app) { VCAP::CloudController::AppModel.make }
+  let!(:revision) { VCAP::CloudController::RevisionModel.make(:app) }
+
+  it 'remove duplicates, add constraint and revert migration' do
+    # create duplicate entries
+    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)
+
+    # 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_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)
+
+    # 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_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

spec/unit/models/runtime/revision_sidecar_model_spec.rb

@@ -43,5 +43,26 @@ module VCAP::CloudController
         end.to raise_error(Sequel::UniqueConstraintViolation)
       end
     end
+
+    describe 'Validations' do
+      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: 'exec')
+        end.to raise_error(Sequel::UniqueConstraintViolation)
+      end
+    end
   end
 end