Replacing validate uniqueness with db constraint on Sidecar model (#4954)

* feat: unique constraint added on db level for sidecar table

- db migration added on columns app_guid and name. Unit testting added too
- sequel validate uniqueness removed from model and around save added instead

* feat: concurrent migration for postgres db

* fix: typo fixed

* fix: comments are fixed

Author: serdarozerr

app/models/runtime/sidecar_model.rb

@@ -16,12 +16,20 @@ class SidecarModel < Sequel::Model(:sidecars)
                 key: :sidecar_guid,
                 primary_key: :guid
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('sidecars_app_guid_name_index')
+
+      errors.add(%i[app_guid name], Sequel.lit("Sidecar with name '#{name}' already exists for given app"))
+      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[app_guid name], message: Sequel.lit("Sidecar with name '#{name}' already exists for given app")
     end
   end
 end

db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb

@@ -0,0 +1,59 @@
+Sequel.migration do
+  no_transaction # required for concurrently option on postgres
+
+  up do
+    transaction do
+      duplicates = self[:sidecars].
+                   select(:app_guid, :name).
+                   group(:app_guid, :name).
+                   having { count(1) > 1 }
+
+      duplicates.each do |dup|
+        ids_to_remove = self[:sidecars].
+                        where(app_guid: dup[:app_guid], name: dup[:name]).
+                        select(:id).
+                        order(:id).
+                        offset(1).
+                        map(:id)
+
+        self[:sidecars].where(id: ids_to_remove).delete
+      end
+    end
+
+    if database_type == :postgres
+      VCAP::Migration.with_concurrent_timeout(self) do
+        add_index :sidecars, %i[app_guid name],
+                  name: :sidecars_app_guid_name_index,
+                  unique: true,
+                  concurrently: true,
+                  if_not_exists: true
+      end
+    else
+      alter_table(:sidecars) do
+        # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations
+        unless @db.indexes(:sidecars).key?(:sidecars_app_guid_name_index)
+          add_index %i[app_guid name], unique: true,
+                                       name: :sidecars_app_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 :sidecars, nil,
+                   name: :sidecars_app_guid_name_index,
+                   concurrently: true,
+                   if_exists: true
+      end
+    else
+      alter_table(:sidecars) do
+        # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations
+        drop_index %i[app_guid name], name: :sidecars_app_guid_name_index if @db.indexes(:sidecars).key?(:sidecars_app_guid_name_index)
+        # rubocop:enable Sequel/ConcurrentIndex
+      end
+    end
+  end
+end

spec/migrations/20260323092954_add_unique_constraint_to_sidecars_spec.rb

@@ -0,0 +1,34 @@
+require 'spec_helper'
+require 'migrations/helpers/migration_shared_context'
+RSpec.describe 'add unique constraint to sidecars', isolation: :truncation, type: :migration do
+  include_context 'migration' do
+    let(:migration_filename) { '20260323092954_add_unique_constraint_to_sidecars.rb' }
+  end
+
+  let!(:app) { VCAP::CloudController::AppModel.make }
+
+  it 'remove duplicates, add constraint and revert migration' do
+    # create duplicate entries
+    db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid)
+    db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid)
+    expect(db[:sidecars].where(name: 'app', app_guid: app.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[:sidecars].where(name: 'app', app_guid: app.guid).count).to eq(1)
+    expect(db.indexes(:sidecars)).to include(:sidecars_app_guid_name_index)
+    expect { db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.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(:sidecars)).not_to include(:sidecars_app_guid_name_index)
+    expect { db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid) }.not_to raise_error
+  end
+end

spec/unit/models/runtime/sidecar_model_spec.rb

@@ -14,12 +14,24 @@ module VCAP::CloudController
     end
 
     describe 'validations' do
+      it { is_expected.to validate_presence :name }
+      it { is_expected.to validate_presence :command }
+    end
+
+    describe 'sidecar model #around_save' do
       let(:app_model) { AppModel.make }
-      let!(:sidecar) { SidecarModel.make(name: 'my_sidecar', app: app_model) }
+      let!(:sidecar) { SidecarModel.make(app_guid: app_model.guid, name: 'sidecar1', command: 'exec') }
 
-      it 'validates unique sidecar name per app' do
-        expect { SidecarModel.create(app: app_model, name: 'my_sidecar', command: 'some-command') }.
-          to raise_error(Sequel::ValidationFailed, /Sidecar with name 'my_sidecar' already exists for given app/)
+      it 'raises validation error on unique constraint violation for sidecar' do
+        expect do
+          SidecarModel.create(guid: SecureRandom.uuid, app_guid: app_model.guid, name: sidecar.name, command: 'exec')
+        end.to raise_error(Sequel::ValidationFailed) { |error| expect(error.message).to include("Sidecar with name 'sidecar1' already exists for given app") }
+      end
+
+      it 'raises the original error on other unique constraint violations' do
+        expect do
+          SidecarModel.create(guid: sidecar.guid, app_guid: app_model.guid, name: 'sidecar2', command: 'exec')
+        end.to raise_error(Sequel::UniqueConstraintViolation)
       end
     end