Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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/sidecar_model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
33 changes: 33 additions & 0 deletions db/migrations/20260323092954_add_unique_constraint_to_sidecars.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
Sequel.migration do
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

alter_table(:sidecars) do
unless @db.indexes(:sidecars).key?(:sidecars_app_guid_name_index)
add_unique_constraint %i[app_guid name],
name: :sidecars_app_guid_name_index
end
end
end
end

down do
alter_table(:sidecars) do
drop_constraint(:sidecars_app_guid_name_index, type: :unique) if @db.indexes(:sidecars).key?(:sidecars_app_guid_name_index)
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
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 dublicates, add constraint and revert migration' do
Comment thread
serdarozerr marked this conversation as resolved.
Outdated
# =========================================================================================
# SETUP: Create duplicate entries to test the de-duplication logic.
# =========================================================================================
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)

# =========================================================================================
# UP MIGRATION: Run the migration to apply the unique constraints.
# ========================================================================================
Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)

# =========================================================================================
# ASSERT UP MIGRATION: Verify that duplicates are removed and constraints are 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)

# =========================================================================================
# TEST IDEMPOTENCY: 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

# =========================================================================================
# DOWN MIGRATION: Roll back the migration to remove the constraints.
# =========================================================================================
Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true)

# =========================================================================================
# ASSERT DOWN MIGRATION: Verify that constraints are 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
20 changes: 16 additions & 4 deletions spec/unit/models/runtime/sidecar_model_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Loading