v3(services): all bindings should have last_operation (cloudfoundry#1863)

When V3 bindings work is complete all binding types should have a
last_operation. However bindings created in V2 will not have this. To
make the output JSON for V3 GET endpoints consistent, we should patch up
missing last_operation objects, rather than present `null` which can be
harder to parse.

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

Author: blgm

app/presenters/mixins/last_operation_helper.rb

@@ -0,0 +1,21 @@
+module VCAP::CloudController::Presenters::Mixins
+  module LastOperationHelper
+    def last_operation(resource)
+      last_operation = resource.try(:last_operation)
+
+      if last_operation
+        last_operation.to_hash
+      else
+        # Bindings created using V2 may not have a last operation, so we make
+        # a best attempt so that the output JSON can be parsed consistently
+        {
+          type: 'create',
+          state: 'succeeded',
+          description: '',
+          created_at: resource.try(:created_at),
+          updated_at: resource.try(:updated_at),
+        }
+      end
+    end
+  end
+end

app/presenters/v3/service_credential_binding_presenter.rb

@@ -1,32 +1,40 @@
 require_relative 'base_presenter'
+require 'presenters/mixins/last_operation_helper'
 
 module VCAP
   module CloudController
     module Presenters
       module V3
         class ServiceCredentialBindingPresenter < BasePresenter
+          include VCAP::CloudController::Presenters::Mixins::LastOperationHelper
+
           def to_hash
             base_hash.merge(extra).merge(decorations)
           end
 
           private
 
+          def binding
+            @resource
+          end
+
           def base_hash
             {
-              guid: @resource.guid,
-              created_at: @resource.created_at,
-              updated_at: @resource.updated_at,
-              name: @resource.name,
-              type: type
+              guid: binding.guid,
+              created_at: binding.created_at,
+              updated_at: binding.updated_at,
+              name: binding.name,
+              type: type,
+              last_operation: last_operation(binding),
             }
           end
 
           def decorations
-            @decorators.reduce({}) { |memo, d| d.decorate(memo, [@resource]) }
+            @decorators.reduce({}) { |memo, d| d.decorate(memo, [binding]) }
           end
 
           def type
-            case @resource
+            case binding
             when VCAP::CloudController::ServiceKey
               'key'
             when VCAP::CloudController::ServiceBinding
@@ -38,65 +46,49 @@ def extra
             case type
             when 'app'
               {
-                last_operation: last_operation,
                 relationships: base_relationships.merge(app_relationship),
                 links: base_links.merge(app_link)
               }
             when 'key'
               {
-                last_operation: nil,
                 relationships: base_relationships,
                 links: base_links
               }
             end
           end
 
-          def last_operation
-            return nil if @resource.last_operation.blank?
-
-            last_operation = @resource.last_operation
-
-            {
-              type: last_operation.type,
-              state: last_operation.state,
-              description: last_operation.description,
-              created_at: last_operation.created_at,
-              updated_at: last_operation.updated_at
-            }
-          end
-
           def base_links
-            parameters = { parameters: "#{path_to_self}/parameters" } unless @resource.service_instance.user_provided_instance?
+            parameters = { parameters: "#{path_to_self}/parameters" } unless binding.service_instance.user_provided_instance?
 
             {
               self: path_to_self,
               details: "#{path_to_self}/details",
-              service_instance: "/v3/service_instances/#{@resource.service_instance_guid}"
+              service_instance: "/v3/service_instances/#{binding.service_instance_guid}"
             }.merge(parameters || {}).transform_values do |path|
               hrefify(path)
             end
           end
 
           def path_to_self
-            "/v3/service_credential_bindings/#{@resource.guid}"
+            "/v3/service_credential_bindings/#{binding.guid}"
           end
 
           def base_relationships
             {
-              service_instance: { data: { guid: @resource.service_instance_guid } }
+              service_instance: { data: { guid: binding.service_instance_guid } }
             }
           end
 
           def app_relationship
-            return {} if @resource.app_guid.blank?
+            return {} if binding.app_guid.blank?
 
-            { app: { data: { guid: @resource.app_guid } } }
+            { app: { data: { guid: binding.app_guid } } }
           end
 
           def app_link
-            return {} if @resource.app_guid.blank?
+            return {} if binding.app_guid.blank?
 
-            { app: hrefify("/v3/apps/#{@resource.app_guid}") }
+            { app: hrefify("/v3/apps/#{binding.app_guid}") }
           end
 
           def hrefify(path)

app/presenters/v3/service_route_binding_presenter.rb

@@ -1,10 +1,13 @@
 require_relative 'base_presenter'
+require 'presenters/mixins/last_operation_helper'
 
 module VCAP
   module CloudController
     module Presenters
       module V3
         class ServiceRouteBindingPresenter < BasePresenter
+          include VCAP::CloudController::Presenters::Mixins::LastOperationHelper
+
           def to_hash
             base.merge(decorations)
           end
@@ -17,7 +20,7 @@ def base
               route_service_url: binding.route_service_url,
               created_at: binding.created_at,
               updated_at: binding.updated_at,
-              last_operation: last_operation,
+              last_operation: last_operation(binding),
               relationships: relationships,
               links: links
             }
@@ -31,20 +34,6 @@ def binding
             @resource
           end
 
-          def last_operation
-            return nil if binding.last_operation.blank?
-
-            last_operation = binding.last_operation
-
-            {
-              type: last_operation.type,
-              state: last_operation.state,
-              description: last_operation.description,
-              created_at: last_operation.created_at,
-              updated_at: last_operation.updated_at
-            }
-          end
-
           def links
             {
               self: {

spec/request/service_credential_bindings_spec.rb

@@ -1296,7 +1296,7 @@ def expected_json(binding)
       created_at: iso8601,
       updated_at: iso8601,
       name: binding.name,
-      last_operation: nil,
+      last_operation: last_operation(binding),
       relationships: {
         service_instance: {
           data: {
@@ -1350,14 +1350,22 @@ def extra(binding)
   end
 
   def last_operation(binding)
-    if binding.last_operation.present?
+    if binding.try(:last_operation)
       {
         type: binding.last_operation.type,
         state: binding.last_operation.state,
         description: binding.last_operation.description,
         created_at: iso8601,
         updated_at: iso8601
       }
+    else
+      {
+        type: 'create',
+        state: 'succeeded',
+        description: '',
+        created_at: iso8601,
+        updated_at: iso8601
+      }
     end
   end
 

spec/unit/presenters/v3/service_credential_binding_presenter_spec.rb

@@ -22,7 +22,7 @@ module CloudController
 
         it 'should include the binding fields plus links and relationships' do
           presenter = described_class.new(credential_binding)
-          expect(presenter.to_hash).to match(
+          expect(presenter.to_hash.with_indifferent_access).to match(
             {
               guid: 'some-guid',
               type: 'app',
@@ -78,6 +78,25 @@ module CloudController
             expect(presenter.to_hash[:name]).to be_nil
           end
         end
+
+        context 'no last_operation' do
+          let(:credential_binding) do
+            ServiceBinding.make(name: 'some-name', guid: 'some-guid', app: app, service_instance: instance)
+          end
+
+          it 'still displays the last operation' do
+            presenter = described_class.new(credential_binding)
+            expect(presenter.to_hash[:last_operation]).to match(
+              {
+                type: 'create',
+                state: 'succeeded',
+                description: '',
+                updated_at: credential_binding.updated_at,
+                created_at: credential_binding.created_at
+              }
+            )
+          end
+        end
       end
 
       describe 'key bindings' do
@@ -94,7 +113,13 @@ module CloudController
               name: 'some-name',
               created_at: credential_binding.created_at,
               updated_at: credential_binding.updated_at,
-              last_operation: nil,
+              last_operation: {
+                type: 'create',
+                state: 'succeeded',
+                description: '',
+                updated_at: credential_binding.updated_at,
+                created_at: credential_binding.created_at
+              },
               relationships: {
                 service_instance: {
                   data: {
@@ -119,6 +144,25 @@ module CloudController
             }
           )
         end
+
+        context 'no last_operation' do
+          let(:credential_binding) do
+            ServiceKey.make(name: 'some-name', guid: 'some-guid', service_instance: instance)
+          end
+
+          it 'still displays the last operation' do
+            presenter = described_class.new(credential_binding)
+            expect(presenter.to_hash[:last_operation]).to match(
+              {
+                type: 'create',
+                state: 'succeeded',
+                description: '',
+                updated_at: credential_binding.updated_at,
+                created_at: credential_binding.created_at
+              }
+            )
+          end
+        end
       end
 
       describe 'for user provided service instances' do

spec/unit/presenters/v3/service_route_binding_presenter_spec.rb

@@ -27,7 +27,7 @@ module CloudController
 
       it 'presents the correct object' do
         presenter = described_class.new(binding)
-        expect(presenter.to_hash).to match(
+        expect(presenter.to_hash.with_indifferent_access).to match(
           {
             guid: guid,
             created_at: binding.created_at,
@@ -67,6 +67,30 @@ module CloudController
         )
       end
 
+      context 'no last_operation' do
+        let(:binding) do
+          RouteBinding.make(
+            guid: guid,
+            service_instance: service_instance,
+            route: route,
+            route_service_url: route_service_url,
+          )
+        end
+
+        it 'still displays the last operation' do
+          presenter = described_class.new(binding)
+          expect(presenter.to_hash[:last_operation]).to match(
+            {
+              type: 'create',
+              state: 'succeeded',
+              description: '',
+              updated_at: binding.updated_at,
+              created_at: binding.created_at
+            }
+          )
+        end
+      end
+
       describe 'decorators' do
         let(:decorator1) { double('FakeDecorator', decorate: { foo: 'bar' }) }
         let(:decorator2) { double('FakeDecorator', decorate: { xyzzy: 'omg' }) }