Skip to content
This repository was archived by the owner on Jun 2, 2021. It is now read-only.

Commit f7134bd

Browse files
pivotal-marcela-campopivotal-marcela-campo
committed
Avoid duplicate service offering names when updating broker
[#172672538](https://www.pivotaltracker.com/story/show/172672538)
1 parent 5c99e81 commit f7134bd

2 files changed

Lines changed: 143 additions & 21 deletions

File tree

‎lib/services/service_brokers/v2/catalog.rb‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,37 @@ def validate_all_service_ids_are_unique
6464
end
6565

6666
def validate_all_service_names_are_unique
67-
if has_duplicates?(services.map(&:name))
67+
new_services_names = services.map(&:name)
68+
if has_duplicates?(new_services_names)
6869
validation_errors.add('Service names must be unique within a broker')
6970
end
71+
72+
if service_broker.exists?
73+
taken_names = taken_names(new_services_names)
74+
if !taken_names.empty?
75+
validation_errors.add("Service names must be unique within a broker. Services with names #{taken_names} already exist")
76+
end
77+
end
78+
end
79+
80+
def taken_names(new_services_names)
81+
clashing_names = []
82+
clashing_services = service_broker.services_dataset.where(label: new_services_names)
83+
clashing_services.each do |old_service|
84+
new_service = services.detect { |ns| ns.name == old_service.name }
85+
next if updating_service?(new_service, old_service)
86+
87+
clashing_names << new_service.name if !can_delete_service?(old_service)
88+
end
89+
clashing_names
90+
end
91+
92+
def can_delete_service?(service)
93+
service.service_plans_dataset.map(&:service_instances_dataset).map(&:count).all?(0)
94+
end
95+
96+
def updating_service?(new_service, old_service)
97+
new_service.broker_provided_id == old_service.unique_id
7098
end
7199

72100
def has_duplicates?(array)

‎spec/unit/lib/services/service_brokers/v2/catalog_spec.rb‎

Lines changed: 114 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -80,38 +80,132 @@ def build_service(attrs={})
8080
end
8181
end
8282

83-
context 'when two services in the catalog have the same name' do
84-
let(:catalog_hash) do
85-
{
83+
context 'unique service names' do
84+
context 'when two services in the catalog have the same name' do
85+
let(:catalog_hash) do
86+
{
8687
'services' => [
8788
build_service('id' => '1', 'name' => 'my-service'),
8889
build_service('id' => '2', 'name' => 'my-service')
8990
]
91+
}
92+
end
93+
94+
it 'gives an error' do
95+
catalog = Catalog.new(broker, catalog_hash)
96+
expect(catalog.valid?).to eq false
97+
expect(catalog.errors.messages).to include('Service names must be unique within a broker')
98+
end
99+
end
100+
101+
context 'when the broker is being created and has not yet been persisted' do
102+
let(:broker) {
103+
VCAP::CloudController::ServiceBroker.new(name: 'not-persisted')
90104
}
105+
let(:catalog_hash) do
106+
{
107+
'services' => [build_service('id' => '1')]
108+
}
109+
end
110+
111+
it 'is does not check for preexistent services' do
112+
catalog = Catalog.new(broker, catalog_hash)
113+
114+
expect(catalog.valid?).to eq true
115+
end
91116
end
92117

93-
it 'gives an error' do
94-
catalog = Catalog.new(broker, catalog_hash)
95-
expect(catalog.valid?).to eq false
96-
expect(catalog.errors.messages).to include('Service names must be unique within a broker')
118+
context 'when a service in the catalog has the same name as a preexistent one for same broker' do
119+
context 'when ids provided by the broker are different' do
120+
context 'when there are service instances for a plan of that offering' do
121+
let(:new_catalog_hash) do
122+
{
123+
'services' => [
124+
build_service('id' => '1', 'name' => 'clashing-service-name'),
125+
build_service('id' => '2', 'name' => 'clashing-service-name2')
126+
]
127+
}
128+
end
129+
let(:broker) {
130+
broker = VCAP::CloudController::ServiceBroker.make
131+
old_service = VCAP::CloudController::Service.make(label: 'clashing-service-name', service_broker: broker)
132+
old_plan = VCAP::CloudController::ServicePlan.make(service: old_service)
133+
VCAP::CloudController::ManagedServiceInstance.make(service_plan: old_plan)
134+
135+
old_service = VCAP::CloudController::Service.make(label: 'clashing-service-name2', service_broker: broker)
136+
old_plan = VCAP::CloudController::ServicePlan.make(service: old_service)
137+
VCAP::CloudController::ManagedServiceInstance.make(service_plan: old_plan)
138+
139+
broker
140+
}
141+
142+
it 'is invalid' do
143+
catalog = Catalog.new(broker, new_catalog_hash)
144+
145+
expect(catalog.valid?).to be false
146+
expect(catalog.errors.messages).to include(include('Services with names ["clashing-service-name", "clashing-service-name2"] already exist'))
147+
end
148+
end
149+
150+
context 'when there are no service instances for a plan of that offering' do
151+
let(:new_catalog_hash) do
152+
{
153+
'services' => [build_service('id' => '1', 'name' => 'clashing-service-name')]
154+
}
155+
end
156+
let(:broker) { VCAP::CloudController::ServiceBroker.make }
157+
let(:old_service) { VCAP::CloudController::Service.make(label: 'clashing-service-name', service_broker: broker) }
158+
let!(:old_plan) { VCAP::CloudController::ServicePlan.make(service: old_service) }
159+
160+
it 'is valid' do
161+
catalog = Catalog.new(broker, new_catalog_hash)
162+
163+
expect(catalog.valid?).to be true
164+
end
165+
end
166+
end
167+
168+
context 'when ids provided by the broker are the same' do
169+
let(:broker) { VCAP::CloudController::ServiceBroker.make }
170+
let(:old_service) { VCAP::CloudController::Service.make(label: 'clashing-service-name', service_broker: broker) }
171+
let(:new_catalog_hash) do
172+
{
173+
'services' => [build_service('id' => old_service.unique_id, 'name' => old_service.label)]
174+
}
175+
end
176+
let(:old_plan) { VCAP::CloudController::ServicePlan.make(service: old_service) }
177+
let!(:instance) { VCAP::CloudController::ManagedServiceInstance.make(service_plan: old_plan) }
178+
179+
it 'is valid' do
180+
catalog = Catalog.new(broker, new_catalog_hash)
181+
182+
expect(catalog.valid?).to be true
183+
end
184+
end
97185
end
98-
end
99186

100-
context 'when a service in the catalog has the same id as a service from a different broker' do
101-
let(:catalog_hash) do
102-
{
103-
'services' => [build_service('id' => '1'), build_service('id' => '2')]
187+
context 'when a service in the catalog has the same name as a service from a different broker' do
188+
let(:catalog_hash) do
189+
{
190+
'services' => [build_service('id' => '1'), build_service('id' => '2')]
191+
}
192+
end
193+
let(:broker) { VCAP::CloudController::ServiceBroker.make }
194+
195+
let(:another_broker) {
196+
broker = VCAP::CloudController::ServiceBroker.make
197+
old_service = VCAP::CloudController::Service.make(name: '1', service_broker: broker)
198+
old_plan = VCAP::CloudController::ServicePlan.make(service: old_service)
199+
VCAP::CloudController::ManagedServiceInstance.make(service_plan: old_plan)
200+
201+
broker
104202
}
105-
end
106-
let(:broker) { VCAP::CloudController::ServiceBroker.make }
107-
let(:another_broker) { VCAP::CloudController::ServiceBroker.make }
108203

109-
it 'is valid' do
110-
existing_catalog = Catalog.new(another_broker, catalog_hash)
111-
catalog = Catalog.new(broker, catalog_hash)
204+
it 'is valid' do
205+
catalog = Catalog.new(broker, catalog_hash)
112206

113-
expect(catalog.valid?).to eq true
114-
expect(existing_catalog.valid?).to eq true
207+
expect(catalog.valid?).to eq true
208+
end
115209
end
116210
end
117211

0 commit comments

Comments
 (0)