Skip to content

Commit 84dad61

Browse files
committed
fix: service_broker validation_unique removel is reverted. Since it is used as guard before calling some http endpoint in check in service_broker_registration. Comments are added
1 parent c7786d8 commit 84dad61

5 files changed

Lines changed: 7 additions & 8 deletions

File tree

app/models/runtime/domain.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,8 @@ def around_save
9797

9898
def validate
9999
validates_presence :name
100+
# Keep validates_unique to check across all Domain subclasses (STI: SharedDomain, PrivateDomain).
101+
# Without it, exact name duplicates would be caught by name_overlaps? below with a misleading error message.
100102
validates_unique :name, dataset: Domain.dataset
101103

102104
validates_format CloudController::DomainDecorator::DOMAIN_REGEX, :name,

app/models/services/service_broker.rb

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ def around_save
2525
rescue Sequel::UniqueConstraintViolation => e
2626
raise e unless e.message.include?('service_brokers_name_index')
2727

28-
errors.add(:name, Sequel.lit('Name must be unique'))
28+
errors.add(:name, :unique)
2929
raise validation_failed_error
3030
end
3131

@@ -34,6 +34,9 @@ def validate
3434
validates_presence :broker_url
3535
validates_presence :auth_username
3636
validates_presence :auth_password
37+
# Keep validates_unique as a pre-guard for ServiceBrokerRegistration
38+
# to avoid unnecessary HTTP calls to the broker if the name is not unique.
39+
validates_unique :name, message: Sequel.lit('Name must be unique')
3740
validates_url :broker_url
3841
validates_url_no_basic_auth
3942
end

lib/services/service_brokers/service_broker_registration.rb

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,6 @@ def create
3030
raise e
3131
end
3232
self
33-
rescue Sequel::ValidationFailed
34-
# Errors have been added to the broker model by around_save (e.g. unique constraint violations)
35-
nil
3633
end
3734

3835
def update
@@ -53,9 +50,6 @@ def update
5350
synchronize_services_and_plans!
5451
end
5552
self
56-
rescue Sequel::ValidationFailed
57-
# Errors have been added to the broker model by around_save (e.g. unique constraint violations)
58-
nil
5953
end
6054

6155
delegate :errors, to: :broker

spec/unit/controllers/services/service_brokers_controller_spec.rb

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,6 @@ def stub_catalog(broker_url: nil, username: nil, password: nil)
291291
post '/v2/service_brokers', public_body
292292
expect(last_response).to have_status_code(201)
293293

294-
stub_catalog
295294
post '/v2/service_brokers', body
296295
expect(last_response).to have_status_code(400)
297296
end

spec/unit/models/services/service_broker_spec.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ module VCAP::CloudController
4242
it { is_expected.to validate_presence :broker_url }
4343
it { is_expected.to validate_presence :auth_username }
4444
it { is_expected.to validate_presence :auth_password }
45+
it { is_expected.to validate_uniqueness :name, message: Sequel.lit('Name must be unique') }
4546

4647
it 'validates the url is a valid http/https url' do
4748
expect(broker).to be_valid

0 commit comments

Comments
 (0)