Replacing validate uniqueness with db constraint on Buildpack model (#4942)

* feat: db migrations created for creating unique on name stack lifecycle fields

* feat: validate_uniqueness removed from validate list, around saved added instead

* fix: number of tests decreased because of rubocop offense

* feat: concurrent migration for postgres db

* fix: rubocop ofens is fixed

* fix: typo is fixed

Author: serdarozerr

app/models/runtime/buildpack.rb

@@ -43,8 +43,16 @@ def self.at_last_position
       where(position: max(:position)).first
     end
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('buildpacks_name_stack_lifecycle_index')
+
+      errors.add(%i[name stack lifecycle], :unique)
+      raise validation_failed_error
+    end
+
     def validate
-      validates_unique %i[name stack lifecycle]
       validates_format(/\A(\w|-)+\z/, :name, message: 'can only contain alphanumeric characters')
 
       validate_stack_existence

db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb

@@ -0,0 +1,60 @@
+Sequel.migration do
+  no_transaction # required for concurrently option on postgres
+  up do
+    transaction do
+      # Remove duplicate entries if they exist
+      duplicates = self[:buildpacks].select(:name, :stack, :lifecycle).group(:name, :stack, :lifecycle).having { count(1) > 1 }
+
+      duplicates.each do |dup|
+        ids_to_remove = self[:buildpacks].
+                        where(name: dup[:name], stack: dup[:stack], lifecycle: dup[:lifecycle]).
+                        select(:id).
+                        order(:id).
+                        offset(1).
+                        map(:id)
+
+        self[:buildpacks].where(id: ids_to_remove).delete
+      end
+    end
+
+    if database_type == :postgres
+      VCAP::Migration.with_concurrent_timeout(self) do
+        drop_index :buildpacks, nil,
+                   name: :unique_name_and_stack,
+                   concurrently: true,
+                   if_exists: true
+        add_index :buildpacks, %i[name stack lifecycle],
+                  name: :buildpacks_name_stack_lifecycle_index,
+                  unique: true,
+                  concurrently: true,
+                  if_not_exists: true
+      end
+    else
+      alter_table(:buildpacks) do
+        # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations
+        drop_index %i[name stack], name: :unique_name_and_stack if @db.indexes(:buildpacks).key?(:unique_name_and_stack)
+        unless @db.indexes(:buildpacks).key?(:buildpacks_name_stack_lifecycle_index)
+          add_index %i[name stack lifecycle], unique: true,
+                                              name: :buildpacks_name_stack_lifecycle_index
+        end
+        # rubocop:enable Sequel/ConcurrentIndex
+      end
+    end
+  end
+
+  down do
+    if database_type == :postgres
+      VCAP::Migration.with_concurrent_timeout(self) do
+        drop_index :buildpacks, nil, name: :buildpacks_name_stack_lifecycle_index, concurrently: true, if_exists: true
+        add_index :buildpacks, %i[name stack], name: :unique_name_and_stack, unique: true, concurrently: true, if_not_exists: true
+      end
+    else
+      alter_table(:buildpacks) do
+        # rubocop:disable Sequel/ConcurrentIndex -- MySQL does not support concurrent index operations
+        drop_index %i[name stack lifecycle], name: :buildpacks_name_stack_lifecycle_index if @db.indexes(:buildpacks).key?(:buildpacks_name_stack_lifecycle_index)
+        add_index %i[name stack], unique: true, name: :unique_name_and_stack unless @db.indexes(:buildpacks).key?(:unique_name_and_stack)
+        # rubocop:enable Sequel/ConcurrentIndex
+      end
+    end
+  end
+end

spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb

@@ -0,0 +1,63 @@
+require 'spec_helper'
+require 'migrations/helpers/migration_shared_context'
+
+RSpec.describe 'add unique constraint to buildpacks', isolation: :truncation, type: :migration do
+  include_context 'migration' do
+    let(:migration_filename) { '20260318083940_add_unique_constraint_to_buildpacks.rb' }
+  end
+
+  describe 'buildpacks table' do
+    it 'removes duplicates, swaps indexes, and handles idempotency' do
+      # Drop old unique index so we can insert duplicates
+      db.alter_table(:buildpacks) { drop_index %i[name stack], name: :unique_name_and_stack }
+
+      db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 1)
+      db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 2)
+      db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'cnb', position: 3)
+      db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs4', lifecycle: 'buildpack', position: 4)
+      db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'go', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 5)
+      db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'go', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 6)
+      db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'go', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 7)
+
+      expect(db[:buildpacks].where(name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack').count).to eq(2)
+      expect(db[:buildpacks].where(name: 'go', stack: 'cflinuxfs3', lifecycle: 'buildpack').count).to eq(3)
+
+      # === UP MIGRATION ===
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
+
+      # Verify duplicates are removed, keeping one per (name, stack, lifecycle)
+      expect(db[:buildpacks].where(name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack').count).to eq(1)
+      expect(db[:buildpacks].where(name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'cnb').count).to eq(1)
+      expect(db[:buildpacks].where(name: 'ruby', stack: 'cflinuxfs4', lifecycle: 'buildpack').count).to eq(1)
+      expect(db[:buildpacks].where(name: 'go', stack: 'cflinuxfs3', lifecycle: 'buildpack').count).to eq(1)
+
+      # Verify old index is dropped and new index is added
+      expect(db.indexes(:buildpacks)).not_to include(:unique_name_and_stack)
+      expect(db.indexes(:buildpacks)).to include(:buildpacks_name_stack_lifecycle_index)
+
+      # Test up migration idempotency
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
+
+      # === DOWN MIGRATION ===
+      # First remove test data that would conflict with the old (name, stack) unique index
+      db[:buildpacks].delete
+
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error
+
+      # Verify new index is dropped and old index is restored
+      expect(db.indexes(:buildpacks)).not_to include(:buildpacks_name_stack_lifecycle_index)
+      expect(db.indexes(:buildpacks)).to include(:unique_name_and_stack)
+
+      # Verify the restored index enforces uniqueness on (name, stack)
+      db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 1)
+      expect do
+        db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'cnb', position: 2)
+      end.to raise_error(Sequel::UniqueConstraintViolation)
+      db[:buildpacks].delete
+
+      # Test down migration idempotency
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error
+      expect(db.indexes(:buildpacks)).not_to include(:buildpacks_name_stack_lifecycle_index)
+    end
+  end
+end

spec/unit/models/runtime/buildpack_spec.rb

@@ -9,8 +9,6 @@ def ordered_buildpacks
     it { is_expected.to have_timestamp_columns }
 
     describe 'Validations' do
-      it { is_expected.to validate_uniqueness %i[name stack lifecycle] }
-
       describe 'stack' do
         let(:stack) {  Stack.make(name: 'happy') }
 
@@ -37,11 +35,6 @@ def ordered_buildpacks
           expect(buildpack.errors.on(:stack)).to include(:buildpack_stack_does_not_exist)
         end
 
-        it 'validates duplicate buildpacks with the same name and stack' do
-          Buildpack.create(name: 'oscar', stack: stack.name)
-          expect(Buildpack.new(name: 'oscar', stack: stack.name)).not_to be_valid
-        end
-
         it 'validates duplicate buildpacks with the same name and nil stack' do
           Buildpack.create(name: 'oscar', stack: nil)
           expect(Buildpack.new(name: 'oscar', stack: nil)).not_to be_valid
@@ -84,6 +77,26 @@ def ordered_buildpacks
           expect(buildpack.errors.on(:lifecycle)).to include(:buildpack_cant_change_lifecycle)
         end
       end
+
+      context 'when unique constraint violation occures' do
+        let(:stack) { Stack.make }
+
+        it 'raises validation error when name, stack and lifecyle is same' do
+          Buildpack.create(name: 'oscar', stack: stack.name, lifecycle: 'cnb')
+          expect do
+            Buildpack.create(name: 'oscar', stack: stack.name, lifecycle: 'cnb')
+          end.to raise_error(Sequel::ValidationFailed, /unique/) do |e|
+            expect(e.errors.on(%i[name stack lifecycle])).to include(:unique)
+          end
+        end
+
+        it 'raises validation error different than the unique name, stack and lifecyle' do
+          existing = Buildpack.create(name: 'oscar', stack: stack.name, lifecycle: 'cnb')
+          duplicate_guid = Buildpack.new(name: 'other', stack: stack.name, lifecycle: 'cnb')
+          duplicate_guid.guid = existing.guid
+          expect { duplicate_guid.save }.to raise_error(Sequel::UniqueConstraintViolation)
+        end
+      end
     end
 
     describe 'Serialization' do