Add unique index for route bindings (#4629)

In case of concurrent requests/inserts duplicated entries in the `route_bindings` table might be created.
This change adds a new migration which removes duplicates and adds a unique constraint.
Additionally the error handling is adjusted so that `UniqueConstraintViolation` errors are rescued correctly.

Author: johha

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

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

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

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

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:)

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