Skip to content

Commit 2210e7b

Browse files
johhajohha
committed
fix test issues
1 parent ee0025f commit 2210e7b

File tree

5 files changed

+83
-55
lines changed

5 files changed

+83
-55
lines changed

‎db/migrations/20250731071027_allow_multiple_service_bindings.rb‎

Lines changed: 50 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,32 +2,66 @@
22
no_transaction # adding an index concurrently cannot be done within a transaction
33

44
up do
5-
transaction do
6-
alter_table :service_bindings do
7-
drop_constraint :unique_service_binding_service_instance_guid_app_guid if @db.indexes(:service_bindings).include?(:unique_service_binding_service_instance_guid_app_guid)
8-
9-
drop_constraint :unique_service_binding_app_guid_name if @db.indexes(:service_bindings).include?(:unique_service_binding_app_guid_name)
5+
if database_type == :mysql && server_version < 80_000 # MySQL versions < 8 cannot drop unique constraints directly
6+
alter_table(:service_bindings) do
7+
# rubocop:disable Sequel/ConcurrentIndex
8+
if @db.indexes(:service_bindings).key?(:unique_service_binding_service_instance_guid_app_guid)
9+
drop_index %i[service_instance_guid app_guid],
10+
name: :unique_service_binding_service_instance_guid_app_guid
11+
end
12+
drop_index %i[app_guid name], name: :unique_service_binding_app_guid_name if @db.indexes(:service_bindings).key?(:unique_service_binding_app_guid_name)
13+
# rubocop:enable Sequel/ConcurrentIndex
14+
end
15+
else
16+
alter_table(:service_bindings) do
17+
drop_constraint(:unique_service_binding_service_instance_guid_app_guid) if @db.indexes(:service_bindings).key?(:unique_service_binding_service_instance_guid_app_guid)
18+
drop_constraint(:unique_service_binding_app_guid_name) if @db.indexes(:service_bindings).key?(:unique_service_binding_app_guid_name)
1019
end
1120
end
1221

13-
VCAP::Migration.with_concurrent_timeout(self) do
14-
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
22+
if database_type == :postgres
23+
VCAP::Migration.with_concurrent_timeout(self) do
24+
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
25+
add_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, concurrently: true, if_not_exists: true
26+
end
27+
elsif database_type == :mysql
28+
alter_table(:service_bindings) do
29+
# rubocop:disable Sequel/ConcurrentIndex
30+
unless @db.indexes(:service_bindings).key?(:service_bindings_app_guid_service_instance_guid_index)
31+
add_index %i[app_guid service_instance_guid],
32+
name: :service_bindings_app_guid_service_instance_guid_index
33+
end
34+
add_index %i[app_guid name], name: :service_bindings_app_guid_name_index unless @db.indexes(:service_bindings).key?(:service_bindings_app_guid_name_index)
35+
# rubocop:enable Sequel/ConcurrentIndex
36+
end
1537
end
1638
end
1739

1840
down do
19-
transaction do
20-
alter_table :service_bindings do
21-
unless @db.indexes(:service_bindings).include?(:unique_service_binding_service_instance_guid_app_guid)
22-
add_unique_constraint %i[service_instance_guid app_guid],
23-
name: :unique_service_binding_service_instance_guid_app_guid
24-
end
25-
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)
41+
alter_table(:service_bindings) do
42+
if @db.indexes(:service_bindings)[:unique_service_binding_service_instance_guid_app_guid].blank?
43+
add_unique_constraint %i[service_instance_guid app_guid],
44+
name: :unique_service_binding_service_instance_guid_app_guid
2645
end
2746
end
47+
alter_table(:service_bindings) do
48+
add_unique_constraint %i[app_guid name], name: :unique_service_binding_app_guid_name if @db.indexes(:service_bindings)[:unique_service_binding_app_guid_name].blank?
49+
end
2850

29-
VCAP::Migration.with_concurrent_timeout(self) do
30-
drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_exists: true, concurrently: true
51+
if database_type == :postgres
52+
VCAP::Migration.with_concurrent_timeout(self) do
53+
drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, concurrently: true, if_exists: true
54+
drop_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, concurrently: true, if_exists: true
55+
end
56+
elsif database_type == :mysql
57+
alter_table(:service_bindings) do
58+
# rubocop:disable Sequel/ConcurrentIndex
59+
if @db.indexes(:service_bindings).key?(:service_bindings_app_guid_service_instance_guid_index)
60+
drop_index %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index
61+
end
62+
drop_index %i[app_guid name], name: :service_bindings_app_guid_name_index if @db.indexes(:service_bindings).key?(:service_bindings_app_guid_name_index)
63+
# rubocop:enable Sequel/ConcurrentIndex
64+
end
3165
end
3266
end
3367
end

‎spec/migrations/20250731071027_allow_multiple_service_bindings_spec.rb‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,22 +12,29 @@
1212
expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid)
1313
expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name)
1414
expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index)
15+
expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index)
1516
end
1617

1718
it 'migrates successfully' do
1819
expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
1920
expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_app_guid_name)
2021
expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_service_instance_guid_app_guid)
2122
expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_service_instance_guid_index)
23+
expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_name_index)
2224
end
2325

2426
it 'does not fail if indexes/constraints are already in desired state' do
2527
db.alter_table :service_bindings do
2628
drop_constraint :unique_service_binding_service_instance_guid_app_guid
2729
drop_constraint :unique_service_binding_app_guid_name
2830
end
29-
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
30-
31+
if db.database_type == :postgres
32+
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
33+
db.add_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_not_exists: true, concurrently: true
34+
else
35+
db.add_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_not_exists: true
36+
db.add_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_not_exists: true
37+
end
3138
expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
3239
end
3340
end
@@ -38,6 +45,7 @@
3845
expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid)
3946
expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name)
4047
expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index)
48+
expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index)
4149
end
4250

4351
it 'does not fail if indexes/constraints are already in desired state' do
@@ -46,7 +54,13 @@
4654
add_unique_constraint %i[service_instance_guid app_guid], name: :unique_service_binding_service_instance_guid_app_guid
4755
add_unique_constraint %i[app_guid name], name: :unique_service_binding_app_guid_name
4856
end
49-
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
57+
if db.database_type == :postgres
58+
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
59+
db.drop_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_exists: true, concurrently: true
60+
else
61+
db.drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_exists: true
62+
db.drop_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_exists: true
63+
end
5064

5165
expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error
5266
end

‎spec/request/service_credential_bindings_spec.rb‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1274,18 +1274,24 @@ def check_filtered_bindings(*bindings)
12741274
}))
12751275
end
12761276

1277-
it 'returns 422 when the binding already exists' do
1278-
api_call.call admin_headers
1279-
expect(last_response.status).to eq(201).or eq(202)
1277+
context 'when only one binding per app and service instance is allowed' do
1278+
before do
1279+
TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1)
12801280

1281-
api_call.call admin_headers
1281+
it 'returns 422 when the binding already exists' do
1282+
api_call.call admin_headers
1283+
expect(last_response.status).to eq(201).or eq(202)
12821284

1283-
expect(last_response).to have_status_code(422)
1284-
expect(parsed_response['errors']).to include(include({
1285-
'detail' => include('The app is already bound to the service instance'),
1286-
'title' => 'CF-UnprocessableEntity',
1287-
'code' => 10_008
1288-
}))
1285+
api_call.call admin_headers
1286+
1287+
expect(last_response).to have_status_code(422)
1288+
expect(parsed_response['errors']).to include(include({
1289+
'detail' => include('The app is already bound to the service instance'),
1290+
'title' => 'CF-UnprocessableEntity',
1291+
'code' => 10_008
1292+
}))
1293+
end
1294+
end
12891295
end
12901296

12911297
context 'when the service instance does not exist' do

‎spec/unit/controllers/runtime/apps_controller_spec.rb‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2496,18 +2496,6 @@ def delete_app
24962496
ServiceBinding.make(service_instance: si2, app: process2.app, name: 'free')
24972497
end
24982498

2499-
it 'binding si2 to process1 with a name in use by process1 is not ok' do
2500-
expect do
2501-
ServiceBinding.make(service_instance: si2, app: process1.app, name: 'out')
2502-
end.to raise_error(Sequel::ValidationFailed, /App binding names must be unique\./)
2503-
end
2504-
2505-
it 'binding si1 to process1 with a new name is not ok' do
2506-
expect do
2507-
ServiceBinding.make(service_instance: si1, app: process1.app, name: 'gravy')
2508-
end.to raise_error(Sequel::ValidationFailed, 'The app is already bound to the service.')
2509-
end
2510-
25112499
it 'binding si2 to process1 with a name in use by process2 is ok' do
25122500
ServiceBinding.make(service_instance: si2, app: process1.app, name: 'free')
25132501
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‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ module VCAP::CloudController
2525
it { is_expected.to validate_presence :app }
2626
it { is_expected.to validate_presence :service_instance }
2727
it { is_expected.to validate_db_presence :credentials }
28-
it { is_expected.to validate_uniqueness %i[app_guid service_instance_guid], message: 'The app is already bound to the service.' }
2928
it { is_expected.to validate_presence [:type] }
3029

3130
it 'validates max length of name' do
@@ -80,19 +79,6 @@ module VCAP::CloudController
8079
let(:app) { AppModel.make }
8180
let(:service_instance) { ServiceInstance.make(space: app.space) }
8281

83-
context 'and the name is not null' do
84-
let(:existing_binding) do
85-
ServiceBinding.make(app: app, name: 'some-name', service_instance: service_instance, type: 'app')
86-
end
87-
88-
it 'adds a uniqueness error' do
89-
other_service_instance = ServiceInstance.make(space: existing_binding.space)
90-
conflict = ServiceBinding.new(app: existing_binding.app, name: existing_binding.name, service_instance: other_service_instance, type: 'app')
91-
expect(conflict.valid?).to be(false)
92-
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\'.'])
93-
end
94-
end
95-
9682
context 'and the name is null' do
9783
let(:existing_binding) do
9884
ServiceBinding.make(app: app, name: nil, service_instance: service_instance, type: 'app')

0 commit comments

Comments
 (0)