Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/models/runtime/buildpack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
27 changes: 20 additions & 7 deletions spec/unit/models/runtime/buildpack_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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') }

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading