V3(services) Refactor handling last operation on binding (cloudfoundry#1907)

[#174748988](https://www.pivotaltracker.com/story/show/174748988)
* Added error handling in the client layer for the last operation response

By concentrating the logic in the client response - it keeps it closer to the response parser that is already doing some of that translation logic. The polling code should only care about the state field and any other raised error should be handled as fatal failure

* Replace old poll implementation with the new one

* Fix formatting

Author: FelisiaM

app/actions/v3/service_binding_create.rb

@@ -21,63 +21,60 @@ def bind(binding, parameters: {}, accepts_incomplete: false)
           complete_binding_and_save(binding, details[:binding], { state: 'succeeded' })
         end
       rescue => e
-        binding.save_with_attributes_and_new_operation(
-          {},
-          {
-            type: 'create',
-            state: 'failed',
-            description: e.message,
-          }
-        )
+        save_failed_state(binding, e)
 
         raise e
       end
 
       def poll(binding)
         client = VCAP::Services::ServiceClientProvider.provide(instance: binding.service_instance)
-        details = fetch_last_operation(client, binding)
-        return ContinuePolling.call(nil) unless details
+        details = client.fetch_and_handle_service_binding_last_operation(binding)
 
-        if details[:last_operation][:state] == 'succeeded'
+        case details[:last_operation][:state]
+        when 'succeeded'
           params = client.fetch_service_binding(binding)
           complete_binding_and_save(binding, params, details[:last_operation])
           return PollingFinished
+        when 'in progress'
+          save_last_operation(binding, details)
+          ContinuePolling.call(details[:retry_after])
+        when 'failed'
+          save_last_operation(binding, details)
+          raise LastOperationFailedState
         end
+      rescue LastOperationFailedState => e
+        raise e
+      rescue => e
+        save_failed_state(binding, e)
+        raise e
+      end
+
+      class BindingNotRetrievable < StandardError; end
 
+      private
+
+      def save_failed_state(binding, e)
         binding.save_with_attributes_and_new_operation(
           {},
           {
             type: 'create',
-            state: details[:last_operation][:state],
-            description: details[:last_operation][:description],
+            state: 'failed',
+            description: e.message,
           }
         )
+      end
 
-        if details[:last_operation][:state] == 'failed'
-          raise LastOperationFailedState.new(details[:last_operation][:description])
-        end
-
-        if binding.reload.terminal_state?
-          PollingFinished
-        else
-          ContinuePolling.call(details[:retry_after])
-        end
-      rescue => e
+      def save_last_operation(binding, details)
         binding.save_with_attributes_and_new_operation(
           {},
           {
             type: 'create',
-            state: 'failed',
-            description: e.message,
+            state: details[:last_operation][:state],
+            description: details[:last_operation][:description],
           }
         )
-        raise e
       end
 
-      class BindingNotRetrievable < StandardError; end
-
-      private
-
       def save_incomplete_binding(precursor, operation)
         precursor.save_with_attributes_and_new_operation(
           {},

lib/services/service_brokers/v2/client.rb

@@ -282,6 +282,24 @@ def fetch_service_binding_last_operation(service_binding)
       end
     end
 
+    def fetch_and_handle_service_binding_last_operation(service_binding)
+      fetch_service_binding_last_operation(service_binding)
+    rescue Errors::HttpClientTimeout,
+           Errors::ServiceBrokerApiUnreachable,
+           HttpRequestError,
+           Errors::ServiceBrokerBadResponse,
+           Errors::ServiceBrokerRequestRejected,
+           Errors::ServiceBrokerApiAuthenticationFailed,
+           Errors::ServiceBrokerResponseMalformed,
+           HttpResponseError
+      result = {}
+      result[:last_operation] = {}
+      result[:last_operation][:state] = 'in progress'
+      result
+    rescue => e
+      raise e.exception("Service binding polling #{service_binding.guid}: #{e.message}")
+    end
+
     def fetch_service_instance(instance)
       path = service_instance_resource_path(instance)
       response = @http_client.get(path)

spec/support/shared_examples/jobs/create_binding_job.rb

@@ -28,7 +28,7 @@
     let(:action) do
       double('BindingAction', {
         bind: nil,
-        poll: poll_response,
+        poll: poll_response
       })
     end
 

spec/support/shared_examples/request/last_operation_response_handling.rb

@@ -37,7 +37,7 @@
           binding.reload
           expect(binding.last_operation.type).to eq(operation_type)
           expect(binding.last_operation.state).to eq('in progress')
-          expect(binding.last_operation.description).to include("Status Code: #{code}")
+          expect(binding.last_operation.description).to be_nil
 
           expect(job.state).to eq(VCAP::CloudController::PollableJobModel::POLLING_STATE)
         end

spec/support/shared_examples/v3_service_binding_create.rb

@@ -125,51 +125,18 @@ class BadError < StandardError; end
 
     before do
       allow(VCAP::Services::ServiceBrokers::V2::Client).to receive(:new).and_return(broker_client)
-      allow(broker_client).to receive(:fetch_service_binding_last_operation).and_return(fetch_last_operation_response)
+      allow(broker_client).to receive(:fetch_and_handle_service_binding_last_operation).and_return(fetch_last_operation_response)
 
       action.bind(binding, accepts_incomplete: true)
     end
 
     it 'fetches the last operation' do
       action.poll(binding)
 
-      expect(broker_client).to have_received(:fetch_service_binding_last_operation).with(binding)
+      expect(broker_client).to have_received(:fetch_and_handle_service_binding_last_operation).with(binding)
     end
 
-    context 'fetching last operations fails' do
-      let(:response) {
-        double('resp', body: '{"description":"no no no"}', code: 422, message: 'failed')
-      }
-      it 'should continue polling for known errors' do
-        [
-          VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerBadResponse.new('uri', 'PUT', response),
-          VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerRequestRejected.new('uri', 'PUT', response),
-          HttpRequestError.new('no no no', 'uri', 'PUT', nil)
-        ].each do |error|
-          allow(broker_client).to receive(:fetch_service_binding_last_operation).and_raise(error)
-
-          status = action.poll(binding)
-          expect(status.finished).to be_falsey
-
-          binding.reload
-          expect(binding.last_operation.state).to eq('in progress')
-          expect(binding.last_operation.description).to include('no no no')
-        end
-      end
-
-      it 'should stop polling for other errors' do
-        allow(broker_client).to receive(:fetch_service_binding_last_operation).and_raise(RuntimeError)
-
-        expect { action.poll(binding) }.to raise_error(RuntimeError)
-
-        binding.reload
-        expect(binding.last_operation.type).to eq('create')
-        expect(binding.last_operation.state).to eq('failed')
-        expect(binding.last_operation.description).to eq('RuntimeError')
-      end
-    end
-
-    context 'response says complete' do
+    context 'last operation state is complete' do
       let(:description) { Sham.description }
       let(:state) { 'succeeded' }
 
@@ -211,7 +178,9 @@ class BadError < StandardError; end
       end
     end
 
-    context 'response says in progress' do
+    context 'last operation state is in progress' do
+      let(:state) { 'in progress' }
+
       it 'returns false' do
         polling_status = action.poll(binding)
         expect(polling_status[:finished]).to be_falsey
@@ -226,7 +195,7 @@ class BadError < StandardError; end
       end
     end
 
-    context 'response says failed' do
+    context 'last operation state is failed' do
       let(:state) { 'failed' }
 
       it 'updates the last operation' do
@@ -238,6 +207,21 @@ class BadError < StandardError; end
       end
     end
 
+    context 'fetching last operations fails' do
+      before do
+        allow(broker_client).to receive(:fetch_and_handle_service_binding_last_operation).and_raise(RuntimeError.new('some error'))
+      end
+
+      it 'should stop polling for other errors' do
+        expect { action.poll(binding) }.to raise_error(RuntimeError)
+
+        binding.reload
+        expect(binding.last_operation.type).to eq('create')
+        expect(binding.last_operation.state).to eq('failed')
+        expect(binding.last_operation.description).to eq('some error')
+      end
+    end
+
     context 'retry interval' do
       context 'no retry interval' do
         it 'returns nil' do

spec/unit/actions/service_credential_binding_create_spec.rb

@@ -284,7 +284,7 @@ module V3
               VCAP::Services::ServiceBrokers::V2::Client,
               {
                 bind: bind_response,
-                fetch_service_binding_last_operation: fetch_last_operation_response,
+                fetch_and_handle_service_binding_last_operation: fetch_last_operation_response,
                 fetch_service_binding: fetch_binding_response,
               }
             )

spec/unit/actions/service_route_binding_create_spec.rb

@@ -256,7 +256,7 @@ module V3
               VCAP::Services::ServiceBrokers::V2::Client,
               {
                 bind: bind_response,
-                fetch_service_binding_last_operation: fetch_last_operation_response,
+                fetch_and_handle_service_binding_last_operation: fetch_last_operation_response,
                 fetch_service_binding: fetch_binding_response,
               }
             )