fix test issues

Author: johha

db/migrations/20250731071027_allow_multiple_service_bindings.rb

@@ -2,32 +2,62 @@
   no_transaction # adding an index concurrently cannot be done within a transaction
 
   up do
-    transaction do
-      alter_table :service_bindings do
-        drop_constraint :unique_service_binding_service_instance_guid_app_guid if @db.indexes(:service_bindings).include?(:unique_service_binding_service_instance_guid_app_guid)
+    db_type = database_type
+    if indexes(:service_bindings)[:unique_service_binding_service_instance_guid_app_guid].present?
+      alter_table(:service_bindings) do
+        if db_type == :postgres
+          drop_constraint(:unique_service_binding_service_instance_guid_app_guid)
+        elsif db_type == :mysql
+          drop_constraint(:unique_service_binding_service_instance_guid_app_guid, unique: true)
+        end
 
-        drop_constraint :unique_service_binding_app_guid_name if @db.indexes(:service_bindings).include?(:unique_service_binding_app_guid_name)
       end
     end
+    alter_table(:service_bindings) { drop_constraint(:unique_service_binding_app_guid_name) } if indexes(:service_bindings)[:unique_service_binding_app_guid_name].present?
 
-    VCAP::Migration.with_concurrent_timeout(self) do
-      add_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_not_exists: true, concurrently: true
+    if db_type == :postgres
+      VCAP::Migration.with_concurrent_timeout(self) do
+        add_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, concurrently: true, if_not_exists: true
+        add_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, concurrently: true, if_not_exists: true
+      end
+    elsif db_type == :mysql
+      alter_table(:service_bindings) do
+        unless @db.indexes(:service_bindings).key?(:service_bindings_app_guid_service_instance_guid_index)
+          add_index %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, concurrently: true
+        end
+        unless @db.indexes(:service_bindings).key?(:service_bindings_app_guid_name_index)
+          add_index %i[app_guid name], name: :service_bindings_app_guid_name_index, concurrently: true
+        end
+      end
     end
   end
 
   down do
-    transaction do
-      alter_table :service_bindings do
-        unless @db.indexes(:service_bindings).include?(:unique_service_binding_service_instance_guid_app_guid)
-          add_unique_constraint %i[service_instance_guid app_guid],
-                                name: :unique_service_binding_service_instance_guid_app_guid
-        end
-        add_unique_constraint %i[app_guid name], name: :unique_service_binding_app_guid_name unless @db.indexes(:service_bindings).include?(:unique_service_binding_app_guid_name)
+    if indexes(:service_bindings)[:unique_service_binding_service_instance_guid_app_guid].blank?
+      alter_table(:service_bindings) do
+        add_unique_constraint %i[service_instance_guid app_guid],
+                              name: :unique_service_binding_service_instance_guid_app_guid
+      end
+    end
+    if indexes(:service_bindings)[:unique_service_binding_app_guid_name].blank?
+      alter_table(:service_bindings) do
+        add_unique_constraint %i[app_guid name], name: :unique_service_binding_app_guid_name
       end
     end
 
-    VCAP::Migration.with_concurrent_timeout(self) do
-      drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_exists: true, concurrently: true
+    if database_type == :postgres
+      VCAP::Migration.with_concurrent_timeout(self) do
+        drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, concurrently: true, if_exists: true
+        drop_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, concurrently: true, if_exists: true
+      end
+    elsif database_type == :mysql
+      alter_table(:service_bindings) do
+        if @db.indexes(:service_bindings).key?(:service_bindings_app_guid_service_instance_guid_index)
+          drop_index %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index,
+                                                         concurrently: true
+        end
+        drop_index %i[app_guid name], name: :service_bindings_app_guid_name_index, concurrently: true if @db.indexes(:service_bindings).key?(:service_bindings_app_guid_name_index)
+      end
     end
   end
 end

spec/migrations/20250731071027_allow_multiple_service_bindings_spec.rb

@@ -12,13 +12,15 @@
         expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid)
         expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name)
         expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index)
+        expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index)
       end
 
       it 'migrates successfully' do
         expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
         expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_app_guid_name)
         expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_service_instance_guid_app_guid)
         expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_service_instance_guid_index)
+        expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_name_index)
       end
 
       it 'does not fail if indexes/constraints are already in desired state' do
@@ -27,6 +29,7 @@
           drop_constraint :unique_service_binding_app_guid_name
         end
         db.add_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_not_exists: true, concurrently: true
+        db.add_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_not_exists: true, concurrently: true
 
         expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
       end
@@ -38,6 +41,7 @@
         expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid)
         expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name)
         expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index)
+        expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index)
       end
 
       it 'does not fail if indexes/constraints are already in desired state' do
@@ -47,6 +51,7 @@
           add_unique_constraint %i[app_guid name], name: :unique_service_binding_app_guid_name
         end
         db.drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_exists: true, concurrently: true
+        db.drop_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_exists: true, concurrently: true
 
         expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error
       end

spec/request/service_credential_bindings_spec.rb

@@ -1274,18 +1274,24 @@ def check_filtered_bindings(*bindings)
                                                                }))
         end
 
-        it 'returns 422 when the binding already exists' do
-          api_call.call admin_headers
-          expect(last_response.status).to eq(201).or eq(202)
+        context 'when only one binding per app and service instance is allowed' do
+          before do
+            TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1)
 
-          api_call.call admin_headers
+            it 'returns 422 when the binding already exists' do
+              api_call.call admin_headers
+              expect(last_response.status).to eq(201).or eq(202)
 
-          expect(last_response).to have_status_code(422)
-          expect(parsed_response['errors']).to include(include({
-                                                                 'detail' => include('The app is already bound to the service instance'),
-                                                                 'title' => 'CF-UnprocessableEntity',
-                                                                 'code' => 10_008
-                                                               }))
+              api_call.call admin_headers
+
+              expect(last_response).to have_status_code(422)
+              expect(parsed_response['errors']).to include(include({
+                                                                     'detail' => include('The app is already bound to the service instance'),
+                                                                     'title' => 'CF-UnprocessableEntity',
+                                                                     'code' => 10_008
+                                                                   }))
+            end
+          end
         end
 
         context 'when the service instance does not exist' do

spec/unit/controllers/runtime/apps_controller_spec.rb

@@ -2496,18 +2496,6 @@ def delete_app
           ServiceBinding.make(service_instance: si2, app: process2.app, name: 'free')
         end
 
-        it 'binding si2 to process1 with a name in use by process1 is not ok' do
-          expect do
-            ServiceBinding.make(service_instance: si2, app: process1.app, name: 'out')
-          end.to raise_error(Sequel::ValidationFailed, /App binding names must be unique\./)
-        end
-
-        it 'binding si1 to process1 with a new name is not ok' do
-          expect do
-            ServiceBinding.make(service_instance: si1, app: process1.app, name: 'gravy')
-          end.to raise_error(Sequel::ValidationFailed, 'The app is already bound to the service.')
-        end
-
         it 'binding si2 to process1 with a name in use by process2 is ok' do
           ServiceBinding.make(service_instance: si2, app: process1.app, name: 'free')
           get "/v2/apps/#{process1.app.guid}/service_bindings?results-per-page=2&page=1&q=name:free"

spec/unit/models/services/service_binding_spec.rb

@@ -25,7 +25,6 @@ module VCAP::CloudController
       it { is_expected.to validate_presence :app }
       it { is_expected.to validate_presence :service_instance }
       it { is_expected.to validate_db_presence :credentials }
-      it { is_expected.to validate_uniqueness %i[app_guid service_instance_guid], message: 'The app is already bound to the service.' }
       it { is_expected.to validate_presence [:type] }
 
       it 'validates max length of name' do
@@ -80,19 +79,6 @@ module VCAP::CloudController
         let(:app) { AppModel.make }
         let(:service_instance) { ServiceInstance.make(space: app.space) }
 
-        context 'and the name is not null' do
-          let(:existing_binding) do
-            ServiceBinding.make(app: app, name: 'some-name', service_instance: service_instance, type: 'app')
-          end
-
-          it 'adds a uniqueness error' do
-            other_service_instance = ServiceInstance.make(space: existing_binding.space)
-            conflict = ServiceBinding.new(app: existing_binding.app, name: existing_binding.name, service_instance: other_service_instance, type: 'app')
-            expect(conflict.valid?).to be(false)
-            expect(conflict.errors.full_messages).to eq(['The binding name is invalid. App binding names must be unique. The app already has a binding with name \'some-name\'.'])
-          end
-        end
-
         context 'and the name is null' do
           let(:existing_binding) do
             ServiceBinding.make(app: app, name: nil, service_instance: service_instance, type: 'app')