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/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..a424ba278b5 --- /dev/null +++ b/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 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