From 4ca4cc3eaf63c94e0b9952d2ff9e9c6ff88c6e15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Thu, 19 Mar 2026 13:17:00 +0100 Subject: [PATCH 1/6] feat: db migrations created for creating unique on name stack lifecycle fields --- ...940_add_unique_constraint_to_buildpacks.rb | 41 +++++++++++ ...dd_unique_constraint_to_buildpacks_spec.rb | 69 +++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb create mode 100644 spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb diff --git a/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb b/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb new file mode 100644 index 00000000000..f72056dc6b4 --- /dev/null +++ b/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb @@ -0,0 +1,41 @@ +Sequel.migration do + 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 + + alter_table(:buildpacks) do + # rubocop:disable Sequel/ConcurrentIndex + 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 + alter_table(:buildpacks) do + # rubocop:disable Sequel/ConcurrentIndex + 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 diff --git a/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb b/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb new file mode 100644 index 00000000000..5980182c3d5 --- /dev/null +++ b/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb @@ -0,0 +1,69 @@ +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 'drops old index, adds new unique index, and handles idempotency' do + # Verify initial state - old index exists, new one does not + expect(db.indexes(:buildpacks)).to include(:unique_name_and_stack) + expect(db.indexes(:buildpacks)).not_to include(:buildpacks_name_stack_lifecycle_index) + + # === UP MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + + # 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 + expect(db.indexes(:buildpacks)).not_to include(:unique_name_and_stack) + expect(db.indexes(:buildpacks)).to include(:buildpacks_name_stack_lifecycle_index) + + # === DOWN MIGRATION === + 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) + + # 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) + expect(db.indexes(:buildpacks)).to include(:unique_name_and_stack) + end + + it 'removes duplicate entries before adding the unique index' 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) + + # Verify duplicates exist before migration + 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 new index exists + expect(db.indexes(:buildpacks)).to include(:buildpacks_name_stack_lifecycle_index) + end + end +end From 704ee032dc3dc68989fef0ec9b651699e9724f90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Thu, 19 Mar 2026 22:22:22 +0100 Subject: [PATCH 2/6] feat: validate_uniqueness removed from validate list, around saved added instead --- app/models/runtime/buildpack.rb | 10 +++++++- spec/unit/models/runtime/buildpack_spec.rb | 27 ++++++++++++++++------ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index 9dec15560c6..5299f3978e8 100644 --- a/app/models/runtime/buildpack.rb +++ b/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 diff --git a/spec/unit/models/runtime/buildpack_spec.rb b/spec/unit/models/runtime/buildpack_spec.rb index 200c8645a13..69c2d88cd91 100644 --- a/spec/unit/models/runtime/buildpack_spec.rb +++ b/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 is occures' do + let(:stack) { Stack.make } + + it 'raise 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 'raise 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 From 8235de764fb1fda9ddd24028e41768ab3692f07f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Fri, 20 Mar 2026 09:19:47 +0100 Subject: [PATCH 3/6] fix: number of tests decreased because of rubocop offense --- ...dd_unique_constraint_to_buildpacks_spec.rb | 60 +++++++++---------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb b/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb index 5980182c3d5..a424ba278b5 100644 --- a/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb +++ b/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb @@ -7,37 +7,7 @@ end describe 'buildpacks table' do - it 'drops old index, adds new unique index, and handles idempotency' do - # Verify initial state - old index exists, new one does not - expect(db.indexes(:buildpacks)).to include(:unique_name_and_stack) - expect(db.indexes(:buildpacks)).not_to include(:buildpacks_name_stack_lifecycle_index) - - # === UP MIGRATION === - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - - # 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 - expect(db.indexes(:buildpacks)).not_to include(:unique_name_and_stack) - expect(db.indexes(:buildpacks)).to include(:buildpacks_name_stack_lifecycle_index) - - # === DOWN MIGRATION === - 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) - - # 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) - expect(db.indexes(:buildpacks)).to include(:unique_name_and_stack) - end - - it 'removes duplicate entries before adding the unique index' 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 } @@ -49,7 +19,6 @@ 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) - # Verify duplicates exist before migration 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) @@ -62,8 +31,33 @@ 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 new index exists + # 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 From 97e3dee844fd710e31ceb08ced57d06d11eaeb2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 12:48:52 +0100 Subject: [PATCH 4/6] feat: concurrent migration for postgres db --- ...940_add_unique_constraint_to_buildpacks.rb | 41 ++++++++++++++++--- spec/unit/models/runtime/buildpack_spec.rb | 4 +- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb b/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb index f72056dc6b4..d894db5789d 100644 --- a/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb +++ b/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb @@ -1,4 +1,5 @@ Sequel.migration do + no_transaction # required for concurrently option on postgres up do transaction do # Remove duplicate entries if they exist @@ -17,9 +18,23 @@ 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 + # 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, @@ -31,11 +46,25 @@ end down do - alter_table(:buildpacks) do - # rubocop:disable Sequel/ConcurrentIndex - 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 + 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 diff --git a/spec/unit/models/runtime/buildpack_spec.rb b/spec/unit/models/runtime/buildpack_spec.rb index 69c2d88cd91..781143c21f9 100644 --- a/spec/unit/models/runtime/buildpack_spec.rb +++ b/spec/unit/models/runtime/buildpack_spec.rb @@ -81,7 +81,7 @@ def ordered_buildpacks context 'when unique constraint violation is occures' do let(:stack) { Stack.make } - it 'raise validation error when name, stack and lifecyle is same' do + 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') @@ -90,7 +90,7 @@ def ordered_buildpacks end end - it 'raise validation error different than the unique name, stack and lifecyle' do + 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 From ecaace48fb0d1c792005201650c34bf0131f285f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 12:58:58 +0100 Subject: [PATCH 5/6] fix: rubocop ofens is fixed --- ...083940_add_unique_constraint_to_buildpacks.rb | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb b/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb index d894db5789d..10e28885ee0 100644 --- a/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb +++ b/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb @@ -3,10 +3,7 @@ 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 = self[:buildpacks].select(:name, :stack, :lifecycle).group(:name, :stack, :lifecycle).having { count(1) > 1 } duplicates.each do |dup| ids_to_remove = self[:buildpacks]. @@ -48,15 +45,8 @@ 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 + 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 From 24cd710dd239dfb947f88253689ee2b5b43da10f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serdar=20=C3=96zer?= Date: Tue, 24 Mar 2026 14:59:45 +0100 Subject: [PATCH 6/6] fix: typo is fixed --- spec/unit/models/runtime/buildpack_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/models/runtime/buildpack_spec.rb b/spec/unit/models/runtime/buildpack_spec.rb index 781143c21f9..b21e910276a 100644 --- a/spec/unit/models/runtime/buildpack_spec.rb +++ b/spec/unit/models/runtime/buildpack_spec.rb @@ -78,7 +78,7 @@ def ordered_buildpacks end end - context 'when unique constraint violation is occures' do + context 'when unique constraint violation occures' do let(:stack) { Stack.make } it 'raises validation error when name, stack and lifecyle is same' do