diff --git a/app/actions/service_route_binding_create.rb b/app/actions/service_route_binding_create.rb index d9e99495862..642efbecd42 100644 --- a/app/actions/service_route_binding_create.rb +++ b/app/actions/service_route_binding_create.rb @@ -37,6 +37,10 @@ def precursor(service_instance, route, message:) MetadataUpdate.update(b, message) end end + rescue Sequel::ValidationFailed => e + raise e unless e.errors.on(%i[route_id service_instance_id])&.include?(:unique) + + already_exists! end class RouteBindingAlreadyExists < StandardError; end diff --git a/app/models/services/route_binding.rb b/app/models/services/route_binding.rb index b24a3d128d4..eff3999fbdc 100644 --- a/app/models/services/route_binding.rb +++ b/app/models/services/route_binding.rb @@ -67,6 +67,15 @@ def save_with_new_operation(attributes, new_operation) self end + def around_save + yield + rescue Sequel::UniqueConstraintViolation => e + raise e unless e.message.include?('route_bindings_route_id_service_instance_id_index') + + errors.add(%i[route_id service_instance_id], :unique) + raise validation_failed_error + end + private def validate_routing_service diff --git a/db/migrations/20251028135214_route_bindings_unique_index.rb b/db/migrations/20251028135214_route_bindings_unique_index.rb new file mode 100644 index 00000000000..ed17516700b --- /dev/null +++ b/db/migrations/20251028135214_route_bindings_unique_index.rb @@ -0,0 +1,47 @@ +Sequel.migration do + up do + transaction do + # Remove duplicate entries if they exist + duplicates = self[:route_bindings]. + select(:route_id, :service_instance_id). + group(:route_id, :service_instance_id). + having { count(:id) > 1 } + + duplicates.each do |dup| + ids_to_remove = self[:route_bindings]. + where(route_id: dup[:route_id], service_instance_id: dup[:service_instance_id]). + select(:id). + order(:id). + offset(1). + map(:id) + + self[:route_bindings].where(id: ids_to_remove).delete + end + + alter_table(:route_bindings) do + # Cannot add unique constraint concurrently as it requires a transaction + # rubocop:disable Sequel/ConcurrentIndex + unless @db.indexes(:route_bindings).key?(:route_bindings_route_id_service_instance_id_index) + add_index %i[route_id service_instance_id], unique: true, + name: :route_bindings_route_id_service_instance_id_index + end + # rubocop:enable Sequel/ConcurrentIndex + end + end + end + + down do + # rubocop:disable Sequel/ConcurrentIndex + if database_type == :mysql + # MySQL replaces the auto generate 'route_id' index with 'route_bindings_route_id_service_instance_id_index' but does not re-create it during down migration + alter_table(:route_bindings) { add_index :route_id, name: :route_id unless @db.indexes(:route_bindings).key?(:route_id) } + end + alter_table(:route_bindings) do + if @db.indexes(:route_bindings).key?(:route_bindings_route_id_service_instance_id_index) + drop_index %i[route_id service_instance_id], unique: true, + name: :route_bindings_route_id_service_instance_id_index + end + end + # rubocop:enable Sequel/ConcurrentIndex + end +end diff --git a/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb b/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb new file mode 100644 index 00000000000..6b385e908e2 --- /dev/null +++ b/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb @@ -0,0 +1,69 @@ +require 'spec_helper' +require 'migrations/helpers/migration_shared_context' + +RSpec.describe 'route bindings unique index', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20251028135214_route_bindings_unique_index.rb' } + end + + let(:space) { VCAP::CloudController::Space.make } + let(:service_instance_1) { VCAP::CloudController::ServiceInstance.make(space:) } + let(:service_instance_2) { VCAP::CloudController::ServiceInstance.make(space:) } + let(:route_1) { VCAP::CloudController::Route.make(space:) } + let(:route_2) { VCAP::CloudController::Route.make(space:) } + + describe 'route_bindings table' do + context 'up migration' do + it 'is in the correct state before migration' do + expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) + end + + it 'removes duplicates and migrates successfully by adding unique index' do + db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + + # Count duplicates before migration + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(2) + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).count).to eq(3) + + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + + # Verify duplicates are removed after migration + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).count).to eq(1) + + # Verify index is added + expect(db.indexes(:route_bindings)).to include(:route_bindings_route_id_service_instance_id_index) + end + + it 'does not fail if indexes/constraints are already in desired state' do + db.alter_table(:route_bindings) { add_index %i[route_id service_instance_id], unique: true, name: :route_bindings_route_id_service_instance_id_index } + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + end + end + + context 'down migration' do + it 'rolls back successfully' do + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) + expect(db.indexes(:route_bindings)).to include(:route_id) if db.database_type == :mysql + end + + it 'does not fail if indexes/constraints are already in desired state' do + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + db.alter_table(:route_bindings) { drop_index %i[route_id service_instance_id], unique: true, name: :route_bindings_route_id_service_instance_id_index } + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + end + end + end +end diff --git a/spec/unit/actions/service_route_binding_create_spec.rb b/spec/unit/actions/service_route_binding_create_spec.rb index e5954c5f5b9..8fade3fbaca 100644 --- a/spec/unit/actions/service_route_binding_create_spec.rb +++ b/spec/unit/actions/service_route_binding_create_spec.rb @@ -164,6 +164,30 @@ module V3 end end + context 'ValidationFailed error handling' do + it 'raises a RouteBindingAlreadyExists error for concurrent creation' do + allow_any_instance_of(RouteBinding).to receive(:save_with_attributes_and_new_operation).and_raise( + Sequel::ValidationFailed.new(RouteBinding.new.tap do |b| + b.errors.add(%i[route_id service_instance_id], :unique) + end) + ) + + expect do + action.precursor(service_instance, route, message:) + end.to raise_error(ServiceRouteBindingCreate::RouteBindingAlreadyExists) + end + + it 'does not rescue other ValidationFailed errors' do + allow_any_instance_of(RouteBinding).to receive(:save_with_attributes_and_new_operation).and_raise( + Sequel::ValidationFailed.new(RouteBinding.new.tap { |b| b.errors.add(:some_field, :some_error) }) + ) + + expect do + action.precursor(service_instance, route, message:) + end.to raise_error(Sequel::ValidationFailed) + end + end + context 'route already bound to a different service instance' do it 'raises an error' do other_instance = UserProvidedServiceInstance.make(space:, route_service_url:) diff --git a/spec/unit/models/services/route_binding_spec.rb b/spec/unit/models/services/route_binding_spec.rb index 729a34f846d..c29255135a9 100644 --- a/spec/unit/models/services/route_binding_spec.rb +++ b/spec/unit/models/services/route_binding_spec.rb @@ -53,6 +53,35 @@ module VCAP::CloudController binding.valid? expect(binding.errors[:service_instance]).to eq [:space_mismatch] end + + context 'when a unique constraint violation occurs' do + let(:space) { Space.make } + let(:service_offering) { Service.make(requires: ['route_forwarding']) } + let(:service_plan) { ServicePlan.make(service: service_offering) } + let(:service_instance) { ManagedServiceInstance.make(space:, service_plan:) } + let(:route) { Route.make(space:) } + + before { RouteBinding.create(service_instance:, route:) } + + it 'raises a Sequel::ValidationFailed error when route_id and service_instance_id are not unique' do + duplicate_binding = RouteBinding.new(service_instance:, route:) + expect do + duplicate_binding.save + end.to raise_error(Sequel::ValidationFailed, /unique/) do |e| + expect(e.errors.on(%i[route_id service_instance_id])).to include(:unique) + end + end + + it 'raises other unique constraint violations normally' do + another_route = Route.make(space:) + binding_with_duplicate_guid = RouteBinding.new( + guid: RouteBinding.first.guid, + service_instance: service_instance, + route: another_route + ) + expect { binding_with_duplicate_guid.save }.to raise_error(Sequel::UniqueConstraintViolation) + end + end end describe '#save_with_new_operation' do