v3(services): revert synchronous broker name updates (#2046)

- the CF CLI always expects a job from a service broker rename
- we therefore should always return a job, rather than HTTP 200
- it's ok to return HTTP 200 without a job for metadata updates
as the CF CLI (cf set-label) will handle this

[#169090128](https://www.pivotaltracker.com/story/show/169090128)

app/actions/v3/service_broker_update.rb

@@ -15,23 +15,31 @@ def initialize(service_broker, message, service_event_repository)
         @service_event_repository = service_event_repository
       end
 
+      # Technically a name change can be done without contacting the broker. However the CF CLI v7.2
+      # (which is the most recent version at the time of writing) expects a broker rename to return a job.
+      # Once CF CLI v7.2 is out of support, then it may make sense to allow name changes to happen
+      # synchronously.
       def update_broker_needed?
-        message.requested?(:url) || message.requested?(:authentication)
+        message.requested?(:url) || message.requested?(:authentication) || message.requested?(:name)
       end
 
       def update_sync
         ServiceBroker.db.transaction do
-          broker.update(process_name(message))
           MetadataUpdate.update(broker, message)
         end
       end
 
       def enqueue_update
-        params = process_name(message)
+        params = {}
         params[:broker_url] = message.url if message.requested?(:url)
         params[:authentication] = message.authentication.to_json if message.requested?(:authentication)
         params[:service_broker_id] = broker.id
 
+        if message.requested?(:name)
+          unique_name! if ServiceBroker.where(name: message.name).exclude(guid: broker.guid).any?
+          params[:name] = message.name
+        end
+
         if broker.in_transitional_state?
           raise InvalidServiceBroker.new('Cannot update a broker when other operation is already in progress')
         end
@@ -57,16 +65,6 @@ def enqueue_update
 
       attr_reader :broker, :service_event_repository, :message
 
-      def process_name(message)
-        params = {}
-        if message.requested?(:name)
-          unique_name! if ServiceBroker.where(name: message.name).exclude(guid: broker.guid).any?
-          params[:name] = message.name
-        end
-
-        params
-      end
-
       def unique_name!
         raise InvalidServiceBroker.new('Name must be unique')
       end

spec/request/service_brokers_spec.rb

@@ -530,7 +530,7 @@ def expect_empty_list(user_headers)
       end
     end
 
-    context 'when updating url or authentication' do
+    context 'when updating url, authentication, or name' do
       let(:update_request_body) {
         {
           name: 'new-name',
@@ -695,11 +695,9 @@ def expect_empty_list(user_headers)
       end
     end
 
-    context 'when updating name or metadata only' do
-      let(:new_name) { Sham.name }
+    context 'when metadata only' do
       let(:update_request_body) {
         {
-          name: new_name,
           metadata: {
             labels: { potato: 'sweet' },
             annotations: { style: 'mashed', amount: 'all' }
@@ -712,7 +710,7 @@ def expect_empty_list(user_headers)
         expect(last_response).to have_status_code(200)
 
         broker.reload
-        expect(broker.name).to eq(new_name)
+        expect(broker.name).to eq('broker name')
         expect(broker.broker_url).to eq('http://example.org/broker-url')
         expect(broker.auth_username).to eq('admin')
         expect(broker.auth_password).to eq('welcome')
@@ -732,7 +730,7 @@ def expect_empty_list(user_headers)
         expect(parsed_response).to match_json_response(
           {
             guid: broker.guid,
-            name: new_name,
+            name: broker.name,
             url: broker.broker_url,
             created_at: iso8601,
             updated_at: iso8601,
@@ -750,18 +748,6 @@ def expect_empty_list(user_headers)
         )
       end
 
-      context 'when a broker with the same name exists' do
-        before do
-          VCAP::CloudController::ServiceBroker.make(name: 'another broker')
-        end
-
-        it 'should return 422 and meaningful error and does not create a broker' do
-          patch("/v3/service_brokers/#{broker.guid}", { name: 'another broker' }.to_json, admin_headers)
-          expect_error(status: 422, error: 'UnprocessableEntity', description: 'Name must be unique')
-          expect(broker.reload.name).to eq 'broker name'
-        end
-      end
-
       context 'when an operation is already in progress' do
         it 'updates the broker anyway' do
           broker.update(state: VCAP::CloudController::ServiceBrokerStateEnum::SYNCHRONIZING)
@@ -770,7 +756,11 @@ def expect_empty_list(user_headers)
           expect(last_response).to have_status_code(200)
 
           broker.reload
-          expect(broker.name).to eq(new_name)
+          expect(broker).to have_labels({ prefix: nil, key: 'potato', value: 'sweet' })
+          expect(broker).to have_annotations(
+            { prefix: nil, key: 'style', value: 'mashed' },
+            { prefix: nil, key: 'amount', value: 'all' }
+          )
           expect(broker.state).to eq(VCAP::CloudController::ServiceBrokerStateEnum::SYNCHRONIZING)
         end
       end

spec/unit/actions/v3/service_broker_update_spec.rb

@@ -30,8 +30,8 @@ module CloudController
         context 'name' do
           let(:request) { { name: 'new-name' } }
 
-          it 'is false' do
-            expect(action.update_broker_needed?).to be_falsey
+          it 'is true' do
+            expect(action.update_broker_needed?).to be_truthy
           end
         end
 
@@ -79,7 +79,6 @@ module CloudController
       describe '#update_sync' do
         let(:request) do
           {
-            name: 'new-name',
             metadata: {
               labels: { potato: 'yam' },
               annotations: { style: 'mashed' }
@@ -91,8 +90,7 @@ module CloudController
           action.update_sync
         end
 
-        it 'updates name and metadata' do
-          expect(existing_service_broker.name).to eq('new-name')
+        it 'updates metadata' do
           expect(existing_service_broker).to have_labels({ key: 'potato', value: 'yam' })
           expect(existing_service_broker).to have_annotations({ key: 'style', value: 'mashed' })
         end